Add configuration option to disable registration of default client #314

ruudk · 2019-02-14T12:46:34Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	fixes #312
Documentation	if this is a new feature, link to pull request in https://github.com/php-http/documentation that adds relevant documentation
License	MIT

What's in this PR?

This allows you to disable the registration of the default HTTPlug client.

Why?

We are heavily using Symfony's auto wire functionality. Sometimes, developer inject HttpClient in the constructor without actually specifying the exact httplug.client.name. No error is thrown, and HTTPlug just picks the httplug.client.default service (or the first client it can find)

Checklist

Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
Documentation pull request created

dbu

thanks! i wonder if we should touch the default client or rather the alias with the client interface / class name (i think there are several, for psr-18 and http async and sync client). or we could remove both when the option is set.

CHANGELOG.md

dbu · 2019-02-19T07:45:38Z

DependencyInjection/HttplugExtension.php

@@ -87,6 +87,10 @@ public function load(array $configs, ContainerBuilder $container)
        $this->configureClients($container, $config);
        $this->configurePlugins($container, $config['plugins']); // must be after clients, as clients.X.plugins might use plugins as templates that will be removed
        $this->configureAutoDiscoveryClients($container, $config);
+
+        if (!$config['register_default_client']) {
+            $container->removeDefinition('httplug.client.default');


your goal is to prevent autowiring from happening, right? would it not be better to have an autowire_client flag and remove the alias with the client class name instead of the default service? with this, you would get an error that the alias is invalid, no?

I think the difference here is Symfony 3.4 and 4+.

With Symfony 3.4: when you have a default service, it will just auto wire when you use its class name.
I believe this changed in 4+ where you have to explicitly set the name of the service to the class name.

I faced this issue in a SF 3.4 application.

dbu · 2019-03-02T09:43:04Z

@ruudk wdyt about the questions i asked?

ruudk · 2019-03-02T09:47:59Z

Sorry for not responding. I will pick this up next week.

dbu · 2019-03-05T10:51:47Z

please also rebase on master when you work on this. we moved the php files into src/ and tests/

i had no conflicts for other open merge requests, but github seems to not want to automatically solve this.

ruudk · 2019-03-05T11:58:11Z

Rebased the PR and didn't apply much changes. See comment above.

dbu · 2019-03-05T12:14:04Z

looking at https://github.com/php-http/HttplugBundle/blob/master/src/Resources/config/services.xml#L44-L45 i am a bit confused. we set the default client to be loaded from discovery if there are no clients defined at all. and then we overwrite that with

HttplugBundle/src/DependencyInjection/HttplugExtension.php

Lines 114 to 123 in d053921

    
           if (null !== $first) { 
        
               // If we do not have a client named 'default' 
        
               if (!array_key_exists('default', $config['clients'])) { 
        
                   $serviceId = 'httplug.client.'.$first; 
        
                   // Alias the first client to httplug.client.default 
        
                   $container->setAlias('httplug.client.default', $serviceId); 
        
                   $default = $first; 
        
               } else { 
        
                   $default = 'default'; 
        
               }

if there are any clients configured. this seems odd.

so the configuration option would prevent both the factory service and aliasing the first client as the default client.

@Nyholm do you remember why we have the factory based client service as well? so that things work if you configure no client at all?

and the alias in https://github.com/php-http/HttplugBundle/blob/master/src/Resources/config/services.xml#L47 is actually on a client with default in the name. did not find where we define that service.

Nyholm · 2019-03-19T15:22:35Z

I like the feature but not the implementation (I think).

David: At runtime we configure profiling to the auto discovered client if we don't got any. It is a bit messy to say the least. Me and @fbourigault spend some long hours trying to figure stuff out.

@ruudk What about only removing the aliases and not the actual service?

ruudk · 2019-03-20T08:17:50Z

@Nyholm I updated the PR. It now only removes the aliases. Therefore, I think we might want to rename the configuration property to something like register_default_client_for_autowiring?

dbu · 2019-03-20T08:45:34Z

agreed, and i think register_default_client_for_autowiring is a good name. it would be canBeDisabled() so that users can set enabled: false if they don't want autowiring.

can you please also rebase your pull request on master? i think we fixed the lowest build there.

Nyholm · 2019-03-20T08:51:14Z

Awesome. This is a much simpler implementation 👍

joelwurtz · 2019-03-20T08:51:28Z

I prefer small value :), what about: default_client_autowiring ? (Or even default_autowiring)

Nyholm · 2019-03-20T08:52:38Z

client_interface_autowring?

dbu · 2019-03-20T08:52:42Z

i would not call it default_autowiring. we could do autowire maybe? i guess there is nothing else that is autowired from this bundle?

joelwurtz · 2019-03-20T08:54:17Z

If we want to use this value for disabling potential futur autowiring of other class, then i'm 👍 for autowiring only

ruudk · 2019-03-20T08:54:40Z

i would not call it default_autowiring. we could do autowire maybe? i guess there is nothing else that is autowired from this bundle?

but what if we add something else in the future that autowires? Then this configuration property becomes too generic and we're screwed.

ruudk · 2019-03-20T08:55:31Z

I really like default_client_autowiring because it's clear what it does.

joelwurtz · 2019-03-20T08:55:51Z

And even if we want to split we can do somehting like that in the futur

autowiring:
    defaut_client: false
    other: true

autowiring is the best choice IMO

ruudk · 2019-03-20T08:56:33Z

Oke, I will change it.

ruudk · 2019-03-20T09:00:39Z

Shouldn't we also remove these aliases then?

        <service id="Http\Message\MessageFactory" alias="httplug.message_factory" public="false" />
        <service id="Http\Message\RequestFactory" alias="httplug.message_factory" public="false" />
        <service id="Http\Message\ResponseFactory" alias="httplug.message_factory" public="false" />
        <service id="Http\Message\StreamFactory" alias="httplug.stream_factory" public="false" />
        <service id="Http\Message\UriFactory" alias="httplug.uri_factory" public="false" />

ruudk · 2019-03-20T09:02:36Z

Rebased PR and applied changes.

dbu · 2019-03-20T09:19:28Z

oh, so we do autowire other things. i feel that it makes no sense to disable autowiring the factories. and therefore i would want to call it default_client_autowiring. if we have more autowiring that we want to allow to disable, we can still move to the nested structure. @joelwurtz can you live with that? i think clean name is more important than short name. and a nested structure seems too much, given that we probably don't want to un-autowire other things in the future.

joelwurtz · 2019-03-20T13:27:31Z

I'm fine with that :) didn't know either there was autowiring for other classes sorry for the noise.

Thanks @ruudk for checking those things

Nyholm

Thank you

ruudk · 2019-03-20T14:12:44Z

Updated the PR one more time ;)

