Make sure we support any psr-18 client #295

Nyholm · 2018-12-29T15:18:57Z

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

Nyholm · 2018-12-29T15:29:11Z

This is just minor improvements. It works even without this patch

Collector/PluginClientFactory.php

Collector/ProfileClient.php

gmponos · 2018-12-29T21:42:40Z

Collector/ProfileClientFactory.php

@@ -6,6 +6,7 @@
 use Http\Client\HttpAsyncClient;
 use Http\Client\HttpClient;
 use Http\HttplugBundle\ClientFactory\ClientFactory;
+use Psr\Http\Client\ClientInterface;


I have checked out at your branch. I run composer update but still this namespace is not discovered by PHPStorm.. and the package is not installed.

Could it be that exists localy on you because you have not run composer update?

You need to make sure you install php-http/client-common:2.0-dev. If not you will get httplug: 1.1

Oh, you are correct.

Nyholm · 2018-12-30T07:02:16Z

Oh, you are correct. Since we have a bunch if httplug1.0 clients in require-dev we will never be able to install httplug2.0 by just running composer update.

I tested this by

composer create-project symfony/skeleton acme
cd acme
composer config minimum-stability dev
composer req php-http/httplug-bundle:dev-master nyholm/psr7

I created my client like:

namespace App\Service;

use Nyholm\Psr7\Response;
use Psr\Http\Client\ClientInterface;
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\ResponseInterface;

class MyHttpClient implements ClientInterface
{
    public function sendRequest(RequestInterface $request): ResponseInterface
    {
        return new Response(250);
    }
}

And I registered it like:

httplug:
    main_alias:
        client: httplug.client.my_client
    discovery:
        client: 'auto'

    clients:
        my_client:
            service: 'App\Service\MyHttpClient'

            http_methods_client: true
            plugins:
                - 'httplug.plugin.content_length'
                - 'httplug.plugin.redirect'

gmponos · 2018-12-30T09:32:02Z

Current PR brings up an issue... If you keep supporting both versions of HTTP-Client you can not depend on the PSR. Is this the direction you want this to go?

True. But we are not depending on the psr. Are we?

Well.. on the current a PR you want to do this if( $client instanceof ClientInterface) although this code works.. even if I don't have version 2 installed it kinda feels weird...

Secondly as a developer of this package... yes I do not depend on PSR client and I might not want to depend... As a consumer using this package.. is there a way that I feel confident that the client I am retrieving is a PSR one? Haven't discovered all the full aspects of the bundle so on this point I might be wrong... I think there is some autodiscovery mechanism.. that's what I am talking about.

Thirdly... here wont this miss the exceptions?

gmponos · 2018-12-30T09:43:28Z

@Nyholm I think that your example works out of luck.. although I know that this was by design it just happens that PSR client and PHP-HTTP to have the same signature sendRequest..

for instance the ProfileClient I think it will work even a custom client of mine that does not implement any interface and it just has the sendRequest.. it feels kinda loose in this way...

Nyholm · 2018-12-30T10:03:55Z

Well.. on the current a PR you want to do this if( $client instanceof ClientInterface) although this code works.. even if I don't have version 2 installed it kinda feels weird...

Yes, I agree. But it works =)

Secondly as a developer of this package... yes I do not depend on PSR client and I might not want to depend... As a consumer using this package.. is there a way that I feel confident that the client I am retrieving is a PSR one? Haven't discovered all the full aspects of the bundle so on this point I might be wrong... I think there is some autodiscovery mechanism.. that's what I am talking about.

The auto discovery is here: https://github.com/php-http/discovery

And no, you cannot be sure unless you decide what you want. If you do composer req php-http/httplug:^2.0, then you will always get a PSR18 client. Same if you do composer req psr/http-client

Thirdly... here wont this miss the exceptions?

That should be updated. Thank you!

although I know that this was by design it just happens that PSR client and PHP-HTTP to have the same signature sendRequest..
for instance the ProfileClient I think it will work even a custom client of mine that does not implement any interface and it just has the sendRequest.. it feels kinda loose in this way...

Technically, yes. But we have code that make sure you submit a client. Check out the ProfileClient and the FlexibleClient

gmponos · 2018-12-30T10:14:58Z

for instance the ProfileClient I think it will work even a custom client

But we have code that make sure you submit a client.

OK.. I missed the ! there...

I will have a deeper look because then I don't quite fully get it how your code example posted above worked with master :/

composer req php-http/httplug-bundle:dev-master nyholm/psr7

dbu

not sure i understand the discussion on the PR. to me this looks correct. we allow either interface, and PHP will not explode if it does not know about a namespace, e.g. the PSR interfaces, in the case of running with httplug 1.x.

Collector/Formatter.php

dbu · 2018-12-30T12:10:05Z

