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

Php8.1 resolve json serializable return datatype deprecation #145

Conversation

MarkBaker
Copy link
Contributor

Resolution for Issue #144

As of PHP 8.1, PHP adds return type declarations to the PHP native functions and interfaces.

For the JsonSerializable::jsonSerialize() interface method, the new signature is:

function jsonSerialize(): mixed {}

As this library still supports PHP 7.3, it is not possible to add this return type, as the mixed return type only became available in PHP 8.0.

As prior to PHP 8.0, attributes are ignored as if they were comments, it is safe to add the attribute to the library.

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; but that should be done as a separate PR.

@mnapoli
Copy link
Member

mnapoli commented Jun 28, 2021

Thank you for tackling this @MarkBaker!

Could we also drop support for PHP 7.3? People using PHP 7.3 will still be able to install the current version of php-enum anyway. That way we bump to PHP 7.4.

WDYT?

@MarkBaker
Copy link
Contributor Author

I see no problem with that, even though my own libraries using this are PHP >=7.2 and will remain so at least until next year. This change doesn't create any BC break, but the previous versions are there anyway; and it's always good to give a little push to upgrade when a new version is .released, even if people aren't yet ready to take the bigger step to PHP 8.

@mnapoli
Copy link
Member

mnapoli commented Jun 28, 2021

Oh I realize that means we must bump to PHP 8.0, because mixed won't be supported on PHP 7.4 🤔 (or I'm missing something)

@MarkBaker
Copy link
Contributor Author

MarkBaker commented Jun 28, 2021

Actually you don't need to bump to 8.0 and apply mixed as the return datatype for jsonSerialized, not with this change. With PHP 8.1, you would normally get the deprecated notice about the mismatch in the jsonSerialized() interface signature; but the #[\ReturnTypeWillChange] annotation suppresses that deprecation notice for PHP 8.1; for PHP 8.0, there's no deprecation notice to suppress because the interface signature does match, so the annotation does nothing (although it is still recognised as an annotation); for PHP < 8.0, the interface signature matches, and the annotation is simply treated as a comment line in the source.

So really the choice is yours: boost the minimum requirement to PHP7.4 for a 1.9.0 release, and modify the jsonSerialize() method signature to have a return datatype of mixed; or this annotation, which allows you to retain backward compatibility with PHP 7.3.

All I'm really doing is offering a choice of approaches.

@mnapoli
Copy link
Member

mnapoli commented Jun 29, 2021

Yep, thanks a lot for all these details, that is actionnable.

Let's merge this one for now, thank you!

@mnapoli mnapoli merged commit 55555d3 into myclabs:master Jun 29, 2021
@MarkBaker MarkBaker deleted the PHP8.1-Resolve-JsonSerializable-Return-Datatype-Deprecation branch June 29, 2021 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants