Declare tentative return types for ext/json #7051
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
krakjoe
added
the
Tentative returns
nikic
approved these changes
May 27, 2021
|
I've started running into "SomeClass::jsonSerialize() should be compatible with JsonSerializable::jsonSerialize(): mixed" errors. I realize the RFC for adding return types got approved, but just a simple GH search shows over a million matches for method declarations for As the I'd kindly like to ask you to reconsider adding return types to PHP native interfaces, or at the very least, to limit it to return types which were available prior to PHP 8.0. |
|
@jrfnl For cases where a return type cannot be added either due to PHP version requirements, or library backwards-compatibility policies, the deprecation warning can be suppressed using the For this particular case, if there are no additional library backwards-compatibility concerns, it's also possibly to use a more specific type -- almost all implementations of jsonSerialize() actually return array. @kocsismate Maybe the deprecation notice should mention the attribute? |
|
@nikic Thanks for your reply. The attribute is useful to know about, especially as it can be used PHP cross-version, though it does feel "off" having to use a PHP 8.0 feature which, as an implementation detail, just happens to be ignored by older PHP versions. I believe it will be valuable if the attribute can be mentioned in the changelog entry about this change. Lines 60 to 63 in 28a1a6b |
|
I also think it'd be good to mention this in the upgrading file. Perhaps you'd like to try a PR @jrfnl? |
jrfnl
added a commit
to jrfnl/PHP-Parallel-Lint
that referenced
this pull request
Jun 21, 2021
…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
This was referenced Jun 21, 2021
jrfnl
added a commit
to jrfnl/PHP-Parallel-Lint
that referenced
this pull request
Jul 23, 2021
…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
pento
pushed a commit
to WordPress/wordpress-develop
that referenced
this pull request
Jul 30, 2021
…hould be compatible with `JsonSerializable::jsonSerialize(): mixed`" error on PHP 8.1. This relates to the [https://wiki.php.net/rfc/internal_method_return_types Return types for internal methods RFC] in PHP 8.1 and in particular, the change made in [php/php-src#7051 PHP PR #7051], which adds a `mixed` return type to the `JsonSerializable::jsonSerialize()` interface method. WordPress only contains one (test) class which implements the `JsonSerializable` interface and this commit fixes the issue for that class. As of PHP 8.1, the `jsonSerialize()` method in classes which implement the `JsonSerializable` interface are expected to have a return type declared. The return type should be `mixed` or a more specific type. This complies with the Liskov principle of covariance, which allows the return type of a child overloaded method to be more specific than that of the parent. The problem with this is that: 1. The `mixed` return type was only introduced in PHP 8.0. 2. Return types in general were only introduced in PHP 7.0. WordPress still has a minimum PHP version of 5.6, so adding the return type is not feasible for the time being. The solution chosen for now is to add an attribute to silence the deprecation warning. While attributes are a PHP 8.0+ feature, due to the choice of the `#[]` syntax, in PHP < 8.0, attributes will just be ignored and treated as comments, so there is no drawback to using the attribute. Props jrf. See #53635. git-svn-id: https://develop.svn.wordpress.org/trunk@51517 602fd350-edb4-49c9-b593-d223f7449a82
nylen
pushed a commit
to nylen/wordpress-develop-svn
that referenced
this pull request
Jul 30, 2021
…hould be compatible with `JsonSerializable::jsonSerialize(): mixed`" error on PHP 8.1. This relates to the [https://wiki.php.net/rfc/internal_method_return_types Return types for internal methods RFC] in PHP 8.1 and in particular, the change made in [php/php-src#7051 PHP PR #7051], which adds a `mixed` return type to the `JsonSerializable::jsonSerialize()` interface method. WordPress only contains one (test) class which implements the `JsonSerializable` interface and this commit fixes the issue for that class. As of PHP 8.1, the `jsonSerialize()` method in classes which implement the `JsonSerializable` interface are expected to have a return type declared. The return type should be `mixed` or a more specific type. This complies with the Liskov principle of covariance, which allows the return type of a child overloaded method to be more specific than that of the parent. The problem with this is that: 1. The `mixed` return type was only introduced in PHP 8.0. 2. Return types in general were only introduced in PHP 7.0. WordPress still has a minimum PHP version of 5.6, so adding the return type is not feasible for the time being. The solution chosen for now is to add an attribute to silence the deprecation warning. While attributes are a PHP 8.0+ feature, due to the choice of the `#[]` syntax, in PHP < 8.0, attributes will just be ignored and treated as comments, so there is no drawback to using the attribute. Props jrf. See #53635. git-svn-id: https://develop.svn.wordpress.org/trunk@51517 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith
pushed a commit
to markjaquith/WordPress
that referenced
this pull request
Jul 30, 2021
…hould be compatible with `JsonSerializable::jsonSerialize(): mixed`" error on PHP 8.1. This relates to the [https://wiki.php.net/rfc/internal_method_return_types Return types for internal methods RFC] in PHP 8.1 and in particular, the change made in [php/php-src#7051 PHP PR #7051], which adds a `mixed` return type to the `JsonSerializable::jsonSerialize()` interface method. WordPress only contains one (test) class which implements the `JsonSerializable` interface and this commit fixes the issue for that class. As of PHP 8.1, the `jsonSerialize()` method in classes which implement the `JsonSerializable` interface are expected to have a return type declared. The return type should be `mixed` or a more specific type. This complies with the Liskov principle of covariance, which allows the return type of a child overloaded method to be more specific than that of the parent. The problem with this is that: 1. The `mixed` return type was only introduced in PHP 8.0. 2. Return types in general were only introduced in PHP 7.0. WordPress still has a minimum PHP version of 5.6, so adding the return type is not feasible for the time being. The solution chosen for now is to add an attribute to silence the deprecation warning. While attributes are a PHP 8.0+ feature, due to the choice of the `#[]` syntax, in PHP < 8.0, attributes will just be ignored and treated as comments, so there is no drawback to using the attribute. Props jrf. See #53635. Built from https://develop.svn.wordpress.org/trunk@51517 git-svn-id: http://core.svn.wordpress.org/trunk@51128 1a063a9b-81f0-0310-95a4-ce76da25c4cd
gMagicScott
pushed a commit
to gMagicScott/core.wordpress-mirror
that referenced
this pull request
Jul 30, 2021
…hould be compatible with `JsonSerializable::jsonSerialize(): mixed`" error on PHP 8.1. This relates to the [https://wiki.php.net/rfc/internal_method_return_types Return types for internal methods RFC] in PHP 8.1 and in particular, the change made in [php/php-src#7051 PHP PR #7051], which adds a `mixed` return type to the `JsonSerializable::jsonSerialize()` interface method. WordPress only contains one (test) class which implements the `JsonSerializable` interface and this commit fixes the issue for that class. As of PHP 8.1, the `jsonSerialize()` method in classes which implement the `JsonSerializable` interface are expected to have a return type declared. The return type should be `mixed` or a more specific type. This complies with the Liskov principle of covariance, which allows the return type of a child overloaded method to be more specific than that of the parent. The problem with this is that: 1. The `mixed` return type was only introduced in PHP 8.0. 2. Return types in general were only introduced in PHP 7.0. WordPress still has a minimum PHP version of 5.6, so adding the return type is not feasible for the time being. The solution chosen for now is to add an attribute to silence the deprecation warning. While attributes are a PHP 8.0+ feature, due to the choice of the `#[]` syntax, in PHP < 8.0, attributes will just be ignored and treated as comments, so there is no drawback to using the attribute. Props jrf. See #53635. Built from https://develop.svn.wordpress.org/trunk@51517 git-svn-id: https://core.svn.wordpress.org/trunk@51128 1a063a9b-81f0-0310-95a4-ce76da25c4cd
grogy
pushed a commit
to php-parallel-lint/PHP-Parallel-Lint
that referenced
this pull request
Aug 13, 2021
…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
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.
No description provided.