WP/I18n: complete refactor, implement PHPCSUtils, modern PHP and more #2214
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The build failure on PHP 5.5 / PHPCS 3.7.1 is unrelated to this PR and need separate investigation. |
Note: I've just tried to reproduce this locally, but haven't been able to. Not sure it is worth spending much time on. |
Maybe: ? |
Thanks for the suggestion, except I'm not sure if that makes things any clearer - "yes, but it is translatable, I'm using a translation function". I believe it's important for the "translatable content" phrasing to stay to make it clear what the message is about. |
|
I checked the tests (these lines were collapsed in the PR as they were not changed) to understand the context of this message. The message makes more sense now, but I'm not sure I could phrase it any better, so I'm fine with it staying as is 👍🏼 |
dingo-d
approved these changes
Mar 22, 2023
|
I'm not sure why I can't just merge this, we didn't have auto-merge before 😕 |
Auto-merge is a feature which has been around for a while now, but the reason this can't be merged is the failing check on PHP 5.5 against PHPCS 3.7.1 (which I couldn't reproduce locally). I've just restarted the build, who knows it may have been a fluke (update: it was not ❌). So to get round this, there are a couple of options:
Opinions ? |
|
I'm for this option
As this is the quickest fix imo. |
|
What fix is in PHPCS 3.7.2 that means this failure with PHP 5.5 would be addressed? |
That can only be answered via option 3.... but the tests pass with PHPCS 3.7.2/master. |
|
Let's go with 3.7.2 as the minimum. |
The `AbstractFunctionRestrictionsSniff` already checks for a next token and verifies that token is an open parenthesis, this doesn't need to be duplicated.
Some new tests were added _after_ the property reset at the end of the test case file. This commit moves the reset to the end of the file again.
As things were, the `WordPress.WP.I18n` sniff was based on the `AbstractFunctionRestrictionsSniff` class and used legacy custom code to parse the function call to parameters.
In this refactor, the parent sniff is switched to the `AbstractFunctionParameterSniff` class, which uses the PHPCSUtils `PassedParameters` class to parse the function call parameters.
Making this change will allow for supporting function calls using named parameters as supported since PHP 8.0.
As the code in the sniff needed significant changes to account for the difference in the array format containing parameter information, all code in the sniff has been reviewed and in a lot of places, rewritten.
The new code is more modular, has much lower cognitive and cyclomatic complexity and should be easier to read, understand and adjust when changes to the sniff are needed in the future.
Having said that, I have tried to keep the _functional_ changes to a minimum while doing this refactor.
I _have_ addressed some logic errors and low hanging fruit, like making the error messages more informative, but other than that, this refactor does not contain significant functional changes and all error codes remain the same as before.
Notes on the refactor:
* The sniff previously contained some logic for toggling messages from error to warning `$is_error = empty( $context['warning'] );`.
This logic has been removed as the `'warning'` array key was never set or changed anyway.
* The `check_argument_tokens()` method has been split and is replaced by the `check_argument_is_string_literal()` and the `check_textdomain_matches()` methods.
* In contrast to the behaviour previously in the `check_argument_tokens()` method, the `check_argument_is_string_literal()` method will consistently return a boolean value indicating whether or not the parameter examined is a string literal.
* The `check_text()` method has been split and is replaced by the `check_placeholders_in_string()`, `check_string_has_translatable_content()` and the `check_string_has_no_html_wrapper()` methods.
* The `check_textdomain_matches()`, `check_placeholders_in_string()`, `check_string_has_translatable_content()`, `check_string_has_no_html_wrapper()` and the `compare_single_and_plural_arguments()` checks will now only be run if the parameter being examined consists of a text string literal.
In other words, these will only run when there are only one or more `T_CONSTANT_ENCAPSED_STRING` tokens (single line and multi-line string supported) as in all other cases, examining the _contents_ of the tokens will be unreliable and is likely to result in false positives.
It looks like this was always the intention, but was previously not always executed correctly.
Case in point: if the parameter consisted of one non-empty token, even though that token was not a string literal, the `compare_single_and_plural_arguments()` check would previously still be executed.
Along the same line, the checks previously contained in the `check_text()` method were previously only executed when the parameter had exactly one functional token. I.e. they were previously not run on multi-line text strings, but they were run on non-text strings, like a `T_VARIABLE` token.
* Take note: the above also means that the (partial) support for providing certain texts as heredocs/nowdocs has been removed.
If tests would show that heredoc/nowdocs _are_ supported by the `pot` file generators, this should be brought back, but as it was, the support for heredocs/nowdocs in the sniff was incomplete anyhow, so better to not support these at all.
* The logic within the `compare_single_and_plural_arguments()` check has also been adjusted to not throw the `MismatchedPlaceholders` error when the `MissingSingularPlaceholder` is already being thrown.
While we shouldn't be hiding one error behind another, this is not one of those cases and the `MismatchedPlaceholders` error would be unsolvable until the `MissingSingularPlaceholder` is solved anyway.
* The `check_for_translator_comment()` will now only examine parameters for placeholders if the parameter passed is a single string literal.
Includes a couple of extra tests to verify.
Additionally, this refactor includes the following other changes:
* In error messages referring to a parameter name, the correct distinction between `$single` and `$singular` is now made.
Previously, those error messages would reference the `$singular` parameter in the `_n_noop()` and `_nx_noop()` functions as if it were called `$single`. This incorrect parameter name becomes confusing when named parameters are taken into account.
* The error message for the `TooManyFunctionArgs` error code will now be thrown on the function name token instead of on the function call parenthesis opener.
* The error message for the `MissingArg*[Default]` error codes will now be thrown on the function name token instead of on the function call parenthesis opener.
* The error message for the `TooManyFunctionArgs` error code has been updated to be more informative.
Old: `Too many arguments for function "%s".`
New: `Too many parameters passed to function "%s()". Expected: %s parameters, received: %s`
* The error message for the `MissingArg*[Default]` error code has been updated to be more informative.
Old: `Missing $%s arg. [...]`
New: `Missing $%s parameter in function call to %s(). [...]`
* The error message for the `NonSingularStringLiteral*` error code has been updated to be more informative.
Old: `The $%s arg must be a single string literal, not "%s".`
New: `The $%s parameter must be a single text string literal. Found: %s`
* The error message for the `InterpolatedVariable*` error codes has been updated to be more informative.
Old: `The $%s arg must not contain interpolated variables or expressions. Found "%s".`
New: `The $%s parameter must not contain interpolated variables or expressions. Found: %s`
* The error message for the `SuperfluousDefaultTextDomain` error code has been updated to be more informative.
Old: `No need to supply the text domain when the only accepted text domain is "default".`
New: `No need to supply the text domain in function call to %s() when the only accepted text domain is "default".`
* The error messages for the `MixedOrderedPlaceholders*` and `UnorderedPlaceholders*` error codes now use quotes around values in the error messages more consistently.
* The error message for the `MixedOrderedPlaceholders*` error codes has been updated to be more informative.
Old: `Multiple placeholders should be ordered. Mix of ordered and non-ordered placeholders found. Found: %s.`
New: `Multiple placeholders in translatable strings should be ordered. Mix of ordered and non-ordered placeholders found. Found: "%s" in %s.` (where the second %s is the complete parameter contents)
* The error message for the `UnorderedPlaceholders*` error codes has been updated to be more informative.
Old: `Multiple placeholders should be ordered. Expected "%s", but got "%s".`
New: `Multiple placeholders in translatable strings should be ordered. Expected "%s", but got "%s" in %s.` (where the third %s is the complete parameter contents)
* The error message for the `NoEmptyStrings` error code has been updated to be more informative.
Old: `Strings should have translatable content`
New: `The $%s text string should have translatable content. Found: %s`
=> This may read a little awkward for the `$text` parameter. Suggestions for further improvement welcome.
* The error message for the `NoHtmlWrappedStrings` error code has been updated to be more informative.
Old: `Strings should not be wrapped in HTML`
New: `Translatable string should not be wrapped in HTML. Found: %s`
* Also note that the placeholders found will be sorted using "natural sort" now for the `MismatchedPlaceholders` error, which should make the error message more readable for those text strings with lots of placeholders.
* The error message for the MissingTranslatorsCommenterror code has been updated to be more informative.
Old: `A gettext call containing placeholders was found, but was not accompanied by a "translators:" comment on the line above to clarify the meaning of the placeholders.`
New: `A function call to %s() with texts containing placeholders was found, but was not accompanied by a "translators:" comment on the line above to clarify the meaning of the placeholders.`
Extra tests have been added to safeguard the following situations which were previously mostly accounted for, but not safeguarded:
* Ensuring that a missing parameter will be recognized and flagged correctly with `MissingArg*[Default]`, even when there is a placeholder comment in the "parameter space".
* Ensuring that the `SuperfluousDefaultTextDomain` fixer does not remove any potential comments _after_ the `'default'` text domain.
* The `MismatchedPlaceholders` check which did not have dedicated tests until now.
The changes made in the refactor (previous commit) means that function calls using named parameters, as supported since PHP 8.0, should now be handled correctly by the sniff. This commit adds unit tests to the sniff to verify and safeguard this.
The `SuperfluousDefaultTextDomain` fixer already handled this correctly. This commit just adds some tests to safeguard this.
…r named params The auto-fixer for the `SuperfluousDefaultTextDomain` warning attempts to remove a complete parameter, though it will bow out from auto-fixing when there is a comment in the tokens which would be removed. This auto-fixer code and the code to determine whether an auto-fix is possible, now needs adjusting to allow for function calls with named parameters as otherwise the fixer could cause parse errors. * The parameter label and the `:` also needs to be removed. * If the named parameter is the first parameter, there will not be a preceding comma, but we will need to remove the comma behind it. * The range of tokens which needs to be checked for comments to determine whether the error is auto-fixable needs to be expanded. This commit makes those changes. Includes additional unit tests to safeguard the fixes.
The `$check_translator_comments` property was introduced in WPCS `0.11.0` via PR 742 mostly to make our own lives easier managing the test case files. It allowed the pre-existing test case file `1` to remain largely unchanged, while the errors for translators comments were tested in a new test case file `2`. At the time, the ability to use modular ignore annotations, i.e. `// phpcs:disable Sniffcode`, did not exist yet. As those now _do_ exist and the minimum supported PHPCS version is beyond the version in which support for the modular ignore annotations was introduced, we can use those instead in the test case files and no longer need the property. For end-users, the use of the property has always been discouraged as they could exclude the error codes in their ruleset instead, so the impact of this change for end-users is expected to be minimal. The removal of the property should be annotated in the wiki when it is being updated for the release of WPCS 3.0.0. Related 1917.
…xt string As things were, the fixer for the `UnorderedPlaceholders*` error, would replace the first non-empty token in the parameter. While this is fine in the majority of cases, this would lead to a mangled text string in the `fixed` file for multi-line text strings. Fixed now. Includes unit test safeguarding the fix.
Issue 1948 flagged an issue which, while rare, should still be flagged. The default value for the `$domain` parameter for nearly all translation functions is (currently) `'default'`, with the exceptions being `_n_noop()` and `_nx_noop()`, where the default value is `null`. If a function call would pass an empty string as the `$domain`, this would previously already be flagged if the end-user had set one or more valid `text_domain`s in their ruleset. However, if no `text_domain` was set in the ruleset, this would not be flagged by the sniff and as an empty string would overrule the default value for the functions, in effect, the text string would become untranslatable as there is no text domain and the functions would no longer fall back to the WP native `'default'` text domain. I haven't checked if/when these default values were changed in Core, but that shouldn't really matter anyway as, as things currently are, passing an empty text string as the `$domain` is problematic and should be flagged. Fixed now. The behaviour for when a valid `text_domain` was set in the ruleset remains unchanged. Includes unit test. Fixes 1948
jrfnl
force-pushed
the
feature/refactor-18n-sniff
branch
from
edb5b6b
to
f39e7a1
Compare
|
I've rebased the PR without changes to get a passing build. |
19 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WP/I18n: minor code simplification
The
AbstractFunctionRestrictionsSniffalready checks for a next token and verifies that token is an open parenthesis, this doesn't need to be duplicated.WP/I18n: fix property reset in tests
Some new tests were added after the property reset at the end of the test case file.
This commit moves the reset to the end of the file again.
WP/I18n: complete refactor of the sniff
As things were, the
WordPress.WP.I18nsniff was based on theAbstractFunctionRestrictionsSniffclass and used legacy custom code to parse the function call to parameters.In this refactor, the parent sniff is switched to the
AbstractFunctionParameterSniffclass, which uses the PHPCSUtilsPassedParametersclass to parse the function call parameters.Making this change will allow for supporting function calls using named parameters as supported since PHP 8.0.
As the code in the sniff needed significant changes to account for the difference in the array format containing parameter information, all code in the sniff has been reviewed and in a lot of places, rewritten.
The new code is more modular, has much lower cognitive and cyclomatic complexity and should be easier to read, understand and adjust when changes to the sniff are needed in the future.
Having said that, I have tried to keep the functional changes to a minimum while doing this refactor.
I have addressed some logic errors and low hanging fruit, like making the error messages more informative, but other than that, this refactor does not contain significant functional changes and all error codes remain the same as before.
Notes on the refactor:
$is_error = empty( $context['warning'] );.This logic has been removed as the
'warning'array key was never set or changed anyway.check_argument_tokens()method has been split and is replaced by thecheck_argument_is_string_literal()and thecheck_textdomain_matches()methods.check_argument_tokens()method, thecheck_argument_is_string_literal()method will consistently return a boolean value indicating whether or not the parameter examined is a string literal.check_text()method has been split and is replaced by thecheck_placeholders_in_string(),check_string_has_translatable_content()and thecheck_string_has_no_html_wrapper()methods.check_textdomain_matches(),check_placeholders_in_string(),check_string_has_translatable_content(),check_string_has_no_html_wrapper()and thecompare_single_and_plural_arguments()checks will now only be run if the parameter being examined consists of a text string literal.In other words, these will only run when there are only one or more
T_CONSTANT_ENCAPSED_STRINGtokens (single line and multi-line string supported) as in all other cases, examining the contents of the tokens will be unreliable and is likely to result in false positives.It looks like this was always the intention, but was previously not always executed correctly.
Case in point: if the parameter consisted of one non-empty token, even though that token was not a string literal, the
compare_single_and_plural_arguments()check would previously still be executed.Along the same line, the checks previously contained in the
check_text()method were previously only executed when the parameter had exactly one functional token. I.e. they were previously not run on multi-line text strings, but they were run on non-text strings, like aT_VARIABLEtoken.If tests would show that heredoc/nowdocs are supported by the
potfile generators, this should be brought back, but as it was, the support for heredocs/nowdocs in the sniff was incomplete anyhow, so better to not support these at all.compare_single_and_plural_arguments()check has also been adjusted to not throw theMismatchedPlaceholderserror when theMissingSingularPlaceholderis already being thrown.While we shouldn't be hiding one error behind another, this is not one of those cases and the
MismatchedPlaceholderserror would be unsolvable until theMissingSingularPlaceholderis solved anyway.check_for_translator_comment()will now only examine parameters for placeholders if the parameter passed is a single string literal.Includes a couple of extra tests to verify.
Additionally, this refactor includes the following other changes:
$singleand$singularis now made.Previously, those error messages would reference the
$singularparameter in the_n_noop()and_nx_noop()functions as if it were called$single. This incorrect parameter name becomes confusing when named parameters are taken into account.TooManyFunctionArgserror code will now be thrown on the function name token instead of on the function call parenthesis opener.MissingArg*[Default]error codes will now be thrown on the function name token instead of on the function call parenthesis opener.TooManyFunctionArgserror code has been updated to be more informative.Old:
Too many arguments for function "%s".New:
Too many parameters passed to function "%s()". Expected: %s parameters, received: %sMissingArg*[Default]error code has been updated to be more informative.Old:
Missing $%s arg. [...]New:
Missing $%s parameter in function call to %s(). [...]NonSingularStringLiteral*error code has been updated to be more informative.Old:
The $%s arg must be a single string literal, not "%s".New:
The $%s parameter must be a single text string literal. Found: %sInterpolatedVariable*error codes has been updated to be more informative.Old:
The $%s arg must not contain interpolated variables or expressions. Found "%s".New:
The $%s parameter must not contain interpolated variables or expressions. Found: %sSuperfluousDefaultTextDomainerror code has been updated to be more informative.Old:
No need to supply the text domain when the only accepted text domain is "default".New:
No need to supply the text domain in function call to %s() when the only accepted text domain is "default".MixedOrderedPlaceholders*andUnorderedPlaceholders*error codes now use quotes around values in the error messages more consistently.MixedOrderedPlaceholders*error codes has been updated to be more informative.Old:
Multiple placeholders should be ordered. Mix of ordered and non-ordered placeholders found. Found: %s.New:
Multiple placeholders in translatable strings should be ordered. Mix of ordered and non-ordered placeholders found. Found: "%s" in %s.(where the second %s is the complete parameter contents)UnorderedPlaceholders*error codes has been updated to be more informative.Old:
Multiple placeholders should be ordered. Expected "%s", but got "%s".New:
Multiple placeholders in translatable strings should be ordered. Expected "%s", but got "%s" in %s.(where the third %s is the complete parameter contents)NoEmptyStringserror code has been updated to be more informative.Old:
Strings should have translatable contentNew:
The $%s text string should have translatable content. Found: %s=> This may read a little awkward for the
$textparameter. Suggestions for further improvement welcome.NoHtmlWrappedStringserror code has been updated to be more informative.Old:
Strings should not be wrapped in HTMLNew:
Translatable string should not be wrapped in HTML. Found: %sMismatchedPlaceholderserror, which should make the error message more readable for those text strings with lots of placeholders.MissingTranslatorsCommenterror code has been updated to be more informative.Old:
A gettext call containing placeholders was found, but was not accompanied by a "translators:" comment on the line above to clarify the meaning of the placeholders.New:
A function call to %s() with texts containing placeholders was found, but was not accompanied by a "translators:" comment on the line above to clarify the meaning of the placeholders.Extra tests have been added to safeguard the following situations which were previously mostly accounted for, but not safeguarded:
MissingArg*[Default], even when there is a placeholder comment in the "parameter space".SuperfluousDefaultTextDomainfixer does not remove any potential comments after the'default'text domain.MismatchedPlaceholderscheck which did not have dedicated tests until now.WP/I18n: add tests with PHP 8.0+ named parameters
The changes made in the refactor (previous commit) means that function calls using named parameters, as supported since PHP 8.0, should now be handled correctly by the sniff.
This commit adds unit tests to the sniff to verify and safeguard this.
WP/I18n: add tests with PHP 7.3+ trailing commas in function calls
The
SuperfluousDefaultTextDomainfixer already handled this correctly. This commit just adds some tests to safeguard this.WP/I18n: fix SuperfluousDefaultTextDomain auto-fix code to allow for named params
The auto-fixer for the
SuperfluousDefaultTextDomainwarning attempts to remove a complete parameter, though it will bow out from auto-fixing when there is a comment in the tokens which would be removed.This auto-fixer code and the code to determine whether an auto-fix is possible, now needs adjusting to allow for function calls with named parameters as otherwise the fixer could cause parse errors.
:also needs to be removed.This commit makes those changes.
Includes additional unit tests to safeguard the fixes.
WP/I18n: remove the public check_translator_comments property
The
$check_translator_commentsproperty was introduced in WPCS0.11.0via PR #742 mostly to make our own lives easier managing the test case files.It allowed the pre-existing test case file
1to remain largely unchanged, while the errors for translators comments were tested in a new test case file2.At the time, the ability to use modular ignore annotations, i.e.
// phpcs:disable Sniffcode, did not exist yet.As those now do exist and the minimum supported PHPCS version is beyond the version in which support for the modular ignore annotations was introduced, we can use those instead in the test case files and no longer need the property.
For end-users, the use of the property has always been discouraged as they could exclude the error codes in their ruleset instead, so the impact of this change for end-users is expected to be minimal.
The removal of the property should be annotated in the wiki when it is being updated for the release of WPCS 3.0.0. Related #1917.
WP/I18n: bug fix - fixer for UnorderedPlaceholders* could mangle text string
As things were, the fixer for the
UnorderedPlaceholders*error, would replace the first non-empty token in the parameter. While this is fine in the majority of cases, this would lead to a mangled text string in thefixedfile for multi-line text strings.Fixed now.
Includes unit test safeguarding the fix.
WP/I18n: new error: passing empty string as text domain
Issue #1948 flagged an issue which, while rare, should still be flagged.
The default value for the
$domainparameter for nearly all translation functions is (currently)'default', with the exceptions being_n_noop()and_nx_noop(), where the default value isnull.If a function call would pass an empty string as the
$domain, this would previously already be flagged if the end-user had set one or more validtext_domains in their ruleset.However, if no
text_domainwas set in the ruleset, this would not be flagged by the sniff and as an empty string would overrule the default value for the functions, in effect, the text string would become untranslatable as there is no text domain and the functions would no longer fall back to the WP native'default'text domain.I haven't checked if/when these default values were changed in Core, but that shouldn't really matter anyway as, as things currently are, passing an empty text string as the
$domainis problematic and should be flagged.Fixed now.
The behaviour for when a valid
text_domainwas set in the ruleset remains unchanged.Includes unit test.
Fixes #1948