-
-
Notifications
You must be signed in to change notification settings - Fork 518
AbstractFunctionRestrictions sniffs: add tests for namespaced names #2581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
AbstractFunctionRestrictions sniffs: add tests for namespaced names #2581
Conversation
The DiscouragedFunctionsUnitTest.inc tests are currently used to test the `AbstractFunctionRestrictions` class and mostly covered everything that was necessary. Except fully qualified call to a non-global function. I opted to add the test in the middle of the file and remove a blank line to keep the namespaced calls tests together.
Warnings and errors in this test case file are declared in a way that is different from the other test case files. It seems to me that it is better not to add a few tests safeguarding the way that namespaced names are handled to this file, instead they should be added to a new file. Thus, it is necessary to rename this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some feedback - not necessarily blocking:
- The "header" comment used
Safeguard support for PHP 8.0+ tokenization of namespaced "names".
is confusing.
These tests are not directly related to PHP 8.0, as on PHPCS 3.x, the "old" tokenization is also used for PHP 8.0 - current, while on PHPCS 4.x, the "PHP 8.0 tokenization" is also used for PHP 7.2 - 7.4.
In my opinion, it would be clearer to call this "Safeguard correct handling of [all types of] namespaced function calls" or "Safeguard against false positives on namespaced function calls". - All the new tests per sniff use the "same" function name/code. When a sniff is looking for multiple functions, it's never a bad thing to have some variation in the tests.
Example:DateTime/RestrictedFunctions
- the namespaced name tests all use thedate_default_timezone_set
function name, while the sniff also looks fordate()
. - For the
\target_function()
test (FQN call to global function), if the test file in general has enough tests, like theDB/RestrictedFunctions
sniff, it would have been perfectly fine to update a few of the pre-existing tests instead of adding a new test for the FQN function call. - A lot of the test files do not contain any tests with non-lowercase function calls to the target functions. Similar to the previous point - if the test file contains sufficient tests, updating a few of these to be non-lowercase (mixed case, uppercase) would not go amiss.
- A lot of the test files also miss tests against false positives for "same name" method calls/constants/properties etc. While not strictly necessary for PHPCS 4.0, still may not be a bad idea to have these tests.
- The
// The sniff should start flagging this once it can resolve relative namespaces.
comment seen in quite some of the test files is only partially correct.
namespace\target_function()
will start to be flagged when in the global namespace, but not when within a namespace.
When namespace and import use tracking is added, we need to make sure both situations are tested (but that's for later, not now).
@@ -41,3 +41,11 @@ $handle = popen( '/bin/ls', 'r' ); | |||
// Issue #898. | |||
class System {} | |||
class Serialize {} | |||
|
|||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could these tests have re-used the currently empty block of lines at the top of the file ?
\curl_init(); | ||
MyNamespace\curl_init(); | ||
\MyNamespace\curl_init(); | ||
namespace\curl_init(); // The sniff should start flagging this once it can resolve relative namespaces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests need to be expanded a lot more.
This sniff also looks at the parameters for a number of target functions and for those check for certain (global) constants and/or function calls in the parameters, so tests are needed to cover those being fully qualified as well.
In preparation for PHPCS 4.0, which changes the tokenization of namespaced names, this PR adds tests with all variants of namespace names (unqualified, partially qualified, fully qualified, and namespace relative) to the sniffs that extend the
AbstractFunctionRestrictions
class when necessary (some commits already contained part of the required tests).The only sniff that is missing in this PR is
WordPress.Security.EscapeOutput
, which will be addressed separately.Before adding the tests for the
WordPress.WP.DeprecatedFunctions
sniff, I opted to rename the test case file, as it works differently from the other test case files, and adding the namespaced names to it seemed incorrect.Note: there will be more sniffs which will need additional tests, so this is only one of the first of a range of PRs adding more tests to help get ready for PHPCS 4.0.