Skip to content

Namespaces/ReservedNames: various improvements#1729

Merged
wimg merged 5 commits intodevelopfrom
feature/namespaces-reservednames-expand-the-list
Aug 31, 2024
Merged

Namespaces/ReservedNames: various improvements#1729
wimg merged 5 commits intodevelopfrom
feature/namespaces-reservednames-expand-the-list

Conversation

@jrfnl
Copy link
Copy Markdown
Member

@jrfnl jrfnl commented Jul 25, 2024

Namespaces/ReservedNames: add extra tests

  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.

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:

  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:

Related to #1299

Namespaces/ReservedNames: add documentation

To view:

phpcs --generator=Text --standard=PHPCompatibility --sniffs=PHPCompatibility.Namespaces.ReservedNames

jrfnl added 5 commits August 11, 2024 04:49
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
```
@jrfnl jrfnl force-pushed the feature/namespaces-reservednames-expand-the-list branch from 3863b78 to 5c97a57 Compare August 11, 2024 02:50
@jrfnl
Copy link
Copy Markdown
Member Author

jrfnl commented Aug 11, 2024

Forced pushed to fix up some comments in the test case file in the fourth commit.

Copy link
Copy Markdown
Member

@wimg wimg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current implementation is the correct way to go, including level 5

@wimg wimg merged commit 34b142a into develop Aug 31, 2024
@wimg wimg deleted the feature/namespaces-reservednames-expand-the-list branch August 31, 2024 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants