moneyphp / money

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

Support PHP 8.2 #715

Closed rvanlaak closed 1 year ago

rvanlaak commented 2 years ago

Seems PHP 8.2 is supported without having to make any changes.

phpspec is not supporting 8.2 yet, but work is in progress. This is blocking us from adding php 8.2 support for now with the current set of requirements.

rvanlaak commented 2 years ago

Closing for now. Wanted to work on this on my own fork first.

rvanlaak commented 2 years ago

phpspec is not supporting 8.2 yet, but https://github.com/phpspec/phpspec/pull/1435. This is blocking us from adding PHP 8.2 support for now with the current set of requirements.

image
evertharmeling commented 1 year ago

With PHP 8.2 now released, it would be nice to have this merged and available :)

mattvb91 commented 1 year ago

any chance of getting this in?

rvanlaak commented 1 year ago

Marked PR as ready for review. CI pipeline run requires maintainer approval.

ruudk commented 1 year ago

@frederikbosch Would you mind approving the workflow so that we can see if it's mergable? Thanks 🙏

shouze commented 1 year ago

Looks like because of phpspec & prophecy, need to add --ignore-platform-req=php to composer install, at least during time of those dependencies upgrade.

frederikbosch commented 1 year ago

The current CI / Docs error is fixed in master.

rogervila commented 1 year ago

PHPSpec 8.2 support is merged but not yet released (see https://github.com/phpspec/phpspec/issues/1434)

@rvanlaak you will have to re-run CI once it is available.

rogervila commented 1 year ago

I fixed PHPSpec CI issues here https://github.com/phpspec/phpspec/pull/1437 I will comment here once a new version is available

rogervila commented 1 year ago

@rvanlaak PHPSpec 7.3.0 is available! Could you re-run CI steps for PHP 8.2?

rogervila commented 1 year ago

Hi @frederikbosch, could you re-run the CI steps for PHP 8.2, since its dependencies have been already upgraded? Thank you!

rogervila commented 1 year ago

Wait, why is there a composer.lock file on the package? @frederikbosch is there any reason to have it? Usually, packages do not contain lock files. @rvanlaak could you please remove it?

Update: Could we merge #722 first?

frederikbosch commented 1 year ago

I believe this was chosen because we were also applying mutation testing, and were planning to use dependabot. See this PR. But, feel free to remove it. I am not against that. I never commit composer.lock in other libs.

frederikbosch commented 1 year ago

You can cherry pick the last two commits - fix docs and remove composer.lock - from master into this branch to see if CI succeeds now.

rogervila commented 1 year ago

Thank you @frederikbosch I have seen some psalm errors on master. I am opening another PR to fix them.

frederikbosch commented 1 year ago

Great, I was also preparing to do that, but please go ahead.

frederikbosch commented 1 year ago

See #724

frederikbosch commented 1 year ago

Thanks for all your help!