WP.I18n: Fix false positive on method calls. #1267
Conversation
The `WP.I18n` sniff did not properly verify whether the `T_STRING` found was actually a function call, in contrast to a method call or a namespaced function call. I've fixed this now by extending the `AbstractFunctionRestriction` sniff and using the logic contained therein to make sure that only plain function calls are being addressed. Includes unit tests. A future further improvement could be extending the `AbstractFunctionParameter` sniff instead to benefit from using the logic from the `get_function_call_parameters()` utility function. For now, I've just made the function parenthesis check slightly more stable, by making it code style independent and adding an extra safeguard for live coding situations. Fixes #1266
jrfnl
force-pushed
the
feature/1266-fix-method-call-false-positive
branch
from
d10f243 to
c2e7f61
Compare
WordPress/Sniffs/WP/I18nSniff.php
Outdated
| $this->exclude = ''; | ||
|
|
||
| // Note: this will give false positives if a method called `_` is used. | ||
| if ( '_' === $this->tokens[ $stack_ptr ]['content'] ) { |
There was a problem hiding this comment.
Is there a particular reason not to move this check as well, other than the fact that _ isn't a part of the i18n group? Couldn't it be added as part of a second group and checked in the process_matched_token() method? That way it wouldn't catch false-positives from _() methods either. (Which admittedly seems unlikely, but I see no reason not to exclude that possibility.)
There was a problem hiding this comment.
Is there a particular reason not to move this check as well, other than the fact that _ isn't a part of the i18n group?
I didn't move for exactly that reason 😁
Couldn't it be added as part of a second group and checked in the process_matched_token() method? That way it wouldn't catch false-positives from _() methods either.
I think there are more elegant ways to do this, I'll have a fiddle with it.
There was a problem hiding this comment.
Reading your comment again, I now understand what you mean by "second group". Yes, that would actually work perfectly. Changing things now.
There was a problem hiding this comment.
Done. Added second commit. Feel free to squash on merge.
I've also moved the other check which checks for a particular function down to below the parenthesis check as an extra safeguard against false positives.
The
WP.I18nsniff did not properly verify whether theT_STRINGfound was actually a function call, in contrast to a method call or a namespaced function call.I've fixed this now by extending the
AbstractFunctionRestrictionsniff and using the logic contained therein to make sure that only plain function calls are being addressed.Includes unit tests.
A future further improvement could be extending the
AbstractFunctionParametersniff instead to benefit from using the logic from theget_function_call_parameters()utility function.For now, I've just made the function parenthesis check slightly more stable, by making it code style independent and adding an extra safeguard for live coding situations.
Fixes #1266
Related to #763