Namespaces/ReservedNames: various improvements#1729
Merged
Conversation
This was referenced Jul 25, 2024
1. Add tests to safeguard that the sniff handles namespace names case-INsensitively. 2. Add tests to safeguard that the sniff does not throw false positives for the `namespace` keyword being used as an operator or when it is used as part of a namespaced name.
The "Namespaces in bundled PHP extensions" policy RFC which was voted on during the PHP 8.1 dev cycle, opened the door to more bundled PHP extensions using their own dedicated top-level namespace. These namespaces being reserved can cause clashes with userland code, as well as unexpected behaviour (the PHP native class being used instead of the expected userland class), so we should flag them. This updates the list of reserved namespace names in use by PHP based on the currently available PHP documentation. Most of these were added as part of the "resources to object" migration to hold the opaque object to replace the resource. Includes tests.
Also includes minor, similar fix to the documentation.
…sions
PECL extensions are PHP extensions which are not bundled with PHP releases.
Extensions are often initially developed on as PECL extensions, but if they are widely adopted, stable and maintained they can be moved to PHP src.
Along the same lines, unmaintained extensions may be unbundled from PHP and moved to PECL.
Userland packages using a top-level namespace in use by a PECL extension could run into trouble as:
1. The server the software runs on may have the extension enabled.
This can lead to nasty bugs and unexpected runtime behaviour as PHP will favour the code in the extension over any userland code, so if there is a same-named class, function, constant, the autoloader for the userland package will not kick in and the extension version will be used instead.
Additionally, if the code is loaded without an autoloader, it can lead to fatal "class already declared" errors and such.
2. PECL extensions may at some point be moved to and bundled with PHP itself, at which point, the problem outlined in [1] becomes even more widespread.
With this in mind and with the "Namespaces in bundled extensions" policy RFC having been accepted during the PHP 8.1 dev cycle and actively encouraging use of namespaces in PHP extensions, I propose we also start flagging top-level namespace names in use by PECL extensions.
Though, as it is not a given that this use will be problematic, I propose flagging these only with a "warning".
The error code for the warning is modular and will include the name flagged, allowing for selective exclusion of the warnings for just one particular namespace name.
The error code suffix - `PECLReserved` - is also different from the error code suffix for other errors/warnings thrown by this sniff (`Found`). This is deliberate to ensure that if a PECL extension becomes a bundled extension and a user of PHPCompatibility would have excluded the warning, they will still get to see the _error_ once the extension has been moved to PHP.
Includes unit tests.
Note: this commit will be easier to review while ignoring whitespace.
Open questions:
1. Should we check flag namespaces used by both the PECL extensions listed in the PHP manual as well as the RFC ? Or only those listed in the manual ?
2. Should the warning be thrown at severity 5 (current implementation) ? Or should we lower the severity for these warnings to, for instance, 3 ?
Lowering the severity would mean that people would not see the warning by default and would need to explicitly set `--severity=3` to see them, making the warning less effective.
For now, I've implemented the feature with the normal severity of `5`, but with modular error codes, which means that selectively ignoring a particular warning from a ruleset or via an inline ignore annotation is straight-forward.
Ref:
* https://wiki.php.net/rfc/namespaces_in_bundled_extensions
Related to 1299
To view: ```bash phpcs --generator=text --standard=phpcompatibility --sniffs=phpcompatibility.namespaces.reservednames ```
3863b78 to
5c97a57
Compare
Member
Author
|
Forced pushed to fix up some comments in the test case file in the fourth commit. |
58 tasks
wimg
approved these changes
Aug 31, 2024
Member
wimg
left a comment
There was a problem hiding this comment.
I think the current implementation is the correct way to go, including level 5
This file contains hidden or 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
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.
Namespaces/ReservedNames: add extra tests
namespacekeyword being used as an operator or when it is used as part of a namespaced name.Namespaces/ReservedNames: update the list of reserved names
The "Namespaces in bundled PHP extensions" policy RFC which was voted on during the PHP 8.1 dev cycle, opened the door to more bundled PHP extensions using their own dedicated top-level namespace.
These namespaces being reserved can cause clashes with userland code, as well as unexpected behaviour (the PHP native class being used instead of the expected userland class), so we should flag them.
This updates the list of reserved namespace names in use by PHP based on the currently available PHP documentation.
Most of these were added as part of the "resources to object" migration to hold the opaque object to replace the resource.
Includes tests.
Related to #1299
Namespaces/ReservedNames: fix grammar in error message
Also includes minor, similar fix to the documentation.
Namespaces/ReservedNames: also flag namespaces reserved by PECL extensions
PECL extensions are PHP extensions which are not bundled with PHP releases.
Extensions are often initially developed on as PECL extensions, but if they are widely adopted, stable and maintained they can be moved to PHP src.
Along the same lines, unmaintained extensions may be unbundled from PHP and moved to PECL.
Userland packages using a top-level namespace in use by a PECL extension could run into trouble as:
This can lead to nasty bugs and unexpected runtime behaviour as PHP will favour the code in the extension over any userland code, so if there is a same-named class, function, constant, the autoloader for the userland package will not kick in and the extension version will be used instead.
Additionally, if the code is loaded without an autoloader, it can lead to fatal "class already declared" errors and such.
With this in mind and with the "Namespaces in bundled extensions" policy RFC having been accepted during the PHP 8.1 dev cycle and actively encouraging use of namespaces in PHP extensions, I propose we also start flagging top-level namespace names in use by PECL extensions.
Though, as it is not a given that this use will be problematic, I propose flagging these only with a "warning".
The error code for the warning is modular and will include the name flagged, allowing for selective exclusion of the warnings for just one particular namespace name.
The error code suffix -
PECLReserved- is also different from the error code suffix for other errors/warnings thrown by this sniff (Found). This is deliberate to ensure that if a PECL extension becomes a bundled extension and a user of PHPCompatibility would have excluded the warning, they will still get to see the error once the extension has been moved to PHP.Includes unit tests.
Note: this commit will be easier to review while ignoring whitespace.
👉 Open questions:
Lowering the severity would mean that people would not see the warning by default and would need to explicitly set
--severity=3to see them, making the warning less effective.For now, I've implemented the feature with the normal severity of
5, but with modular error codes, which means that selectively ignoring a particular warning from a ruleset or via an inline ignore annotation is straight-forward.Ref:
Related to #1299
Namespaces/ReservedNames: add documentation
To view: