-
Notifications
You must be signed in to change notification settings - Fork 22
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: silence the deprecation notices about missing return types #64
PHP 8.1: silence the deprecation notices about missing return types #64
Conversation
@grogy It would be great if this PR could be merged and released quite quickly. PHP 8.1.0alpha1 got released two weeks ago, so I expect that over the next few weeks/months, more and more projects will start running their CI workflows against PHP 8.1 and if linting using PHP Parallel Lint is part of that CI workflow, the build against PHP 8.1 will fail due to this issue, as can be seen in the builds I've set up for PHP-Console-Highlighter and PHP-Console-Color. Considering there are over 6000 projects which have this repo declared as a dependency, getting a release, which includes this fix, out soon seems prudent. |
This commit: * Add a new dependency on the PHP Parallel Lint package for fast PHP linting. The PHP Parallel Lint package, in combination with the PHP Console Highlighter provides the following advantages in comparison with "plain" PHP linting: - Higher speed due to the parallel processes. - Improved usability by providing color coded syntax highlighting of found errors on the command-line. - Integration with the `cs2pr` tool, allowing for the results of the lint command to be shown in-line in PRs. * Adds a Composer `lint` script for easy access to the tool for devs, while making sure the correct command line parameters will be used. The linting command as currently set up, will also check the example files for linting errors. * Adds a GH Actions job for linting the code on the high/low supported PHP versions, one arbitrary interim version + an experimental build against PHP 8.1. The `cs2pr` tool has been enabled and will show the results of the non-experimental lint runs in-line in PRs. **Note**: For PHP 8.1, the `cs2pr` tool is not used as there is a known issue in the Parallel Lint tool with PHP 8.1 which breaks on the checkstyle reporting. There is already a [PR open](php-parallel-lint/PHP-Parallel-Lint#64) to fix this upstream. Once this PR has been merged and a new version of Parallel Lint has been released, the separate step for PHP 8.1 linting can be removed. * Makes the `test` job in the GHA workflow dependent on the `lint` job having passed... ... as the tests would fail anyway if there are linting errors. Also adjusts the name of the `test` jobs to include the word "Test" so they can be easily distinguished from the Lint jobs. Refs: * https://github.com/php-parallel-lint/PHP-Parallel-Lint
This commit: * Add a new dependency on the PHP Parallel Lint package for fast PHP linting. The PHP Parallel Lint package, in combination with the PHP Console Highlighter provides the following advantages in comparison with "plain" PHP linting: - Higher speed due to the parallel processes. - Improved usability by providing color coded syntax highlighting of found errors on the command-line. - Integration with the `cs2pr` tool, allowing for the results of the lint command to be shown in-line in PRs. * Adds a Composer `lint` script for easy access to the tool for devs, while making sure the correct command line parameters will be used. The linting command as currently set up, will also check the example files for linting errors. * Adds a GH Actions job for linting the code on the high/low supported PHP versions, one arbitrary interim version + an experimental build against PHP 8.1. The `cs2pr` tool has been enabled and will show the results of the non-experimental lint runs in-line in PRs. **Note**: For PHP 8.1, the `cs2pr` tool is not used as there is a known issue in the Parallel Lint tool with PHP 8.1 which breaks on the checkstyle reporting. There is already a [PR open](php-parallel-lint/PHP-Parallel-Lint#64) to fix this upstream. Once this PR has been merged and a new version of Parallel Lint has been released, the separate step for PHP 8.1 linting can be removed. * Makes the `test` job in the GHA workflow dependent on the `lint` job having passed... ... as the tests would fail anyway if there are linting errors. Also adjusts the name of the `test` jobs to include the word "Test" so they can be easily distinguished from the Lint jobs. Refs: * https://github.com/php-parallel-lint/PHP-Parallel-Lint
…type As of PHP 8.1, PHP adds return type declarations to the PHP native functions. For the `JsonSerializable::jsonSerialize()` interface method, the new signature is: ```php function jsonSerialize(): mixed {} ``` As this libary still supports PHP 5.3, it is not possible to add this return type as: 1. Return types weren't available until PHP 7.0 and 2. the `mixed` return type only became available in PHP 8.0. For libraries supporting PHP 7.0+, it would have been possible to fix this by adding an `array` return type (higher specificity). For libraries still supporting PHP < 7.0, there are two choices: 1. Either decouple from the `JsonSerialize` interface. 2. Or use a PHP 8.1 attribute to silence the deprecation notice. As prior to PHP 8.0, attributes are ignored as if they were comments, it is safe to add the attribute to the library and IMO, this is prefered over decoupling the classes from the `JsonSerializable` interface. To prevent PHPCS tripping up over "something" existing between the function docblock and the declaration, PHPCS 3.6.0 should be used, which is the first PHPCS version with full PHP 8.0 syntax support in the sniffs (albeit that there are still some small things to fix up in PHPCS). Refs: * https://wiki.php.net/rfc/internal_method_return_types * php/php-src#7051
46ad428
to
f6c4e1c
Compare
@grogy Friendly reminder.... |
As this PR is still open and more return types have been added to PHP native interfaces since, I've added a second commit, fixing another set of these. The PR description has been updated with details about the second commit. @grogy What can I do to move this forward ? Builds for projects using this package which have started to run CI against PHP 8.1 are failing due to this issue. Example: https://github.com/WordPress/WordPress-Coding-Standards/pull/2004/checks?check_run_id=3184200170 |
… method return types As of PHP 8.1, PHP adds return type declarations to the PHP native functions. For the `RecursiveFilterIterator`, the relevant method signatures are: * `accept(): bool` * `hasChildren(): bool` * `getChildren(): ?RecursiveFilterIterator` As this libary still supports PHP 5.3, it is not possible to add this return type as: 1. Return types weren't available until PHP 7.0 and 2. the `mixed` return type only became available in PHP 8.0. For libraries still supporting PHP < 7.0, there are two choices: 1. Either decouple from the interface. 2. Or use a PHP 8.1 attribute to silence the deprecation notice. As prior to PHP 8.0, attributes are ignored as if they were comments, it is safe to add the attribute to the library and IMO, this is prefered over decoupling the classes from the interface. To prevent PHPCS tripping up over "something" existing between the function docblock and the declaration, PHPCS 3.6.0 should be used, which is the first PHPCS version with full PHP 8.0 syntax support in the sniffs (albeit that there are still some small things to fix up in PHPCS). Refs: * https://wiki.php.net/rfc/internal_method_return_types
a70415a
to
06af4e4
Compare
PHP attributes are elegant solution. Thank you, I merge it with new release |
Thanks. When did you have mind for doing a new release ? Is anything else still needed for such a release ? |
Ah! Just saw the tag, thanks! |
I released new version now - https://github.com/php-parallel-lint/PHP-Parallel-Lint/releases/tag/v1.3.1 The problem with incorrect types for new version of PHP is important. Thanks for perfect description of options in MR, it was very helping ❤️ |
PHP 8.1: silence the deprecation notice about jsonSerialize() return type
As of PHP 8.1, PHP adds return type declarations to the PHP native functions.
For the
JsonSerializable::jsonSerialize()
interface method, the new signature is:As this libary still supports PHP 5.3, it is not possible to add this return type as:
mixed
return type only became available in PHP 8.0.For libraries supporting PHP 7.0+, it would have been possible to fix this by adding an
array
return type (higher specificity).For libraries still supporting PHP < 7.0, there are two choices:
JsonSerialize
interface.As prior to PHP 8.0, attributes are ignored as if they were comments, it is safe to add the attribute to the library and IMO, this is prefered over decoupling the classes from the
JsonSerializable
interface.To prevent PHPCS tripping up over "something" existing between the function docblock and the declaration, PHPCS 3.6.0 should be used, which is the first PHPCS version with full PHP 8.0 syntax support in the sniffs (albeit that there are still some small things to fix up in PHPCS).
Refs:
PHP 8.1: silence the deprecation notice about RecursiveFilterIterator method return types
As of PHP 8.1, PHP adds return type declarations to the PHP native functions.
For the
RecursiveFilterIterator
, the relevant method signatures are:accept(): bool
hasChildren(): bool
getChildren(): ?RecursiveFilterIterator
As this libary still supports PHP 5.3, it is not possible to add this return type as:
mixed
return type only became available in PHP 8.0.For libraries still supporting PHP < 7.0, there are two choices:
As prior to PHP 8.0, attributes are ignored as if they were comments, it is safe to add the attribute to the library and IMO, this is prefered over decoupling the classes from the interface.
To prevent PHPCS tripping up over "something" existing between the function docblock and the declaration, PHPCS 3.6.0 should be used, which is the first PHPCS version with full PHP 8.0 syntax support in the sniffs (albeit that there are still some small things to fix up in PHPCS).
Refs: