search

moneyphp / money

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

Add `jsonSerialize` Return Types #675

Closed AlexandruGG closed 2 years ago

AlexandruGG commented 2 years ago

Add jsonSerialize return types

Addresses: https://github.com/moneyphp/money/issues/656 Related to PR: https://github.com/moneyphp/money/pull/670

While we can't say that Money fully supports 8.1 until the workflows are updated to run on it, the benefit of this is we don't have to wait for PHPSpec fixes from https://github.com/phpspec/phpspec/pull/1397. Once that is fixed we can have CI running on 8.1 as well.

Also note that these changes are completely safe even without running workflows on 8.1.

AlexandruGG commented 2 years ago

hey @sagikazarmark @driesvints 👋

What do you think of this one? Would allow users to update to PHP 8.1 without the deprecations

driesvints commented 2 years ago

I don't think it's safe to merge this without also running the builds. You'll be left in the dark if anything goes amis.

driesvints commented 2 years ago

Another option is to drop PhpSpec altogether if we don't want to wait and convert all tests to PHPUnit tests.

AlexandruGG commented 2 years ago

I don't think it's safe to merge this without also running the builds. You'll be left in the dark if anything goes amis.

I'm not suggesting to merge this without running the existing CI builds.

If you're referring to running the workflows on 8.1 like here, I think that's definitely something to aim for but I see it as unrelated to this PR. The changes here are quite small, adding those return types shouldn't make any difference in terms of functionality in either 8.0 or 8.1.

driesvints commented 2 years ago

@AlexandruGG I meant the PHP 8.1 builds. I don't think it's safe to merge 8.1 support if there's no actual builds to verify the implementation.

AlexandruGG commented 2 years ago

@AlexandruGG I meant the PHP 8.1 builds. I don't think it's safe to merge 8.1 support if there's no actual builds to verify the implementation.

I agree that the library should not state it supports PHP 8.1 unless it also runs its workflows on 8.1

Perhaps we could rename this PR to "improve typing", because in essence that's all it's doing.

driesvints commented 2 years ago

@AlexandruGG Ah I finally see what you mean. Yes, you're correct, that sounds reasonable 👍

frederikbosch commented 2 years ago

@AlexandruGG Thank you, your commit was merged in the final PR that also fixed the CI pipeline, see #676.