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

Feat: PHP8 / Symfony 5+ support #47

Merged
merged 8 commits into from
Jan 17, 2022
Merged

Conversation

bpolaszek
Copy link
Collaborator

This PR adds support for PHP8 and Symfony 5+, by bumping minimal PHP version to 7.1 and adding return types.
Hence it also drops support for older Symfony versions, and will require a new major release as some BC breaks are expected.

@lennerd
Copy link
Owner

lennerd commented May 5, 2021

Wow, awesome! Thanks for putting this together. Will review this ASAP.

@lennerd lennerd self-requested a review May 5, 2021 07:59
@bpolaszek
Copy link
Collaborator Author

Looks like Travis isn't triggered. Do you want to keep it or do you want me to switch this repo to Github Actions?

@coveralls
Copy link

coveralls commented May 5, 2021

Coverage Status

Coverage increased (+1.1%) to 96.875% when pulling 1adaa71 on bpolaszek:feat/php8-sf5 into b35c10c on lennerd:master.

Copy link
Owner

@lennerd lennerd left a comment

Choose a reason for hiding this comment

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

I had a first change to look at the code. Over all LGTM (see comments, questions, requests). Again thanks for the time you spend updating all classes and especially the tests. 👍

Next (probably in the evening) I will look at the Travis setup. Looks like there is some configuration missing or which at least needs to be fixed. Will keep you posted.

.gitignore Show resolved Hide resolved
BotDetector.php Show resolved Hide resolved
Metadata/MetadataInterface.php Show resolved Hide resolved
Metadata/Dumper/MetadataDumperInterface.php Show resolved Hide resolved
BotDetectorInterface.php Show resolved Hide resolved
composer.lock Show resolved Hide resolved
@lennerd
Copy link
Owner

lennerd commented May 13, 2021

Replaced Travis CI setup with Github Actions. Let me know what you think.

@bpolaszek
Copy link
Collaborator Author

Hi @lennerd,

Sorry, still didn't have the time to review your review, but I don't forget it, it's still in my todos.

Thanks!

Copy link
Collaborator Author

@bpolaszek bpolaszek left a comment

Choose a reason for hiding this comment

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

Done, finally :-)

@bpolaszek
Copy link
Collaborator Author

Rebased + updated Github Actions config.

@lennerd
Copy link
Owner

lennerd commented Jun 30, 2021

@bpolaszek not sure how Github send notifications about failed checked. So in case you didn't notice … 😬 😉

@bpolaszek
Copy link
Collaborator Author

Hey @lennerd - sorry I keep snoozing that one 😞
I'm not currently available to work on this at the very moment, but don't hesitate to harass me if you don't get any news until next month.

Thanks!
Ben

@lennerd
Copy link
Owner

lennerd commented Jul 7, 2021

No problem and thanks for letting me know. Will notify you next month if needed. 😉

@bpolaszek
Copy link
Collaborator Author

bpolaszek commented Jul 25, 2021

Hey @lennerd - I just took a quick look again and CI fails because Symfony's config component LoaderInterface::load has changed its signature from 4.4 to 5.0.

So, either we have to maintain 2 distinct implementations of YamlLoader (and properly detect what version of the Config component is used) which is a PITA, or, ship a 2.1 version dropping Symfony 4 support (and I'd advocate for dropping PHP < 7.4 support as well, since older versions are EOLed).

WDYT?

@bpolaszek bpolaszek force-pushed the feat/php8-sf5 branch 3 times, most recently from 98edd4d to 95f51e3 Compare August 16, 2021 14:14
@bpolaszek
Copy link
Collaborator Author

I dropped PHP < 7.4 support as well as Symfony < 5.
This would require a major version bump. CI passes. WDYT?

@bpolaszek
Copy link
Collaborator Author

Ping @lennerd :-)

@lennerd
Copy link
Owner

lennerd commented Jan 13, 2022

@bpolaszek sorry for the long delay! And thanks again for all the changes. Everything looks good so far.

My problem is I haven‘t used this library nor Symfony or even PHP for a very long time. So to be honest maintaing this project is not my top priority anymore. But, I might have a wild idea.

What do you think about beeing a contributer to this project? As a first act you could merge this PR. 🙂

@bpolaszek
Copy link
Collaborator Author

Hi @lennerd,

No worries, I understand 🙂
Sure, I'll do my best to keep it up 👍

@lennerd
Copy link
Owner

lennerd commented Jan 14, 2022

@bpolaszek Nice. 👍 Invitation should be on it‘s way. Let me know if this works for you.

@bpolaszek
Copy link
Collaborator Author

bpolaszek commented Jan 15, 2022

@lennerd sounds good 🙂 may I push the button? I usually use the "squash/merge" option in github in my other projects, but if you prefer merge commits, please let me know. Thanks again! 🙏

Capture d’écran 2022-01-15 à 08 07 59

@lennerd
Copy link
Owner

lennerd commented Jan 15, 2022

You may! 🙂 In the meantime I also prefer squash and merge.

So I would say, go for it!

@bpolaszek bpolaszek merged commit 8e88207 into lennerd:master Jan 17, 2022
@bpolaszek bpolaszek deleted the feat/php8-sf5 branch January 17, 2022 08:14
@bpolaszek
Copy link
Collaborator Author

Here we go! 🎉

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

3 participants