Skip to content

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Feb 16, 2023

As per issue #3760, if the stty size command errors out, shell_exec() will return null. Per the documentation, the shell_exec() command can also return false, which would also be an unusable value (preg_match() will not match, so 'auto' would be cast to int, resulting in 0 being set as the value).

If the output of shell_exec() would be null, PHP as of PHP 8.1 will throw a "passing null to non-nullable" notice.

Additionally, if the output from shell_exec() would be false, the original input would be an invalid non-numeric value or the original input would be auto, but the shell_exec() command is disabled, the code as-it-was, would set the reportWidth to 0, which is not that helpful. See: https://3v4l.org/GE7sK

I've now changed the logic to only update the reportWidth property value when the received value is auto AND a valid CLI width could be determined. This solves the originally reported issue + two of the above mentioned additional issues. The only one not solved is "_the original input would be an invalid non-numeric value _", but in that case, it's clear user-error and not something which PHPCS needs to handle.

I've now changed the logic to:

  • If the received value is auto, check if a valid CLI width can be determined and only then use that value.
  • In all other cases, including when the value is set directly on the object, the value will be validated and if not valid, a default value of width 80 will be used, which is in line with the default width as stated in the documentation.

Fixes #3760

@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 16, 2023

Note: the build failure is unrelated to this PR and will be fixed once #3737 has been merged.

@gsherwood gsherwood added this to the 3.8.0 milestone Feb 22, 2023
@jrfnl jrfnl force-pushed the feature/3760-php-8.1-config-passing-null-to-non-nullable branch from 3a98c12 to 91179a4 Compare March 2, 2023 04:11
@jrfnl jrfnl force-pushed the feature/3760-php-8.1-config-passing-null-to-non-nullable branch from 91179a4 to 91021e1 Compare May 5, 2023 11:03
@jrfnl
Copy link
Contributor Author

jrfnl commented May 5, 2023

I've updated this PR to include a default value of 80, which is in line with the documentation about report width in the wiki.

I've also updated the commit message and the PR description to be in line with this additional change.

Compared to the original PR, the main difference is that the "the original input would be an invalid non-numeric value" case is now also handled and will silently ignore the invalid value and fall back to the default width of 80.

Suggested changelog entry

The "reportWidth" will now consistently fall back to a width of 80 when either an invalid report width was passed or the width of the terminal could not be determined when report width was set to 'auto' (default).
This will also prevent a potential "passing null to non-nullable" deprecation notice on PHP 8.1+.

@jrfnl jrfnl force-pushed the feature/3760-php-8.1-config-passing-null-to-non-nullable branch from 91021e1 to 23a3e15 Compare May 5, 2023 11:23
As per issue 3760, if the `stty size` command errors out, `shell_exec()` will return `null`.
Per the [documentation](https://www.php.net/manual/en/function.shell-exec.php#refsect1-function.shell-exec-returnvalues), the `shell_exec()` command can also return `false`, which would also be an unusable value (`preg_match()` will not match, so `'auto'` would be cast to int, resulting in `0` being set as the value).

If the output of `shell_exec()` would be `null`, PHP as of PHP 8.1 will throw a "passing null to non-nullable" notice.

Additionally, if the output from `shell_exec()` would be `false`, the original input would be an invalid non-numeric value or the original input would be `auto`, but the `shell_exec()` command is disabled, the code as-it-was, would set the `reportWidth` to `0`, which is not that helpful. See: https://3v4l.org/GE7sK

I've now changed the logic to:
* If the received value is `auto`, check if a valid CLI width can be determined and only then use that value.
* In all other cases, including when the value is set directly on the object, the value will be validated and if not valid, a default value of width `80` will be used, which is in line with the default width as stated in [the documentation](https://github.com/squizlabs/PHP_CodeSniffer/wiki/Configuration-Options#setting-the-default-report-width).

Fixes 3760
@jrfnl jrfnl force-pushed the feature/3760-php-8.1-config-passing-null-to-non-nullable branch from 23a3e15 to 8bdbd56 Compare May 5, 2023 11:28
gsherwood added a commit that referenced this pull request May 7, 2023
@gsherwood gsherwood merged commit 35fc9b9 into squizlabs:master May 7, 2023
@jrfnl jrfnl deleted the feature/3760-php-8.1-config-passing-null-to-non-nullable branch May 7, 2023 04:51
@jrfnl
Copy link
Contributor Author

jrfnl commented Dec 8, 2023

FYI: this fix is included in today's PHP_CodeSniffer 3.8.0 release.

As per #3932, development on PHP_CodeSniffer will continue in the PHPCSStandards/PHP_CodeSniffer repository. If you want to stay informed, you may want to start "watching" that repo (or watching releases from that repo).

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.

PHP Deprecated: preg_match(): Passing null to parameter #2

3 participants