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

Allow httplug 2.0 #802

Merged
merged 2 commits into from Nov 3, 2019
Merged

Allow httplug 2.0 #802

merged 2 commits into from Nov 3, 2019

Conversation

acrobat
Copy link
Collaborator

@acrobat acrobat commented Aug 6, 2019

A pr to try/test if we can support both v1 and v2 of httplug. Currently there are still errors regarding typehint incompatibilities, so still a WIP to get things up and running

/cc @Nyholm is it actually possible to support both versions?

Copy link
Collaborator

@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.

Yes, it is very possible.

Difference between version 1 and 2 of php-http/httplug is just PHP7 return types.

composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@acrobat
Copy link
Collaborator Author

acrobat commented Aug 7, 2019

Yes, it is very possible.

Difference between version 1 and 2 of php-http/httplug is just PHP7 return types.

@Nyholm

Ok bumping the supported php version to 7.1 (currently the oldest supported version) would kinda solve this. But the failures in travis are related about return typehints, but adding them here (and matching those of httplug) is a BC break in our lib as users could extend these classes/methods. Or am I seeing things wrong here?

@Nyholm
Copy link
Collaborator

Nyholm commented Aug 7, 2019

Ok bumping the supported php version to 7.1

Not needed if we still support version 1.0 of httplug.

@acrobat
Copy link
Collaborator Author

acrobat commented Aug 7, 2019

@Nyholm but without bumping php we can't support httplug 2.0. So I guess the only way to do it, is to bump the major version and switch to httplug v2?

@acrobat acrobat force-pushed the allow-httplug-2.0 branch 5 times, most recently from 06efa6d to 93a2d27 Compare August 8, 2019 17:54
@acrobat
Copy link
Collaborator Author

acrobat commented Aug 8, 2019

Ok, this looks promising! @Nyholm I've found your PR for the mailgun api lib and so I learned that the VersionBridgePlugin class exists. The only failing tests are now caused by a final class in httplug 2.0

I will take a look next time to get these tests passing again

@acrobat acrobat force-pushed the allow-httplug-2.0 branch 3 times, most recently from a52f146 to 0fae913 Compare August 8, 2019 19:54
@acrobat
Copy link
Collaborator Author

acrobat commented Aug 8, 2019

Implementation is done and all tests are green now, but as I suspected the BC check fails now. So I guess really the only way to do it is bump the httplug requirement to v2 in the next major release?

@Nyholm Nyholm marked this pull request as ready for review November 3, 2019 11:16
@Nyholm
Copy link
Collaborator

Nyholm commented Nov 3, 2019

Sorry for being slow coming back to this.

The BC warnings you see is actually fine =)

You get a bunch new from our ExceptionInterface. That is because HTTPlug starting to implement throwable. The ExceptionInterface has always intended to be implemented on an exception => which will implement these new functions... So you will only get issues if you have a non-exception and implement our ExceptionInterface.

The other "BC warnings" comes from our plugins. They use the "version bridge" to automatically adapt if you installed HTTPlug v1 or v2.

@Nyholm Nyholm changed the title [WIP] Allow httplug 2.0 Allow httplug 2.0 Nov 3, 2019
@Nyholm
Copy link
Collaborator

Nyholm commented Nov 3, 2019

Thank you @acrobat for the work you put in to this.

@Nyholm Nyholm merged commit 18c6083 into KnpLabs:master Nov 3, 2019
@acrobat acrobat deleted the allow-httplug-2.0 branch November 3, 2019 13:48
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