Vonage / vonage-php-sdk-core

Vonage REST API client for PHP. API support for SMS, Voice, Text-to-Speech, Numbers, Verify (2FA) and more.
https://developer.vonage.com/
Apache License 2.0
916 stars 180 forks source link

Currently Tied to the HTTPlug Guzzle Adaptor #5

Closed tjlytle closed 4 years ago

tjlytle commented 8 years ago

For ease of installation, the HTTPlug Guzzle 6 adapter is in require. You can certainly add another adapter and use that when creating the client, but that leaves unused code under /vendor and provides no good way to remove the Guzzle dependency if there's some version conflict (say another package using Guzzle 5).

Not having any adapter required would mean installation took two commands (with the first replaceable by another adaptor):

composer require php-http/guzzle6-adapter
composer require nexmo/client

Is that an issue? Could a composer hook be used to interactively prompt for an adaptor if one doesn't exist?

Nyholm commented 8 years ago

Yes, If you require the virtual package php-http/client-implementation then composer will fail and ask you to install a HTTPlug client. See http://docs.php-http.org/en/latest/httplug/library-developers.html

Nyholm commented 8 years ago

You could also use http-php/discovery to remove the dependency to zendframework/zend-diactoros

tjlytle commented 8 years ago

@Nyholm Yeah, understand the value of using php/client-implementation. Main concern is having to communicate that you need to both require nexmo/client and pick an adaptor. Shipping with one by default helps solve that, but certainly not a perfect solution.

Haven't fully explored this, but do you know if there's any way to interrupt composer's error (when missing a client implementation), and dynamically add guzzle as a default?

I did play with http-php/discovery, but found puli kind of hard to work with / understand. I didn't take a lot of time with it though.

Nyholm commented 8 years ago

Depending on php-http/client-implementation will interrupt composer if a client is missing, but you can't provide a default one. You could say something like:

to install this lib run composer require nexmo/client php-http/guzzle6-adaper

Have you seen the discovery lib after version 1.0? Puli is no longer a requirement. :)

tjlytle commented 8 years ago

Puli is no longer a requirement. :)

I'll take another look.

Nyholm commented 8 years ago

I wrote it myself. I'm pretty sure :) You can use puli but it is no requirement.

mheap commented 7 years ago

I had a chat with @tjlytle about this yesterday. We're planning on renaming nexmo/client to nexmo/client-core and have nexmo/client be an empty package that requires nexmo/client-core and php-http/guzzle6-adapter.

Any reason that this wouldn't work?

Nyholm commented 7 years ago

Sure. That would work. Then people that do not want to use Guzzle can require nexmo/client-core. It works and is is okey. But the alternative would be to have an extra line in the readme saying: composer require nexmo/client php-http/guzzle6-adaper

FYI: nexmo/client would be the same as our "quick start": https://github.com/php-http/httplug-quickstart

Ilyes512 commented 5 years ago

Why use http-php if your force feed a specific version of the guzzle adapter... I can't use php-http/guzzle6-adaptor in my Laravel (5.7) project because somehow this frigging library got added as a requirement (which is not the fault of the maintainers of this repo). Removing the requirement and adding the virtual package is indeed the solution...

A half decent solution would also be to update the adapter version to ^2...

atymic commented 5 years ago

I'm also blocked from installing this package due to the v1 adapter requirement. For the moment i'll have to fork and install that :(

lornajane commented 5 years ago

Thanks for commenting! Sorry for the delay, we're putting in a few changes for a v2 release but one of them will remove this problem - I'll try to keep you all posted as we go along, I think PR #182 is the most relevant here

dennis-koster commented 5 years ago

This is quite a big problem actually. It is now impossible to use version 2 of php-http/httplug within Laravel 5.7, as:

Requiring the guzzle6-adapter sort of defeats the purpose of having your package be setup to accept any httpplug compliant http client in my opinion. You can use the suggest keyword in composer.json instead. For instance:

"suggest": {
    "php-http/guzzle5-adapter": "Install this package if Guzzle 5 is available in the application.",
    "php-http/guzzle6-adapter": "Install this package if Guzzle 6 is available in the application.",
    "php-http/curl-client": "Install this package if Guzzle is not available in the application, but curl is."
  }
dragonmantank commented 5 years ago

@denniskoster We hope to have this fixed with the upcoming v2 release here within the next few weeks, because as you've noticed this is becoming more and more of an issue.

dennis-koster commented 5 years ago

@dragonmantank Thanks for the quick response.

That's good news. However, laravel will still require ^1.0 indirectly, through ^1.0 of laravel/nexmo-notification-channel, so releasing v2 will not solve this problem.

Any chance there's a fix for this in v1?

dragonmantank commented 5 years ago

It's something we need to play around with, because swapping out this is a potentially BC-breaking change for existing installs, which under semver would be a new version. We are actively looking into solving this (and working with the nexmo-notification-channel plugin) as seamlessly for everyone as possible.

dragonmantank commented 5 years ago

Hello everyone who has given feedback on this issue. We are just about finished with the new release, but one thing we need to make sure we handle is this specific issue. We've done some testing and we think it's handled, but we are not sure 100% about projects in the field.

The current plan of creating a metapackage to allow users to have a version with guzzle and a core version without so far is working great in testing.

We do know that stock Laravel and this package seem to be fine, so we are looking to track down what other "normal/common" Laravel package might be bringing in a conflict. If possible, could anyone that has encountered this issue share their composer.json file? I would love to real confirmation that this is handled properly.

Thank you!

gbuckingham89 commented 5 years ago

Hi @dragonmantank,

Thanks for taking the time to look at this.

I've been unable to install this in a Laravel 5.8 project (neither directly nor as a dependancy of the laravel/nexmo-notification-channel package) due to issues with httplug and guzzle.

I've attached a copy of the composer require errors and my composer.json file. Hopefully these help with your testing.

dragonmantank commented 5 years ago

Thank you @gbuckingham89 !

This helped be determine that the conflict seems to actually be between us and sentry/sentry-laravel. Once I removed that package, I could require our regular packages. This does not help you in this case though, as I'm going to guess you are installing that package because you need it :P

Switching to our new 2.0.0 test package without guzzle let it install properly:

○ → composer require nexmo/client-core                                                                                                                                                       Using version ^2.0 for nexmo/client-core
./composer.json has been updated
Loading composer repositories with package information
Updating dependencies (including require-dev)
Package operations: 1 install, 0 updates, 0 removals
  - Installing nexmo/client-core (2.0.0): Cloning d4d31ccbdc from cache

So we are doing good on that front. Even with sentry/sentry-laravel our new guzzle-less package installs fine.

We have in our deployment roadmap a PR prepped for laravel/nexmo-notification-channel which should switch it to this new nexmo/client-core package and alleviate the problem from a purely Laravel standpoint. I looked over the package and I don't see anything that would break by them upgrading to our new 2.0.0 package.

Again, thank you very much, this helped immensely! For anyone else, I am happy to test their composer.json files as well. The larger the sample size, the better.

andrei-dascalu commented 5 years ago

@dragonmantank where can this "nexmo/client-core" package be found?

ALso I wonder if this issue can be revisited? At the moment I have a project that would need interfacing with nexmo but as it's tied to php-http/guzzle6-adapter (v1.1.1), it's impossible as we have another dependency requiring php-http/guzzle6-adapter (v2.0.1) thus causing a conflict and nexmo can't be installed.

Is there a way to force nexmo to use Diactoros ?

dragonmantank commented 5 years ago

@andrei-dascalu It's actually going to be this repo once we do some package name flip-flopping this week (we're down to one final test we need to work out). Essentially this repo will no longer require guzzle, but we will provide a wrapper nexmo/client that still provides guzzle out-of-the-box. This will be a drop-in replacement for users.

In your case, the idea would be you switch to nexmo/client-core, and provide your own Http\Client\HttpClient implementation when making a \Nexmo\Client object. If you want, you can look at the v2-alpha branch (CAUTION: consider it alpha until we merge it in) and see if it helps.

dragonmantank commented 5 years ago

Version 2.0.0 has been released, which should take care of this issue!

This repo is now named nexmo/client-core in packagist, so if you are having Guzzle issues change your composer.json to:

"nexmo/client-core": "^2.0"

This will switch to using the Guzzle-less version of our SDK, and you can pass in an HTTPlug-compatible adapter as the third parameter for a Nexmo\Client:

$httpAdapter = new Symfony\Component\HttpClient\HttplugClient();
$client = new Nexmo\Client($authObject, $options, $httpAdapter);

I'll leave this issue open for a bit in case anyone has any problems, but crosses fingers I think we finally got this fixed!

dragonmantank commented 4 years ago

Finally closing this issue out. I think it's been solved to the best of our abilities, and we've been talking about expanding this further in v3, among other changes.