dbu

cool! minor nitpick for the changelog, and the lowest version tests break. i suggest skipping your test on php 5 to keep this sane.

tests/Functional/DiscoveryTest.php

CHANGELOG.md

dbu · 2019-03-20T16:49:00Z

one last thing i noticed: you add a git submodule documentation in this pull request. can you please remove that change? otherwise this looks ready to merge to me, thanks!

ruudk · 2019-03-20T17:30:17Z

Good catch. I accidentally staged that directory after all changing this PR 10 times ;) It's fixed now.

Nyholm

Thank you.

dbu · 2019-03-21T07:18:57Z

thanks a lot and sorry for the back and forth!

dbu · 2019-03-21T07:32:16Z

i documented this in php-http/documentation#260

ruudk force-pushed the disable_default_clients branch from 196d9b0 to 5d05ccf Compare February 14, 2019 13:00

dbu reviewed Feb 19, 2019

View reviewed changes

ruudk force-pushed the disable_default_clients branch from 5d05ccf to 8266434 Compare March 5, 2019 11:56

ruudk force-pushed the disable_default_clients branch from 8266434 to 2c85746 Compare March 5, 2019 13:45

ruudk force-pushed the disable_default_clients branch from 2c85746 to 96121b4 Compare March 20, 2019 08:17

ruudk force-pushed the disable_default_clients branch from 96121b4 to 3cfe74b Compare March 20, 2019 09:02

Nyholm approved these changes Mar 20, 2019

View reviewed changes

ruudk force-pushed the disable_default_clients branch from 3cfe74b to b54efdb Compare March 20, 2019 14:12

dbu approved these changes Mar 20, 2019

View reviewed changes

tests/Functional/DiscoveryTest.php Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

ruudk force-pushed the disable_default_clients branch 2 times, most recently from b35fcfc to 083a825 Compare March 20, 2019 16:39

Add default_client_autowiring configuration option

7fa5109

ruudk force-pushed the disable_default_clients branch from 083a825 to 7fa5109 Compare March 20, 2019 17:29

Nyholm approved these changes Mar 20, 2019

View reviewed changes

dbu merged commit bbe830d into php-http:master Mar 21, 2019

dbu mentioned this pull request Mar 21, 2019

document autowiring and disabling it php-http/documentation#260

Merged

ruudk deleted the disable_default_clients branch March 21, 2019 09:05

Add configuration option to disable registration of default client #314

Add configuration option to disable registration of default client #314

Uh oh!

Conversation

ruudk commented Feb 14, 2019

What's in this PR?

Why?

Checklist

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dbu Feb 19, 2019

Choose a reason for hiding this comment

Uh oh!

ruudk Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

dbu commented Mar 2, 2019

Uh oh!

ruudk commented Mar 2, 2019

Uh oh!

dbu commented Mar 5, 2019

Uh oh!

ruudk commented Mar 5, 2019

Uh oh!

dbu commented Mar 5, 2019

Uh oh!

Nyholm commented Mar 19, 2019

Uh oh!

ruudk commented Mar 20, 2019

Uh oh!

dbu commented Mar 20, 2019

Uh oh!

Nyholm commented Mar 20, 2019

Uh oh!

joelwurtz commented Mar 20, 2019

Uh oh!

Nyholm commented Mar 20, 2019

Uh oh!

dbu commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joelwurtz commented Mar 20, 2019

Uh oh!

ruudk commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ruudk commented Mar 20, 2019

Uh oh!

joelwurtz commented Mar 20, 2019

Uh oh!

ruudk commented Mar 20, 2019

Uh oh!

ruudk commented Mar 20, 2019

Uh oh!

ruudk commented Mar 20, 2019

Uh oh!

dbu commented Mar 20, 2019

Uh oh!

joelwurtz commented Mar 20, 2019

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

ruudk commented Mar 20, 2019

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dbu commented Mar 20, 2019

Uh oh!

ruudk commented Mar 20, 2019

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

dbu commented Mar 20, 2019 •

edited

Loading

ruudk commented Mar 20, 2019 •

edited

Loading