Skip to content

Conversation

@Nyholm
Copy link
Member

@Nyholm Nyholm commented Dec 28, 2018

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets
Documentation
License MIT

What's in this PR?

We should show that we support PSR-18. I've replaced Http\Client\HttpClient with Psr\Http\Client\ClientInterface on all the places where we consume a client.

This will fix #122

Copy link
Member

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

Sounds reasonable

@Nyholm
Copy link
Member Author

Nyholm commented Dec 29, 2018

Thank you for the review

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

very nice. great that the psr turned out to be compatible 👍

{
if (!($client instanceof HttpClient) && !($client instanceof HttpAsyncClient)) {
if (!($client instanceof ClientInterface) && !($client instanceof HttpAsyncClient)) {
throw new \LogicException('Client must be an instance of Http\\Client\\HttpClient or Http\\Client\\HttpAsyncClient');
Copy link
Contributor

Choose a reason for hiding this comment

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

we should adjust the exception message too

$request = $request->withUri($request->getUri()
$request = $request->withUri(
$request->getUri()
->withPath($this->uri->getPath().$request->getUri()->getPath())
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure that this is more readable than the previous formatting. should we indent this some more? or extract the argument on this line to a variable so we can put $request->getUri()->withPath($prefixedPath) on one line?

Copy link
Member Author

Choose a reason for hiding this comment

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

this change must have been suggested/automatic.

I like your suggestion though.

@Nyholm
Copy link
Member Author

Nyholm commented Dec 29, 2018

This will fix #122

@Nyholm Nyholm added this to the v2.0.0 milestone Dec 29, 2018
@dbu dbu merged commit 53bddb2 into 2.x Dec 29, 2018
@dbu dbu deleted the 2.x-psr18 branch December 29, 2018 08:52
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