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

support money 4.0, remove prophecy deprecation warnings #112

Merged
merged 9 commits into from
Jan 10, 2022

Conversation

frederikbosch
Copy link
Contributor

No description provided.

@localheinz
Copy link
Contributor

@frederikbosch

What do you think about dropping support for otherwise unsupported versions of PHP in a separate pull request?

@frederikbosch
Copy link
Contributor Author

@localheinz I decided to drop versions because it's hard to get the tests working with moneyphp 4.0 because of the committed composer.lock.

@frederikbosch
Copy link
Contributor Author

@localheinz Dropping PHP 7.2 prevents a dependency hell. See the failing tests. With a committed composer.lock, multiple PHP versions, multiple Money versions, I decided not to split. And, while I was at it, I thought why not remove the unsupported versions. Hence I removed PHP 7.3 too. We could tag this as minor version update (2.3.0) because nothing changed in the src folder, no BC breaks.

run: composer install

- name: Setup problem matchers for PHP
run: echo "::add-matcher::${{ runner.tool_cache }}/php.json"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frederikbosch

Ha, nice, wasn't aware of this matcher!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took it from the Money lib

@PowerKiKi
Copy link
Collaborator

PowerKiKi commented Jan 10, 2022

There's already a lot going on this PR...

As far as I am concerned I'd like to keep support for PHP 7.4, but anything older (and thus EOL) can, and should, be dropped.

If we assume that 7.4 becomes the "odd" one, and PHP 8.0 is the "default", then I'd rather have the CI treat 7.4 as a special case with composer update but all other versions, 8.0 and 8.1 use the standard composer install. This CI is faster and more predictable.

@PowerKiKi
Copy link
Collaborator

Actually what I had in mind was an optional step like so:

 - name: Remove lock for older PHP
  if: matrix.php-version == '7.4'
  run: rm composer.lock

Then there is never a single composer update anywhere in the CI. I think it is easier to read, understand and thus maintain.

@PowerKiKi
Copy link
Collaborator

I see the advantage of doing it that way, but IMHO this is over-engineered for this project. I'd rather keep things simple.

@frederikbosch
Copy link
Contributor Author

@PowerKiKi I applied your PHP 7.4 suggestions.

@frederikbosch
Copy link
Contributor Author

frederikbosch commented Jan 10, 2022

If this is fine with you @PowerKiKi and @localheinz, I will merge it in and tag 2.3.0.

curl -LO https://scrutinizer-ci.com/ocular.phar
php ocular.phar code-coverage:upload --format=php-clover coverage-clover.xml
- name: Upload Scrutinizer coverage
uses: sudo-bot/action-scrutinizer@latest
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is totally fine with me, but I was curious if there are any advantages of using an action here ?

Copy link
Collaborator

@PowerKiKi PowerKiKi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

However if you feel like removing prophecy I wouldn't mind either. But that can also be done later...

If you release, be sure to create an annotated tag that contains the changelog, so we get pretty GitHub releases for free (and nice git history)

@PowerKiKi
Copy link
Collaborator

(maybe just a little squash ?)

Copy link
Contributor

@localheinz localheinz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@frederikbosch frederikbosch merged commit 946257f into master Jan 10, 2022
@frederikbosch frederikbosch deleted the money_4 branch January 10, 2022 16:17
@frederikbosch
Copy link
Contributor Author

Thanks both for your quick help!

@localheinz
Copy link
Contributor

Thank you for putting in the work, @frederikbosch!

@frederikbosch
Copy link
Contributor Author

I see that there is a release Github action, I missed that one. That is failing now. I created the tag via Github releases.

Alvyre pushed a commit to stadline/camt that referenced this pull request Oct 9, 2024
* support money 4.0
* remove prophecy deprecation warnings
* drop unsupported php versions
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.

3 participants