search

moneyphp / money

PHP implementation of Fowler's Money pattern.
http://moneyphp.org
MIT License
4.6k stars 440 forks source link

JSON serialization return type warning under PHP 8.1 #656

Closed Firehed closed 2 years ago

Firehed commented 3 years ago

When starting to test on the 8.1 alpha builds, I encountered the following deprecation warnings from this library:

PHP Deprecated:  Declaration of Money\Currency::jsonSerialize() should be compatible with JsonSerializable::jsonSerialize(): mixed in /var/www/html/vendor/moneyphp/money/src/Currency.php on line 86
PHP Deprecated:  Declaration of Money\Money::jsonSerialize() should be compatible with JsonSerializable::jsonSerialize(): mixed in /var/www/html/vendor/moneyphp/money/src/Money.php on line 521

I'm currently still on 3.x, but it looks like the issue is present on 4.x and HEAD.

It looks like adding native return types (replacing the docblock annotations) fixes the issue, and as the two classes are declared final I don't think it should create any BC issues.

Happy to send a PR to fix if you'd like - let me know!

frederikbosch commented 3 years ago

Reopen when 8.1 is out of the door and issue still there. We will not create fixes for alpha releases of PHP.

Firehed commented 3 years ago

Will do :)

someniatko commented 2 years ago

@frederikbosch Hi, it is still the same for the PHP 8.1RC6, which is the last pre-release version.

jaikdean commented 2 years ago

I've confirmed this to also be present under the GA release of PHP 8.1.0.

Firehed commented 2 years ago

I'm more than happy to create a PR to fix this, but I see at least one open that's trying to do the same thing. Can any maintainer comment on how they'd like to proceed?

frederikbosch commented 2 years ago

PR #670 is the way forward. At the moment incompatible dependencies - phpspec in particular - are blocking the merge of this PR now.

AlexandruGG commented 2 years ago

PR #670 is the way forward. At the moment incompatible dependencies - phpspec in particular - are blocking the merge of this PR now.

hey @frederikbosch I also opened #675, could you please have a look? I think dev dependencies don't need to block this as it's quite a small change

frederikbosch commented 2 years ago

Support is there, see version 4.0.3. Thanks for the help.