Discovery bugfixes and more flexible. #87

Nyholm · 2016-07-16T00:01:53Z

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

What's in this PR?

With this PR I wanted to achieve two things:

A way to configure which client that should be used with auto discovery
A way to opt-out of providing a auto discovery client in the toolbar.

sagikazarmark · 2016-07-16T22:07:50Z

DependencyInjection/Configuration.php

@@ -83,6 +83,8 @@ public function getConfigTreeBuilder()
                            ->defaultValue('auto')
                        ->end()
                        ->scalarNode('formatter')->defaultNull()->end()
+                        ->booleanNode('inject_debugger_for_discovered_http_client')->defaultTrue()->end()


I am not sure if we want this opt-in by default.

Why not? All the other clients are opt-in by default.

Correct me if I am wrong, but this will register the default client in the discovery, will it not? If so, all the problems I listed in the previous PR can still occur. I would rather have this as an optional feature stating you can turn this on for discovered clients if you want.

Can we find a shorter name? Like profile_discovered_client and profile_discovered_async_client

Correct me if I am wrong, but this will register the default client in the discovery, will it not?

No, It will find take a (new) unconfigured client and add toolbar debugging on that and then make sure that this client is found by discovery in the future.

This PR also lets to you decide if you want to use a configured client as auto discovery.

Can we find a shorter name? Like profile_discovered_client and profile_discovered_async_client

Yes please. It was quite late last night when I wrote this. =)

Okay, so by default a discovered, unconfigured client is simply wrapped with debug tools and put back to discovery and you can also choose to use a configured client for that, but that is optional, opt-out by default?

dbu · 2016-07-18T07:33:41Z

you might need to cherry-pick commits into a new branch as #84 is now squashed. there is a conflict reported by github.

Nyholm · 2016-07-18T07:35:33Z

Yes. I'll make this PR ready for review shortly.

…uld be used.

Nyholm · 2016-07-18T08:43:30Z

There. This PR is ready for review.

Consider the following config

httplug:
    toolbar:
      profile_discovered_client: true # Default
      profile_discovered_async_client: false # Default
    classes:
    clients:
        my_guzzle6:
            factory: 'httplug.factory.guzzle6'
        acme:
            factory: 'httplug.factory.curl'

This will create a two new private services: httplug.auto_discovery_client.pure and httplug.auto_discovery_client.plugin. The underlying client is a new client found by auto discovery. Next time a third party library is using auto discovery it will find the httplug.auto_discovery_client.plugin which is configured with debug stuff.

If a user want to control what client is found by auto discovery he/she may add use_with_discovery to the client.

httplug:
    toolbar:
      profile_discovered_client: false # Does not matter when 'use_with_discovery'
      profile_discovered_async_client: false # Default
    classes:
    clients:
        my_guzzle6:
            factory: 'httplug.factory.guzzle6'
        acme:
            factory: 'httplug.factory.curl'
            use_with_discovery: 'http_client'

sagikazarmark · 2016-07-18T09:21:58Z

use_with_discovery: 'http_client'

What does this mean? Neither the key nor the value is obvious IMO.

Nyholm · 2016-07-18T09:24:55Z

You can choose what client to be found by HttpClientDiscovery::find and HttpAsyncDiscovery::find.

I added a comment about it here: https://github.com/php-http/HttplugBundle/pull/87/files#diff-850942b3ba24ab03a40aaa81b6152852R129

But maybe that was not the best one

sagikazarmark · 2016-07-18T09:28:26Z

What if someone wants one client to be used as both? Is there a type check to make sure the chosen client is actually of the relevant type? What if this is configured for multiple clients?

Nyholm · 2016-07-18T09:31:02Z

What if someone wants one client to be used as both?

Correct. That was missing

Is there a type check to make sure the chosen client is actually of the relevant type?

Yes, In the discovery strategies contstuctor

What if this is configured for multiple clients?

Then an exception will be thrown

sagikazarmark · 2016-07-18T09:32:56Z

Then maybe we could have the following configuration:

httplug:
    discovery:
        client: client_id # optional
        async_client: async_client_id # optional
        profile_client: true
        profile_async_client: false

Nyholm · 2016-07-18T09:36:41Z

I like it!
Will do

Nyholm · 2016-07-18T09:44:48Z

Since profile_client as no effect when using httplug.discovery.client I suggest:

httplug:
    discovery:
        client: 'auto' # could also be a service name like 'httplug.client.acme'
        async_client: 'none' # default

But this does not however tell you about the profiling... Is there a better config?

sagikazarmark · 2016-07-18T09:46:19Z

When there is a concrete client configured, profiling is enabled for it automatically?

Nyholm · 2016-07-18T09:46:27Z

Yes

