Skip to content
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

✨ PHP 8.1: New PHPCompatibility.Variables.RemovedIndirectModificationOfGlobals sniff #1487

Merged

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Apr 5, 2023

Access to the $GLOBALS array is now subject to a number of restrictions.
Read and write access to individual array elements like $GLOBALS['var']
continues to work as-is. Read-only access to the entire $GLOBALS array also
continues to be supported. However, write access to the entire $GLOBALS
array is no longer supported. For example, array_pop($GLOBALS) will result
in an error.

Refs:

This commit introduces a new sniff which attempts to find all code patterns affected by this change, as per the code samples highlighted in the RFC.

  • There will no longer be a recursive “GLOBALS” key in the $GLOBALS variable.
  • Writes to $GLOBALS taken as a whole will now generate a compile error, this includes list assignments and unsetting of $GLOBALS.
  • Passing $GLOBALS by reference will trigger a runtime Error exception.
  • Appending unnamed entries to the $GLOBALS array is no longer supported.
  • etc

Notes:

  • There are two specific situations which constitute a behavioural change. I.e. the code will work in both PHP < 8.1 as well as PHP 8.1+, but the result of the code will be different. The sniff will flag those situations with a warning instead of an error and will only flag these when both PHP < 8.1 as well as PHP 8.1+ need to be supported (based on the provided testVersion).
  • There is one known false negative - when $GLOBALS with array access is used in a short list. Detecting whether a variable is used in a short list from within the list is hard and potentially very token walking intensive. As this is very, very much an edge case, which would be rare to come across in real code, I have deemed it unwise to pursue detection of this.
  • The extract($GLOBALS, EXTR_REFS); code sample, which is taken literally from the RFC, will not in actual fact throw a fatal error in PHP 8.1. As it is explicitly highlighted in the RFC as code that will break, it will be treated the same as all other code where $GLOBALS is passed by reference and will be flagged with an error.
  • For non-PHP native functions, it is not possible to the detect whether $GLOBALS is being passed by reference. As things are, these will not be flagged, though it could be considered to flag them with a lower severity to allow for manual inspection.
  • For calls to PHP native functions in combination with named parameters, it is only possible to detect pass by reference if the parameter name hasn't changed between PHP versions. Unfortunately, it has for the two function calls used in the tests. To get round that, we'd need a complete list of all PHP functions which receive parameters by reference, the parameter names across PHP versions and the parameter positions. In my estimation, that wouldn't actually add much value as for code written using PHP 8.0+ syntax, we should generally be able to expect that PHPCS will also be run on PHP 8.0+, in which case, it's a non-issue.

Includes unit tests.
Includes XML docs.

Related to #1299

…OfGlobals sniff

> Access to the $GLOBALS array is now subject to a number of restrictions.
> Read and write access to individual array elements like $GLOBALS['var']
> continues to work as-is. Read-only access to the entire $GLOBALS array also
> continues to be supported. However, write access to the entire $GLOBALS
> array is no longer supported. For example, array_pop($GLOBALS) will result
> in an error.

Refs:
* https://www.php.net/manual/en/migration81.incompatible.php#migration81.incompatible.core.globals-access
* https://github.com/php/php-src/blob/28a1a6be0873a109cb02ba32784bf046b87a02e4/UPGRADING#L23-L29
* https://wiki.php.net/rfc/restrict_globals_usage
* php/php-src#6487

This commit introduces a new sniff which attempts to find all code patterns affected by this change, as per the code samples highlighted in the RFC.

* There will no longer be a recursive “GLOBALS” key in the `$GLOBALS` variable.
* Writes to `$GLOBALS` taken as a whole will now generate a compile error, this includes list assignments and unsetting of `$GLOBALS`.
* Passing `$GLOBALS` by reference will trigger a runtime `Error` exception.
* Appending unnamed entries to the `$GLOBALS` array is no longer supported.
* etc

Notes:
* There are two specific situations which constitute a behavioural change. I.e. the code will work in both PHP < 8.1 as well as PHP 8.1+, but the result of the code will be different.
    The sniff will flag those situations with a `warning` instead of an `error` and will only flag these when both PHP < 8.1 as well as PHP 8.1+ need to be supported (based on the provided `testVersion`).
* There is one known false negative - when `$GLOBALS` with array access is used in a short list.
    Detecting whether a variable is used in a short list _from within the list_ is hard and potentially very token walking intensive.
    As this is very, very much an edge case, which would be rare to come across in real code, I have deemed it unwise to pursue detection of this.
* The `extract($GLOBALS, EXTR_REFS);` code sample, which is taken literally from the RFC, will not in actual fact throw a fatal error in PHP 8.1.
    As it is explicitly highlighted in the RFC as code that will break, it will be treated the same as all other code where `$GLOBALS` is passed by reference and will be flagged with an `error`.
* For non-PHP native functions, it is not possible to the detect whether `$GLOBALS` is being passed by reference.
    As things are, these will not be flagged, though it could be considered to flag them with a lower `severity` to allow for manual inspection.
* For calls to PHP native functions in combination with named parameters, it is only possible to detect pass by reference if the parameter name hasn't changed between PHP versions.
    Unfortunately, it has for the two function calls used in the tests.
    To get round that, we'd need a complete list of all PHP functions which receive parameters by reference, the parameter names across PHP versions and the parameter positions.
    In my estimation, that wouldn't actually add much value as for code written using PHP 8.0+ syntax, we should generally be able to expect that PHPCS will also be run on PHP 8.0+, in which case, it's a non-issue.

Includes unit tests.
Includes XML docs.
@wimg wimg merged commit cc58790 into develop Apr 20, 2023
41 checks passed
@wimg wimg deleted the php-8.1/new-removedindirectmodificationofglobals-sniff branch April 20, 2023 14:53
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.

None yet

2 participants