Skip to content

PHP 7.2: RemovedGlobalVariables: add detection for use of $php_errormsg#528

Merged
wimg merged 1 commit intomasterfrom
php-7.2/deprecated-php-errormsg
Nov 22, 2017
Merged

PHP 7.2: RemovedGlobalVariables: add detection for use of $php_errormsg#528
wimg merged 1 commit intomasterfrom
php-7.2/deprecated-php-errormsg

Conversation

@jrfnl
Copy link
Copy Markdown
Member

@jrfnl jrfnl commented Oct 28, 2017

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:

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.

> `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.
@jrfnl jrfnl requested a review from wimg October 28, 2017 06:58
@jrfnl jrfnl added this to the 8.1.0 milestone Nov 13, 2017
@wimg wimg merged commit f06fce3 into master Nov 22, 2017
@wimg wimg deleted the php-7.2/deprecated-php-errormsg branch November 22, 2017 22:54
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