PHP 7.2: RemovedGlobalVariables: add detection for use of $php_errormsg #528
Conversation
This file contains 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
> `track_errors` ini setting and `$php_errormsg` variable > > When the `track_errors` ini setting is enabled, a `$php_errormsg` variable is created in the local scope when a non-fatal error occurs. Given that the preferred way of retrieving such error information is by using `error_get_last()`, this feature has been deprecated. Refs: * http://php.net/manual/en/migration72.deprecated.php#migration72.deprecated.track_errors-and-php_errormsg * https://wiki.php.net/rfc/deprecations_php_7_2#create_function * http://php.net/manual/en/reserved.variables.phperrormsg.php **Implementation notes:** As the `$php_errormsg` variable is not a superglobal, quite some extra checks are needed. All the same, adding it to the `RemovedGlobalVariables` sniff seemed like the most logical place for this variable deprecation. I could, however, split it off into a separate sniff if so preferred. **Sniff notes:** The sniff does not take the value of the `track_errors` ini setting into account as the scan is often run on a different system than the one on which the software is deployed, so taking it into account would make the sniff unreliable. The sniff _does_ check for a variable of the same name being assigned a value within the same scope as the `$php_errormsg` variable is used. This should prevent most false positives. Based on the current tests, there is one false positive at this moment and that is when a closure imports the `$php_errormsg` variable by reference and the `$php_errormsg` variable has been assigned a value within the scope from which the closure is importing it. This is such a specific case and would require quite some extra code to check for, that I deemed this unnecessary as the chances of this throwing a false positive in real life code are minuscule. If this false positive is at some point reported, the additional code could still be added.
wimg
approved these changes
Nov 22, 2017
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.
Refs:
Implementation notes:
As the
$php_errormsgvariable is not a superglobal, quite some extra checks are needed.All the same, adding it to the
RemovedGlobalVariablessniff seemed like the most logical place for this variable deprecation.I could, however, split it off into a separate sniff if so preferred.
Sniff notes:
The sniff does not take the value of the
track_errorsini setting into account as the scan is often run on a different system than the one on which the software is deployed, so taking it into account would make the sniff unreliable.The sniff does check for a variable of the same name being assigned a value within the same scope as the
$php_errormsgvariable is used. This should prevent most false positives.Based on the current tests, there is one false positive at this moment and that is when a closure imports the
$php_errormsgvariable by reference and the$php_errormsgvariable has been assigned a value within the scope from which the closure is importing it.This is such a specific case and would require quite some extra code to check for, that I deemed this unnecessary as the chances of this throwing a false positive in real life code are minuscule.
If this false positive is at some point reported, the additional code could still be added.