sagikazarmark · 2016-07-18T09:47:50Z

Then I am fine with that. Although probably we should have null (~) values by default and those should be mapped to auto and none. WDYT?

Nyholm · 2016-07-18T12:30:50Z

This PR is now ready for review

dbu · 2016-07-18T15:21:12Z

DependencyInjection/Configuration.php

+                            ->info('Set to "auto" to see auto discovered client in the web profiler. If provided a service id for a client then this client will be found by auto discovery.')
+                        ->end()
+                        ->scalarNode('async_client')
+                            ->defaultNull()


why not auto as well?

Because if no AsyncClient is found we get an exception. I do not think I can catch that.

okay. but then lets change the info to mention that you should set this to auto if you know that you have an async client available.

Or should I put that in the docs?

I added it in the docs:
https://github.com/php-http/documentation/pull/118/files#diff-33b9b1161d0ce83931e73c48bf9e6900R71

dbu · 2016-07-18T15:24:42Z

i have one question and one suggestion. but overall, this looks good, i like it. please also start with the documentation as soon as we are set on how this works, so that we don't forget the details of auto vs null and so on...

sagikazarmark · 2016-07-18T17:05:00Z

Can we merge this and tag v1.2?

sagikazarmark · 2016-07-18T20:26:06Z

Discovery/ConfiguredClientsStrategy.php

    }

    /**
     * {@inheritdoc}
     */
    public static function getCandidates($type)
    {
-        if (static::$client !== null && $type == HttpClient::class) {
+        if (static::$client !== null && $type === HttpClient::class) {


Should the type check rather come first? This way the first condition is evaluated even if the type is a MessageFactory.

sagikazarmark added the in progress label Jul 16, 2016

Nyholm changed the title ~~Dev conf discovery~~ Discovery bugfixes and more flexible. Jul 16, 2016

sagikazarmark changed the title ~~Discovery bugfixes and more flexible.~~ Discovery bugfixes and more flexible. Jul 16, 2016

sagikazarmark added needs review and removed in progress labels Jul 16, 2016

sagikazarmark modified the milestone: v1.2.0 Jul 16, 2016

Nyholm mentioned this pull request Jul 16, 2016

Update HttplugBundle with new configuration php-http/documentation#114

Closed

sagikazarmark reviewed Jul 16, 2016
View reviewed changes

Nyholm force-pushed the dev-conf-discovery branch 2 times, most recently from 201bcb6 to d5e9bcd Compare July 18, 2016 06:47

Nyholm force-pushed the dev-conf-discovery branch 2 times, most recently from d0f9be9 to c380db9 Compare July 18, 2016 08:34

Let user configure what do be found in discovery and if profiling sho…

d49f7d6

…uld be used.

Nyholm force-pushed the dev-conf-discovery branch from c380db9 to d49f7d6 Compare July 18, 2016 08:35

Nyholm added 2 commits July 18, 2016 10:45

Use strict comparison.

c1f0982

fixed tests

5c015a2

Nyholm added 3 commits July 18, 2016 14:17

Updated config

c8359e7

Bugfix

5509df4

Removed code not used

a665340

dbu reviewed Jul 18, 2016
View reviewed changes

Minor fix

27b6238

sagikazarmark reviewed Jul 18, 2016
View reviewed changes

Nyholm added 2 commits July 18, 2016 22:35

evalute class type first

5c1a5b9

Removed public="true"

cb37a48

sagikazarmark merged commit 1e806aa into master Jul 18, 2016

sagikazarmark deleted the dev-conf-discovery branch July 18, 2016 21:16

sagikazarmark removed the needs review label Jul 18, 2016

Discovery bugfixes and more flexible. #87

Discovery bugfixes and more flexible. #87

Uh oh!

Conversation

Nyholm commented Jul 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in this PR?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Jul 18, 2016

Uh oh!

Nyholm commented Jul 18, 2016

Uh oh!

Nyholm commented Jul 18, 2016

Uh oh!

sagikazarmark commented Jul 18, 2016

Uh oh!

Nyholm commented Jul 18, 2016

Uh oh!

sagikazarmark commented Jul 18, 2016

Uh oh!

Nyholm commented Jul 18, 2016

Uh oh!

sagikazarmark commented Jul 18, 2016

Uh oh!

Nyholm commented Jul 18, 2016

Uh oh!

Nyholm commented Jul 18, 2016

Uh oh!

sagikazarmark commented Jul 18, 2016

Uh oh!

Nyholm commented Jul 18, 2016

Uh oh!

sagikazarmark commented Jul 18, 2016

Uh oh!

Nyholm commented Jul 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Jul 18, 2016

Uh oh!

sagikazarmark commented Jul 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Nyholm commented Jul 16, 2016 •

edited

Loading