Oh, you are correct. Since we have a bunch if httplug1.0 clients in require-dev we will never be able to install httplug2.0 by just running composer update.

is this issue fixed with #294? can you rebase and maybe add some tests that make sure both httplug 1 (when installed) and a psr-18 client (when installed) work with this code?

Nyholm · 2018-12-30T12:10:46Z

Correct.
Sure!

gmponos · 2018-12-30T12:23:28Z

not sure i understand the discussion on the PR.

Yea.. I understand that I did not express my self correct on this.. also I was lost too because I thought that PHP will explode if it does not find a namespace which it doesn't... tests will be helpful to..

dbu · 2018-12-30T12:28:02Z

Yea.. I understand that I did not express my self correct on this..

no worries, better speak up than silently be confused ;-) and i think it was very helpful here, because in the beginning we had no test covering the changes, which is dangerous.

and the fact that php ignores unknown namespaces in instanceof is ... "interesting".

dbu

looks good to merge to me.

lets give @gmponos time to review as well.

gmponos

According to the scope of the changes they are fine.. on an unrelated topic I believe that bundle must drop support for PHP 5 and HttpClient interface and bump to a new version.

Nyholm · 2018-12-30T19:53:55Z

Why do you believe that?

Nyholm · 2018-12-30T20:03:54Z

Continue in #296

gmponos reviewed Dec 29, 2018

View reviewed changes

Collector/PluginClientFactory.php Outdated Show resolved Hide resolved

Nyholm commented Dec 29, 2018

View reviewed changes

Collector/ProfileClient.php Outdated Show resolved Hide resolved

gmponos reviewed Dec 29, 2018

View reviewed changes

gmponos mentioned this pull request Dec 30, 2018

support client-common 2.0 #291

Merged

1 task

dbu approved these changes Dec 30, 2018

View reviewed changes

Collector/Formatter.php Outdated Show resolved Hide resolved

Nyholm added 4 commits December 30, 2018 13:11

Make sure we support any psr-18 client

8bc38f9

wring use statement

5e93b35

Updated exceptions

d0775d3

use use statements

ac72580

Nyholm force-pushed the patch-276 branch from 15e199f to ac72580 Compare December 30, 2018 12:12

Added tests with a PSR18 client

04f1ec3

dbu approved these changes Dec 30, 2018

View reviewed changes

gmponos approved these changes Dec 30, 2018

View reviewed changes

Nyholm merged commit 853b2c4 into master Dec 30, 2018

Nyholm mentioned this pull request Dec 30, 2018

Release 1.14 #296

Closed

Nyholm deleted the patch-276 branch December 31, 2018 07:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure we support any psr-18 client #295

Make sure we support any psr-18 client #295

Nyholm commented Dec 29, 2018

Nyholm commented Dec 29, 2018

gmponos Dec 29, 2018

Nyholm Dec 30, 2018

Nyholm Dec 30, 2018

Nyholm commented Dec 30, 2018 •

edited

Loading

gmponos commented Dec 30, 2018 •

edited

Loading

gmponos commented Dec 30, 2018

Nyholm commented Dec 30, 2018

gmponos commented Dec 30, 2018 •

edited

Loading

dbu left a comment

dbu commented Dec 30, 2018 •

edited

Loading

Nyholm commented Dec 30, 2018

gmponos commented Dec 30, 2018

dbu commented Dec 30, 2018

dbu left a comment

gmponos left a comment

Nyholm commented Dec 30, 2018

Nyholm commented Dec 30, 2018

Make sure we support any psr-18 client #295

Make sure we support any psr-18 client #295

Conversation

Nyholm commented Dec 29, 2018

Nyholm commented Dec 29, 2018

gmponos Dec 29, 2018

Choose a reason for hiding this comment

Nyholm Dec 30, 2018

Choose a reason for hiding this comment

Nyholm Dec 30, 2018

Choose a reason for hiding this comment

Nyholm commented Dec 30, 2018 • edited Loading

gmponos commented Dec 30, 2018 • edited Loading

gmponos commented Dec 30, 2018

Nyholm commented Dec 30, 2018

gmponos commented Dec 30, 2018 • edited Loading

dbu left a comment

Choose a reason for hiding this comment

dbu commented Dec 30, 2018 • edited Loading

Nyholm commented Dec 30, 2018

gmponos commented Dec 30, 2018

dbu commented Dec 30, 2018

dbu left a comment

Choose a reason for hiding this comment

gmponos left a comment

Choose a reason for hiding this comment

Nyholm commented Dec 30, 2018

Nyholm commented Dec 30, 2018

Nyholm commented Dec 30, 2018 •

edited

Loading

gmponos commented Dec 30, 2018 •

edited

Loading

gmponos commented Dec 30, 2018 •

edited

Loading

dbu commented Dec 30, 2018 •

edited

Loading