Ignore return values when a filter callback is abstract #581
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
2 tasks
GaryJones
reviewed
Sep 9, 2020
GaryJones
reviewed
Sep 9, 2020
* Adjust the "abstract method" test to:
1. Expect a warning.
2. Prevent the hook in getting confused with other functions in the same test file.
* Remove the "abstract method implementation" test as it wasn't testing anything.
1. The sniff does not look for child classes, so wouldn't examine that code snippet anyway.
Adding this functionality is not that useful either as in most cases, the child class will not be in the same file as the abstract parent class.
2. And even if the sniff did examine it, it would still not recognize it as a child class as the class doesn't `extend` the abstract.
* Add a test for a typical case where declared functions do not have a scope opener/closer due to a tokenizer bug.
PR squizlabs/PHP_CodeSniffer 3066 is open upstream to fix this.
* Add a "live coding" test case.
|
I've added two additional commits to this PR, updating the unit tests and implementing my suggestion from #580 (comment) |
GaryJones
reviewed
Sep 9, 2020
GaryJones
reviewed
Sep 9, 2020
|
Noting that the new Warning will now appear when there was no warning previously (which was a false negative due to the lack of support for short array syntax). |
Correct. I've also given it a separate error code on purpose with that in mind. Semver-wise, it should probably go into a minor rather than a patch version as it can be seen as a new feature. |
… error This implements the suggestion made in Automattic#580 (comment) If the method a filter callback points to is an abstract method, a warning will be thrown asking for manual inspection of the child class implementations of the abstract method. In case of parse or tokenizer error, the sniff will bow out and stay silent.
jrfnl
force-pushed
the
hotfix/abstract-filter-callback
branch
from
14c9d9b to
4356274
Compare
rebeccahum
previously requested changes
Sep 11, 2020
Co-authored-by: Rebecca Hum <[email protected]>
GaryJones
approved these changes
Sep 13, 2020
rebeccahum
approved these changes
Sep 14, 2020
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.
If a filter is defined in the constructor of an abstract class, and uses an abstract method as its callback, relying on implementing classes to define the return value, the
WordPressVIPMinimum.Hooks.AlwaysReturnInFilter.MissingReturnStatementsniff causes phpcs to crash.This PR adds a unit test to confirm the issue, and implements a fix using the same code that is used by phpcs itself.
Fixes #580