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

Conversation

@acrobat
Copy link
Collaborator

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?

lib/Github/Client.php Outdated Show resolved Hide resolved
Copy link
Collaborator

Nyholm left a comment

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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 acrobat:allow-httplug-2.0 branch 5 times, most recently from 06efa6d to 93a2d27 Aug 8, 2019
@acrobat

This comment has been minimized.

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 acrobat:allow-httplug-2.0 branch 3 times, most recently from a52f146 to 0fae913 Aug 8, 2019
@acrobat

This comment has been minimized.

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 force-pushed the acrobat:allow-httplug-2.0 branch from 0fae913 to 88cb0f6 Nov 3, 2019
@Nyholm Nyholm marked this pull request as ready for review Nov 3, 2019
@Nyholm Nyholm force-pushed the acrobat:allow-httplug-2.0 branch from 88cb0f6 to 206078e Nov 3, 2019
@Nyholm

This comment has been minimized.

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

This comment has been minimized.

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
2 checks passed
2 checks passed
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@acrobat acrobat deleted the acrobat:allow-httplug-2.0 branch Nov 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.