WP/I18n: complete refactor, implement PHPCSUtils, modern PHP and more #2214

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WP/I18n: complete refactor, implement PHPCSUtils, modern PHP and more #2214

WP/I18n: complete refactor, implement PHPCSUtils, modern PHP and more #2214

Commits on Mar 27, 2023