Skip to content

Replace HTTPlug factories by PSR-17 #1184

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 15, 2023
Merged

Replace HTTPlug factories by PSR-17 #1184

merged 2 commits into from
Dec 15, 2023

Conversation

nicolas-grekas
Copy link
Contributor

All interfaces of php-http/message-factory are now deprecated.
This PR provides a way to move to PSR-17 instead.

@nicolas-grekas nicolas-grekas force-pushed the psr17 branch 4 times, most recently from 7c339e5 to 411e864 Compare May 11, 2023 12:33
@nicolas-grekas
Copy link
Contributor Author

Failures will be fixed once 4.4 will be tagged.

@norkunas
Copy link
Member

@jbelien could you review/merge this? :)

Nyholm
Nyholm approved these changes Jun 30, 2023
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you.

If we do a new Major after this PR, then we can also remove the getMessageFactory() method instead of deprecate it.

@jbelien
Copy link
Member

jbelien commented Jul 8, 2023 •
edited
Loading

Thanks for the review @Nyholm
Good idea indeed to create a new major release so we can also remove getMessageFactory(). 👍

@jbelien jbelien linked an issue Jul 9, 2023 that may be closed by this pull request
Abandoned dependency php-http/message-factory #1187
Closed
jbelien
jbelien previously requested changes Jul 9, 2023
View reviewed changes
Copy link
Member

@jbelien jbelien left a comment

Choose a reason for hiding this comment

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

Could you fix the 8 failing tests ? I had a quick look and it seems related to createRequest() function.

@nicolas-grekas
Copy link
Contributor Author

nicolas-grekas commented Aug 5, 2023 •
edited
Loading

Needs geocoder-php/provider-integration-tests#20 before I can move forward.

@jbelien
Copy link
Member

jbelien commented Aug 6, 2023

Needs geocoder-php/provider-integration-tests#20 before I can move forward.

Woops! Sorry, I've missed that one. Checking it now 👍

@ker0x
Copy link

ker0x commented Sep 25, 2023

Hello @nicolas-grekas @jbelien ! Is there any progress on this PR?

@jbelien
Copy link
Member

jbelien commented Sep 26, 2023

Hello @nicolas-grekas @jbelien ! Is there any progress on this PR?

I've just merged geocoder-php/provider-integration-tests#20 and created a new release ; @nicolas-grekas can now update this PR. 👍

@nicolas-grekas nicolas-grekas force-pushed the psr17 branch 10 times, most recently from 68d716c to 162224a Compare September 27, 2023 10:58
@nicolas-grekas
Copy link
Contributor Author

PR rebased and ready. The two failures are related to the subtree splitting and need v4.6.0 of the common-http package to be released.

This was referenced Oct 2, 2023
Merged
Open
@Chris53897
Copy link
Contributor

@jbelien Is there a planned release date for version 4.6 for https://github.com/geocoder-php/php-common-http

@Stadly
Copy link
Contributor

Stadly commented Nov 3, 2023

Looking forward to this being released :)

@ruudk
Copy link
Contributor

ruudk commented Dec 14, 2023

@Nyholm You are probably very busy, but do you know what is needed to move this PR forward? Thanks 💙

Nyholm
Nyholm approved these changes Dec 14, 2023
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you.

I think it all looks good. Just one minor

src/Http/composer.json Outdated
"php-http/message-factory": "^1.0.2",
"psr/http-message": "^1.0 || ^2.0",
"psr/http-message-implementation": "^1.0",
"php": "^7.4 || ^8.0",
Copy link
Member

Choose a reason for hiding this comment

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

This looks like an issue while rebasing. Please remove this line and support php8 only.

Nyholm
Nyholm approved these changes Dec 14, 2023
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

@jbelien I am happy to merge.

@jbelien jbelien dismissed their stale review December 15, 2023 15:59

Unblock reviews!

@jbelien
Copy link
Member

jbelien commented Dec 15, 2023

@jbelien I am happy to merge.

Thanks a lot @Nyholm for taking care of it! 😄
Sorry last few months have been very busy.
I've dismissed my review so it's not blocking.

@Nyholm
Copy link
Member

Nyholm commented Dec 15, 2023

No worries. That is how it is sometimes.

Let's move forward with this =)

@Nyholm Nyholm merged commit b89f986 into geocoder-php:master Dec 15, 2023
@Nyholm
Copy link
Member

Nyholm commented Dec 15, 2023

Thank you for the PR and the reviews

@nicolas-grekas nicolas-grekas deleted the psr17 branch December 18, 2023 08:51
@ruudk ruudk mentioned this pull request Dec 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nyholm Nyholm Nyholm approved these changes

@jbelien jbelien jbelien left review comments

Assignees
No one assigned
Labels
improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Abandoned dependency php-http/message-factory
8 participants
@nicolas-grekas @norkunas @jbelien @ker0x @Chris53897 @Stadly @ruudk @Nyholm