Fix checks for PHP 8.1 and above #746
Merged
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
Make it clear which ruleset being tested, even on a test failure.
PHP 8.1 increased the strictness of types used with native functions, and this meant WPCS 2.3.0 had a bug: WordPress/WordPress-Coding-Standards#2035 For the VIPCS ruleset tests with PHP 8.1 or above it would skip over a bunch of lines, and mark other lines as expecting errors or warnings incorrectly. Running: ``` vendor/squizlabs/php_codesniffer/bin/phpcs --standard=WordPressVIPMinimum --severity=1 WordPressVIPMinimum/ruleset-test.inc -s ``` ...shows all the expected violations, but also this for line 1: > An error occurred during processing; checking has been aborted. The error message was: trim(): Passing null to parameter #1 ($string) of type string is deprecated in .../vipcs/vendor/wp-coding-standards/wpcs/WordPress/Sniff.php on line 1144 (Internal.Exception) Line 1144 relate to the retrieval of the `minimum_supported_wp_version` config value. Since it appeared to be coming through as null, the RulesetTest.php now includes setting this as a command-line argument when PHPCS is called to run the ruleset test. When WPCS 3.0 is the minimum supported, these changes can be reverted.
PHP 8.1 increased the strictness of types used with native functions, and this meant WPCS 2.3.0 had a bug: WordPress/WordPress-Coding-Standards#2035 For the VIPCS coding standards check with PHP 8.1 or above it would report incorrect violations from sniffs that included the retrieval of optional command-line arguments. These violations did not appear when running PHPCS with PHP 8.0 or below. Since the retrieval of the optional command-line arguments appeared to be coming through as null, causing the problems, the command to run PHPCS on the VIPCS codebase now includes setting of several command-line arguments. With the `prefixes` enabled, extra sniff behaviour is enabled, and VIPCS has no need to adhere to consistent prefixes (unlike actual WordPress plugin and theme codebases). As such, the PrefixAllGlobals rule is excluded for VIPCS. When WPCS 3.0 is the minimum supported, these changes can be reverted.
When running the unit tests with PHP 8.1 or above, PHPUnit would give a deprecation notice: > PHP Deprecated: strlen(): Passing null to parameter #1 ($string) of type string is deprecated in .../vipcs/vendor/phpunit/phpunit/src/Util/Getopt.php on line 159 The tests still ran successfully though. This notice originated from the use of `--filter WordPressVIPMinimum`, and was fixed by adding an equals sign: `--filter=WordPressVIPMinimum`. Under both versions, the same number of tests and test files and unique error codes were created.
rebeccahum
requested changes
Jan 31, 2023
rebeccahum
approved these changes
Jan 31, 2023
jrfnl
reviewed
Mar 7, 2023
Comment on lines
-20
to
+22
| "$(pwd)/vendor/bin/phpunit" --filter WordPressVIPMinimum "$(pwd)/vendor/squizlabs/php_codesniffer/tests/AllTests.php" --no-coverage --no-configuration --bootstrap=./tests/bootstrap.php --dont-report-useless-tests $@ | ||
| "$(pwd)/vendor/bin/phpunit" --filter=WordPressVIPMinimum "$(pwd)/vendor/squizlabs/php_codesniffer/tests/AllTests.php" --no-coverage --no-configuration --bootstrap=./tests/bootstrap.php --dont-report-useless-tests $@ | ||
| else | ||
| "$(pwd)/vendor/bin/phpunit" --filter WordPressVIPMinimum "$(pwd)/vendor/squizlabs/php_codesniffer/tests/AllTests.php" --no-coverage $@ | ||
| "$(pwd)/vendor/bin/phpunit" --filter=WordPressVIPMinimum "$(pwd)/vendor/squizlabs/php_codesniffer/tests/AllTests.php" --no-coverage $@ |
There was a problem hiding this comment.
This one surprises me as WPCS itself does not appear to be afflicted by the same issue, which makes me think something else is going on....
There was a problem hiding this comment.
It does for me. Still the same fatal happening, but the deprecation disappears when adding in the =:
There was a problem hiding this comment.
What version of PHPUnit are you running the tests on ?
There was a problem hiding this comment.
Also, IIRC, for PHP 8.1+, this is solved via the --no-configuration bit, so if that's not the case for you, I suspect there is something else going on with your setup.
There was a problem hiding this comment.
What version of PHPUnit are you running the tests on ?
7.5.20.
There was a problem hiding this comment.
And the --no-configuration gets applied correctly ?
There was a problem hiding this comment.
Changing the Composer script to @php ./vendor/phpunit/phpunit/phpunit --filter WordPress ./vendor/squizlabs/php_codesniffer/tests/AllTests.php --no-configuration and then running it produces even more errors.
Details
PHP Warning: Undefined global variable $PHP_CODESNIFFER_PEAR in /Users/gary/code/wpcs/vendor/squizlabs/php_codesniffer/tests/AllTests.php on line 12
Warning: Undefined global variable $PHP_CODESNIFFER_PEAR in /Users/gary/code/wpcs/vendor/squizlabs/php_codesniffer/tests/AllTests.php on line 12
PHP Warning: include_once(CodeSniffer/Core/AllTests.php): Failed to open stream: No such file or directory in /Users/gary/code/wpcs/vendor/squizlabs/php_codesniffer/tests/AllTests.php on line 16
Warning: include_once(CodeSniffer/Core/AllTests.php): Failed to open stream: No such file or directory in /Users/gary/code/wpcs/vendor/squizlabs/php_codesniffer/tests/AllTests.php on line 16
PHP Warning: include_once(): Failed opening 'CodeSniffer/Core/AllTests.php' for inclusion (include_path='.:/usr/local/Cellar/php@8.1/8.1.13/share/php@8.1/pear') in /Users/gary/code/wpcs/vendor/squizlabs/php_codesniffer/tests/AllTests.php on line 16
Warning: include_once(): Failed opening 'CodeSniffer/Core/AllTests.php' for inclusion (include_path='.:/usr/local/Cellar/php@8.1/8.1.13/share/php@8.1/pear') in /Users/gary/code/wpcs/vendor/squizlabs/php_codesniffer/tests/AllTests.php on line 16
PHP Warning: include_once(CodeSniffer/Standards/AllSniffs.php): Failed to open stream: No such file or directory in /Users/gary/code/wpcs/vendor/squizlabs/php_codesniffer/tests/AllTests.php on line 17
Warning: include_once(CodeSniffer/Standards/AllSniffs.php): Failed to open stream: No such file or directory in /Users/gary/code/wpcs/vendor/squizlabs/php_codesniffer/tests/AllTests.php on line 17
PHP Warning: include_once(): Failed opening 'CodeSniffer/Standards/AllSniffs.php' for inclusion (include_path='.:/usr/local/Cellar/php@8.1/8.1.13/share/php@8.1/pear') in /Users/gary/code/wpcs/vendor/squizlabs/php_codesniffer/tests/AllTests.php on line 17
Warning: include_once(): Failed opening 'CodeSniffer/Standards/AllSniffs.php' for inclusion (include_path='.:/usr/local/Cellar/php@8.1/8.1.13/share/php@8.1/pear') in /Users/gary/code/wpcs/vendor/squizlabs/php_codesniffer/tests/AllTests.php on line 17
PHP Fatal error: Uncaught Error: Class "PHP_CodeSniffer\Tests\Core\AllTests" not found in /Users/gary/code/wpcs/vendor/squizlabs/php_codesniffer/tests/AllTests.php:56
Stack trace:
#0 [internal function]: PHP_CodeSniffer\Tests\PHP_CodeSniffer_AllTests::suite('PHP_CodeSniffer...')
#1 /Users/gary/code/wpcs/vendor/phpunit/phpunit/src/Runner/BaseTestRunner.php(125): ReflectionMethod->invoke(NULL, 'PHP_CodeSniffer...')
#2 /Users/gary/code/wpcs/vendor/phpunit/phpunit/src/TextUI/Command.php(183): PHPUnit\Runner\BaseTestRunner->getTest('./vendor/squizl...', '/Users/gary/cod...', Array)
#3 /Users/gary/code/wpcs/vendor/phpunit/phpunit/src/TextUI/Command.php(162): PHPUnit\TextUI\Command->run(Array, true)
#4 /Users/gary/code/wpcs/vendor/phpunit/phpunit/phpunit(61): PHPUnit\TextUI\Command::main()
#5 {main}
thrown in /Users/gary/code/wpcs/vendor/squizlabs/php_codesniffer/tests/AllTests.php on line 56
Fatal error: Uncaught Error: Class "PHP_CodeSniffer\Tests\Core\AllTests" not found in /Users/gary/code/wpcs/vendor/squizlabs/php_codesniffer/tests/AllTests.php:56
Stack trace:
#0 [internal function]: PHP_CodeSniffer\Tests\PHP_CodeSniffer_AllTests::suite('PHP_CodeSniffer...')
#1 /Users/gary/code/wpcs/vendor/phpunit/phpunit/src/Runner/BaseTestRunner.php(125): ReflectionMethod->invoke(NULL, 'PHP_CodeSniffer...')
#2 /Users/gary/code/wpcs/vendor/phpunit/phpunit/src/TextUI/Command.php(183): PHPUnit\Runner\BaseTestRunner->getTest('./vendor/squizl...', '/Users/gary/cod...', Array)
#3 /Users/gary/code/wpcs/vendor/phpunit/phpunit/src/TextUI/Command.php(162): PHPUnit\TextUI\Command->run(Array, true)
#4 /Users/gary/code/wpcs/vendor/phpunit/phpunit/phpunit(61): PHPUnit\TextUI\Command::main()
#5 {main}
thrown in /Users/gary/code/wpcs/vendor/squizlabs/php_codesniffer/tests/AllTests.php on line 56
Script @php ./vendor/phpunit/phpunit/phpunit --filter WordPress ./vendor/squizlabs/php_codesniffer/tests/AllTests.php --no-configuration handling the run-tests event returned with error code 255
There was a problem hiding this comment.
Sounds like this would be better debugged via a videocall with screenshare at some point... ? Feel free to ping me to set it up.
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.
Running
composer test(orcomposer checkafter #741 is merged) would lead to several issues under PHP 8.1 and 8.2. This PR fixes those issues.Ruleset tests: Add label before test runs
Make it clear which ruleset is being tested, even on a test failure. This just makes it easier to debug.
Ruleset Tests: Workaround WPCS trim() bug
PHP 8.1 increased the strictness of types used with native functions, and this meant WPCS 2.3.0 had a bug: WordPress/WordPress-Coding-Standards#2035
For the VIPCS ruleset tests with PHP 8.1 or above, it would skip over a bunch of lines in the test file and report other lines as expecting errors or warnings incorrectly.
Running:
...shows all the expected violations, but also this for line 1:
Line 1144 relates to the retrieval of the
minimum_supported_wp_versionconfig value. Since it appeared to be coming through asnull, theRulesetTest.phpnow includes setting a trivial value via a command-line argument when PHPCS is called to run the ruleset test.When WPCS 3.0 is the minimum supported, this change can be reverted since the original bug would have been fixed.
CS: Work around WPCS trim() bug
PHP 8.1 increased the strictness of types used with native functions, and this meant WPCS 2.3.0 had a bug: WordPress/WordPress-Coding-Standards#2035
The VIPCS coding standards check with PHP 8.1 or above would report incorrect violations from sniffs that included the retrieval of optional command-line arguments. These violations did not appear when running PHPCS with PHP 8.0 or below.
Since the retrieval of the optional command-line arguments appeared to be coming through as null, causing the problems, the command to run PHPCS on the VIPCS codebase now includes the setting of several command-line arguments.
With the
prefixesenabled, extra sniff behaviour is enabled, and VIPCS does not need to adhere to consistent prefixes (unlike actual WordPress plugin and theme codebases). As such, the PrefixAllGlobals rule is excluded for VIPCS.When WPCS 3.0 is the minimum supported, these changes can be reverted since the original bug would have been fixed.
Unit tests: Fix PHPUnit deprecation
When running the unit tests with PHP 8.1 or above, PHPUnit would give a deprecation notice:
The tests still ran successfully, though.
This notice originated from the use of
--filter WordPressVIPMinimumand was fixed by adding an equals sign:--filter=WordPressVIPMinimum.Under both versions, the same number of tests and test files and unique error codes were created.