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 | Runner::processChildProcs(): fix passing null to non-nullable bug #3425

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Sep 5, 2021

The DummyFile::__construct() method expects a string as the first parameter, subsequently passes it to the File::setContent() method, which also expects a string, which then passes it to the Common::detectLineEndings() method, which again expects a string.

Within the Common::detectLineEndings() method, the string is then passed to the PHP native preg_match() function, which (again) expects a string for the $subject parameter.

When using PHPCS with parallel processing turned on, on a system which allows for parallel processing, the DummyFile class was, however, being instantiated in the Runner::processChildProcs() method with null as the content of the first parameter, which on PHP 8.1 leads to preg_match(): Passing null to parameter #2 ($subject) of type string is deprecated deprecation notices.

This deprecation notice was then caught as a RuntimeException in the File::setContent() method and passed on to the File::addWarningOnLine(), which called the File::addMessage() method.

The File::addMessage() parameter then ran into trouble as the $this->config property has not been set up yet, as DummyFile::__construct() calls File::setContent() before calling the parent::__construct() method, leading to the Undefined array key "cache" notices which were making the build fail.

Fixed now by passing an empty string instead of null as the $content for the DummyFile in the Runner::processChildProcs() method.

This then leaves one more issue: the DummyFile::__construct() method contains a conditional code block which was only run when $content !== null. As this conditional code block is also not necessary to be run when an empty string would be passed to the constructor, changing this condition to $content !== '' makes that the condition can still match and maintains the efficiency tweak the condition was safeguarding.


This fixes the last, currently known, PHP 8.1 runtime issue. This does not make PHPCS fully compatible with PHP 8.1 yet as there are syntax changes in PHP 8.1 for which the Tokenizer will need adjusting, as well as sniff adjustments still needed, but at least the runtime issues should be out of the way now.

…le bug

The `DummyFile::__construct()` method expects a `string` as the first parameter, subsequently passes it to the `File::setContent()` method, which also expects a `string`, which then passes it to the `Common::detectLineEndings()` method, which again expects a `string`.

Within the `Common::detectLineEndings()` method, the string is then passed to the PHP native `preg_match()` function, which (again) expects as `string` for the `$subject` parameter.

When using PHPCS with parallel processing turned on, on a system which allows for parallel processing, the `DummyFile` class was, however, being instantiated in the `Runner::processChildProcs()` method with `null` as the content of the first parameter, which on PHP 8.1 leads to `preg_match(): Passing null to parameter squizlabs#2 ($subject) of type string is deprecated` deprecation notices.

This deprecation notice was then caught as a `RuntimeException` in the `File::setContent()` method and passed on to the `File::addWarningOnLine()`, which called the `File::addMessage()` method.

The `File::addMessage()` parameter then ran into trouble as the `$this->config` property has not been set up yet, as `DummyFile::__construct()` calls `File::setContent()` before calling the `parent::__construct()` method, leading to the `Undefined array key "cache"` notices which were making the build fail.

Fixed now by passing an empty string instead of `null` as the `$content` for the `DummyFile` in the `Runner::processChildProcs()` method.

This then leaves one more issue: the `DummyFile::__construct()` method contains a conditional code block which was only run when `$content !== null`. As this conditional code block is also not necessary to be run when an empty string would be passed to the constructor, changing this condition to `$content !== ''` makes that the condition can still match and maintains the efficiency tweak the condition was safeguarding.
@jrfnl
Copy link
Contributor Author

jrfnl commented Sep 5, 2021

@gsherwood Could this PR please be earmarked still for PHPCS 3.6.1 ? Thanks.

@gsherwood gsherwood added this to Idea Bank in PHPCS v3 Development via automation Sep 27, 2021
@gsherwood gsherwood added this to the 3.6.1 milestone Sep 27, 2021
gsherwood added a commit that referenced this pull request Sep 27, 2021
@gsherwood gsherwood merged commit 7eef328 into squizlabs:master Sep 27, 2021
PHPCS v3 Development automation moved this from Idea Bank to Ready for Release Sep 27, 2021
@gsherwood
Copy link
Member

That was incredibly well explained. Thanks a lot for tracking that down and fixing it.

@jrfnl jrfnl deleted the feature/php-8.1-fix-parallel-running-null-to-non-nullable branch September 27, 2021 04:49
@jrfnl
Copy link
Contributor Author

jrfnl commented Sep 27, 2021

Thanks. Took some tracking down and processing to grep it myself, so figured it would be helpful to document in detail what was happening here.

@web-programmer-here
Copy link

web-programmer-here commented Feb 15, 2023

@jrfnl I still get this error

PHP Deprecated:  preg_match(): Passing null to parameter #2 ($subject) 
of type string is deprecated in ../squizlabs/php_codesniffer/src/Config.php on line 228

Deprecated: preg_match(): Passing null to parameter #2 ($subject) 
of type string is deprecated in ../squizlabs/php_codesniffer/src/Config.php on line 228
....... 7 / 7 (100%)

I am using "squizlabs/php_codesniffer": "^3.7.1", with PHP 8.2.2
Is this issue related to this?

Setting that line to something like this should get rid of that error

&& (isset($matches) && preg_match('|\d+ (\d+)|', shell_exec('stty size 2>&1'), $matches) === 1)

@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 15, 2023

@web-programmer-here That looks like an unrelated issue.

N.B.: Your "fix" is not a fix, but a bug in the making. PHP is complaining about the output of the shell_exec() function call being null, not about $matches.

When you write up the issue properly, please mention your OS as well as it's likely to do with the stty command not being available on your OS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
PHPCS v3 Development
Ready for Release
Development

Successfully merging this pull request may close these issues.

None yet

3 participants