Implement httplug #487
Implement httplug #487
Conversation
| @@ -26,6 +30,7 @@ | |||
| "geoip2/geoip2": "If you are going to use the GeoIP2DatabaseProvider.", | |||
| "symfony/stopwatch": "If you want to use the TimedGeocoder" | |||
| }, | |||
| "minimum-stability": "beta", | |||
There was a problem hiding this comment.
It can be removed, httplug is now in a stable version :)
There was a problem hiding this comment.
Sadly no, because the discovery depends on puli which is in beta state currently 😢
|
You should ignore the |
| { | ||
| parent::__construct(); | ||
|
|
||
| $this->adapter = $adapter; | ||
| $this->client = $client ?: HttpClientDiscovery::find(); | ||
| $this->factory = $factory ?: MessageFactoryDiscovery::find(); |
There was a problem hiding this comment.
One of the way we have done this for not having discovery, is to put discovery in suggest package then on the constructor we check the existence of the DiscoveryClass, if it has not been found we throw a \LogicException with a specific error which will be in our documentation for easy googling on that, here is an example :
if (null === $responseFactory && !class_exists('\Http\Discovery\MessageFactoryDiscovery')) {
throw new \LogicException('No response factory provided and no discovery service is present to guess it, maybe you need to install php-http/discovery package?');
}There was a problem hiding this comment.
I'm not sure about the discovery service at all, it brings many problems and puli as new dependency. Maybe we should wait for a stable release of the discovery service.
|
Also |
|
It seems puli have a problem with hhvm and php7, because travis passes for php 5.5, 5.6 but not for php7 and hhvm. |
|
Some notes: Currently the tests don't passes on PHP7 and HHVM because puli is sadly not compatible with that. |
| "php-http/mock-client": "^0.2", | ||
| "php-http/message": "^1.0", | ||
| "php-http/guzzle6-adapter": "^1.0", | ||
| "php-http/discovery": "^0.7.0", |
There was a problem hiding this comment.
Use "php-http/discovery": "^0.8.0" and put that in require.
|
any updates ? |
|
If there is interest from the maintainers to merge this, I will be happy to complete this PR. |
|
Yes, this should be merged into the lib at some point, especially because the current http lib is deprecated now. cc @egeloen |
|
Great. I'll try to get some work this weekend and next week. @Baachi I hope you do not mind that I continue your work. |
|
Feel free to continue the work. |
|
See #512 |
I started to integrate HTTPlug into geocoder.
Current status: Its integrated but i'm not about some implentation details. For example i don't know if we really need the
php-http/discoverypackage because itintroduce a lot of packages for a small benefit.
Reviews and thoughts are welcome!
Fixes #484 and #480