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

Parallel running with multiple file directives leads to "passing null to non-nullable" notice on PHP 8.1 #3677

Open
Kleinast opened this issue Sep 26, 2022 · 13 comments

Comments

@Kleinast
Copy link

Kleinast commented Sep 26, 2022

Describe the bug
I get an error when run phpcs or phpcs-fix when I upgrade my project on php 8.1.
vendor/squizlabs/php_codesniffer/src/Files/LocalFile.php:32 it seem some path files are "null" in LocalFile->files and php 8.1

PHPCS output here

PHP Fatal error: Uncaught PHP_CodeSniffer\Exceptions\RuntimeException: trim(): Passing null to parameter #1 ($string) of type string is deprecated in /srv/vendor/squizlabs/php_codesniffer/src/Files/LocalFile.php on line 32 in /srv/vendor/squizlabs/php_codesniffer/src/Runner.php:604
Stack trace:
#0 [internal function]: PHP_CodeSniffer\Runner->handleErrors()
#1 /srv/vendor/squizlabs/php_codesniffer/src/Files/LocalFile.php(32): trim()
#2 /srv/vendor/squizlabs/php_codesniffer/src/Files/FileList.php(192): PHP_CodeSniffer\Files\LocalFile->__construct()
#3 /srv/vendor/squizlabs/php_codesniffer/src/Runner.php(484): PHP_CodeSniffer\Files\FileList->current()
#4 /srv/vendor/squizlabs/php_codesniffer/src/Runner.php(114): PHP_CodeSniffer\Runner->run()
#5 /srv/vendor/squizlabs/php_codesniffer/bin/phpcs(18): PHP_CodeSniffer\Runner->runPHPCS()
#6 /srv/vendor/bin/phpcs(120): include('...')
#7 {main}
thrown in /srv/vendor/squizlabs/php_codesniffer/src/Runner.php on line 604

Fatal error: Uncaught PHP_CodeSniffer\Exceptions\RuntimeException: trim(): Passing null to parameter #1 ($string) of type string is deprecated in /srv/vendor/squizlabs/php_codesniffer/src/Files/LocalFile.php on line 32 in /srv/vendor/squizlabs/php_codesniffer/src/Runner.php on line 604

Versions (please complete the following information):

  • PHP: 8.1
  • PHPCS: [3.7.1]

This small PR seem fix the problem #3676 (but as said in PR, not sure it is a total fix)

@jrfnl
Copy link
Contributor

jrfnl commented Sep 26, 2022

@Kleinast Could you please elaborate how you got this deprecation notice and how it can be reproduced ?

  • What type of setup are you using ? (Composer, PHAR, etc)
  • How are you running PHPCS ? (CLI, via IDE, etc)
  • What command are you using ?
  • Are you using a custom ruleset or one of the build-in rulesets ? And if so, which ?
  • Are you using any external standards ?

@Kleinast
Copy link
Author

Kleinast commented Sep 26, 2022

* What type of setup are you using ? (Composer, PHAR, etc)

We use composer

* How are you running PHPCS ? (CLI, via IDE, etc)

running from cli

* What command are you using ?
 php vendor/bin/phpcs
 php vendor/bin/php-cs-fixer fix --dry-run --diff
* Are you using a custom ruleset or one of the build-in rulesets ? And if so, which ?

we are using this package for ruleset https://github.com/Geolid/phpcs

* Are you using any external standards ?

Not sure what you mean by external standards

@jrfnl
Copy link
Contributor

jrfnl commented Sep 26, 2022

I'll have a look see, but may still need more information.

php vendor/bin/phpcs
php vendor/bin/php-cs-fixer fix --dry-run --diff

Just FYI: php-cs-fixer is a completely different tool. The fixer which is included with PHPCS can be called using phpcbf.

@jrfnl
Copy link
Contributor

jrfnl commented Sep 26, 2022

Tried to reproduce and failed.

  1. I created a new project.
  2. Ran composer require --dev geolid/phpcs
  3. Created a simple dummy file to run tests on.
  4. Copied the phpcs.xml.dist file to the project root and adjusted the file directive.
  5. Ran vendor/bin/phpcs -ps . and saw the expected CS errors and no errors related to the LocalFile class.

Ran with PHPCS develop and PHP 8.1.10.

In other words, please post an exact reproduction scenario as, as things are, the issue is not reproducible.

Not sure what you mean by external standards

The geolid/phpcs package is an external standard which provides a ruleset.

@jrfnl
Copy link
Contributor

jrfnl commented Sep 26, 2022

Note: I do see some weird stuff going on in that repo, but not sure if it is related and you should probably report all that to the maintainers of that package:

  • The ruleset.xml file contains a <config name="installed_paths" ... directive.
    This should generally not be used in an installable external standard and may be the cause of the problem.
  • That <config name="installed_paths"... directive "registers" paths to at least two other external standards which aren't being installed.
  • The composer.json file includes at least one external standard which includes the dealerdirect/phpcodesniffer-composer-installer Composer plugin, which will actually register the installed external standards correctly.

Based on this, I have a feeling this is an issue with the external standard you are using doing things wrong and not something which needs to be solved in PHPCS.

@Kleinast
Copy link
Author

thx a lot for your tests, i'll dig into the geolid/phpcs package or try to reproduce the error on tiny project

@Kleinast
Copy link
Author

I tryed to dig, and it seem it is linked to the parallel feature, I get no error on whe it is set to 1 and I get some trim() error when more than 1. I think the batches in parrallel may be create some $path = $todo->key(); = null
but I dont understand very much what exactly is the cause of that.

@Kleinast
Copy link
Author

I try different configs on my project and found what in my conf create the issue, geolid/phpcs is not the cause I tryed without it.
In my config:

<?xml version="1.0"?>
<ruleset name="PHP CS">
    <arg name="basepath" value="."/>
    <arg name="colors"/>
    <arg name="parallel" value="75"/>
    <arg name="extensions" value="php"/>
    <arg value="p"/>


    <file>src</file>
    <file>databases</file>
    <file>tests</file>
    <file>public/index.php</file>
    <file>databases/DoctrineMigrations</file>

    <rule ref="./vendor/geolid/phpcs/src/Geolid/ruleset.xml"/>
</ruleset>

The error trigger when parallel is more than 1 and I reference this files to check:

    <file>databases</file>
    <file>databases/DoctrineMigrations</file>

If I keep only one of this two file to check, I don't get the error anymore
So it seem that parrallel testing with config referencing common file créate some null path in FileList->current()

@saas786

This comment was marked as off-topic.

@jimmy-sandoval

This comment was marked as off-topic.

@jrfnl

This comment was marked as off-topic.

@jrfnl

This comment was marked as off-topic.

@jimmy-sandoval

This comment was marked as off-topic.

@jrfnl jrfnl changed the title Error on Php8.1 Parallel running with multiple file directives leads to "passing null to non-nullable" notice on PHP 8.1 Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants