search

arkaitzgarro / elastic-apm-laravel

Laravel APM agent for Elastic v2 intake API
MIT License
79 stars 17 forks source link

Remove Guzzle #95

Closed veelasky closed 4 years ago

veelasky commented 4 years ago

Remove explicitly defined guzzle http client adapter.

And also bump laravel version to support version 8.

arkaitzgarro commented 4 years ago

Hi @veelasky, could you elaborate what’s the reasoning behind removing the Guzzle adapter?

veelasky commented 4 years ago

the underlying HttpClient required by nipwaayoni/elastic-apm-php-agent only states it need to comply with PSR17 and handled by this package php-http/discovery.

Because Guzzle has released version 7, it will be cumbersome if the underlying laravel application uses guzzle version 7, since the adapter for version 6 and 7 is different.

arkaitzgarro commented 4 years ago

Shouldn't be the solution to add php-http/guzzle7-adapter as well, instead of removing adapters? That way the discovery mechanism will pick up the right adapter based on the installed client. @dstepe does this make sense, or will it cause an issue having both adapters?

dstepe commented 4 years ago

I don't think they can both be included. The two Guzzle adapters each require the version of Guzzle they support:

guzzle6-adapter "guzzlehttp/guzzle": "^6.0" https://github.com/php-http/guzzle6-adapter/blob/master/composer.json#L21

guzzle7-adapter "guzzlehttp/guzzle": "^7.0" https://github.com/php-http/guzzle7-adapter/blob/master/composer.json#L17

That's going to create an immediate dependency conflict.

In general, I advocate that library packages should require the fewest dependencies possible, leaving as much choice to the consumer as they can.

arkaitzgarro commented 4 years ago

I see. @veelasky could you add a suggestion in Composer to install guzzle7-adapter? And I guess we also need to specify in the README that the final user is responsible to install the adapter.

dstepe commented 4 years ago

Laravel 7 introduced an HTTP client (https://laravel.com/docs/7.x/http-client) which is based on Guzzle, so the need to for the user of this package to require Guzzle should be limited to Laravel 6.x unless they want a specific version of Guzzle.

veelasky commented 4 years ago

done. 👍

thanks for the great package, can't wait for the major release.

arkaitzgarro commented 4 years ago

Guzzle references are still present in composer.lock file though. I guess we need to remove guzzlehttp/guzzle, guzzlehttp/promises and guzzlehttp/psr7 right?

veelasky commented 4 years ago

oops sorry, thought it was being ignored by .gitignore

dstepe commented 4 years ago

I believe that most library packages do not include the composer.lock file in source code management. Composer only uses that file when you install dependencies for the package project itself, as in when doing development on the package. It won't have any impact when installing the dependencies of a Laravel application, so any mention of Guzzle in the lock file should not have an impact on the Laravel app install.

That said, running a composer update with the adapter removed does remove the Guzzle package dependency from the lock file, which could then be committed. Of course, that updates a number of other things, but again, that only impacts people who are developing this package.

I would vote to remove composer.lock from the git repo.

arkaitzgarro commented 4 years ago

The main issue removing composer.lock file is that CI runs out of memory when resolving the dependencies. I wouldn't mind removing it, but circleci refuses to work.