Skip to content

Implement httplug - #487

Closed
Baachi wants to merge 6 commits into
geocoder-php:masterfrom
Baachi:feature/httplug
Closed

Baachi wants to merge 6 commits into
geocoder-php:masterfrom
Baachi:feature/httplug

Conversation

@Baachi

@Baachi Baachi commented Jan 18, 2016

Copy link
Copy Markdown
Member

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/discovery package because it
introduce a lot of packages for a small benefit.

Reviews and thoughts are welcome!

Fixes #484 and #480

Comment thread composer.json Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be removed, httplug is now in a stable version :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly no, because the discovery depends on puli which is in beta state currently 😢

@joelwurtz

Copy link
Copy Markdown

You should ignore the .puli directory

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?');
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@joelwurtz

Copy link
Copy Markdown

Also CachedResponseClient can maybe be remove and use the PluginClient from php-http which provides a CachePlugin working with a PSR6 implementation.

@Baachi

Baachi commented Feb 1, 2016

Copy link
Copy Markdown
Member Author

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.

@Baachi

Baachi commented Feb 22, 2016

Copy link
Copy Markdown
Member Author

Some notes: Currently the tests don't passes on PHP7 and HHVM because puli is sadly not compatible with that.
Also we must bump the requirement to php 5.5 because httplug needs at least php 5.5.

@Baachi Baachi changed the title [WIP] Implement httplug Implement httplug Feb 24, 2016
Comment thread composer.json
"php-http/mock-client": "^0.2",
"php-http/message": "^1.0",
"php-http/guzzle6-adapter": "^1.0",
"php-http/discovery": "^0.7.0",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use "php-http/discovery": "^0.8.0" and put that in require.

@smartpierre

Copy link
Copy Markdown

any updates ?

@Nyholm

Nyholm commented Jun 9, 2016

Copy link
Copy Markdown
Member

If there is interest from the maintainers to merge this, I will be happy to complete this PR.

@willdurand

Copy link
Copy Markdown
Member

Yes, this should be merged into the lib at some point, especially because the current http lib is deprecated now.

cc @egeloen

@Nyholm

Nyholm commented Jun 9, 2016

Copy link
Copy Markdown
Member

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.

@Baachi

Baachi commented Jun 10, 2016

Copy link
Copy Markdown
Member Author

Feel free to continue the work.

@Nyholm Nyholm mentioned this pull request Jun 12, 2016
@willdurand

Copy link
Copy Markdown
Member

See #512

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.

5 participants