Add a discovery strategy

[Nyholm]

The clients found by discovery is not logged by the dev toolbar. This PR will add a new strategy that make sure we find a configured client.

Problems

We need to make sure constructor of ConfiguredClientsStrategy is executed. Because everything in the dependency injection is cached we can rely on that.
A hack that will work is to register httplug.strategy as a event listener on kernel.request and console.command... Not sure if Im happy with that. Any input is welcome.

This PR is blocked by

• Bugfixes discovery#71
• Allow for a candidate to have a Closure as a class parameter discovery#66

[dbu]

should we put this in a sub-namespace "Discovery"?

[Nyholm]

Sure.

[dbu]

this makes sense to me. what happens if we have no client explicitly configured? then the HttplugBundle uses discovery itself - do we have a chicken-and-egg problem here?

this is not complete yet, you need to prepend the discovery strategy so that it actually does something, right? do you want to add a container tag for discovery strategies? not sure how you can make this executed in the generated container, probably need to add a call to the definition of the discovery service itself?

[Nyholm]

this makes sense to me. what happens if we have no client explicitly configured? then the HttplugBundle uses discovery itself - do we have a chicken-and-egg problem here?

I believe that is fine as long as we fetch the HttpClient before we add the new strategy. Otherwise we will get problems with the discovery cache.

this is not complete yet, you need to prepend the discovery strategy so that it actually does something, right?

Correct.

do you want to add a container tag for discovery strategies? not sure how you can make this executed in the generated container, probably need to add a call to the definition of the discovery service itself?

Im not sure that will work. The problem is that there is not discovery service. It is a class with static properties that will forget their configuration at each request. We cant really use the dependency injection as you normally would. (As far as I know).

Feel free to make changes and try an idea if you got any. But remember to test the code with a prebuilt container as well.

[Nyholm]

This is ready for review and merge. A question that I want you to consider: Are we okey with the new strategy being injected on request and commands like this?

[dbu]

i think this makes sense, yes. thanks tobias!

do we need to document this somewhere? like saying in the bundle doc that the configured client will also be found by discovery?

[Nyholm]

I would say that would be pretty standard feature. I as a user do not care if it the client is auto-discovered or not.. What we should mention in the docs though is now we can log all HTTP messages sent by your application. The end developer does not need do to anything special but making sure their third-party libraries support HTTPlug.

".... So if you want a nice web debug toolbar integration for all request .... please send PRs to third party lib to use HTTPlug".

[Nyholm]

Thank you for merging BTW!

[sagikazarmark]

To be honest, I am not sure about this: what if I have two separate client configured with separate authentication details, plugins, etc. What happens then if I call discovery somewhere in the middle of the code? Will it return one of the configured clients?

[dbu]

as i understood, it will use the "default" client. and i think the doc should mention this. its not that obvious to me - auto discovery is not a feature of the HttplugBundle, so being explicit about it could avoid confusion. (but mention to prefer explicit injection if possible)

[sagikazarmark]

okay, but let's say the default client in a micro-application is configured to use authentication and there is a base URL too. If I use the discovery in any library, I will get this client instead of a "pure" unconfigured one and that could cause problems. I would say people should use the bundle and the container to get a client and then it will be visible in the profiler. This way any other code (external library, etc) which uses the discovery could receive a non-functional client and could lead to a case which is quite hard to debug.

[sagikazarmark]

Can we make this configurable? Discovery itself is already magic, and I am a little bit uncomfortable with too much magic. Personally I don't miss a non-configured client from the profiler, if any third party code uses the discovery. That's rather wrong configuration. But if this is always turned on then the scenario outlined above could cause many headaches.

[Nyholm]

You are correct @sagikazarmark. I did not think of that. This new Discovery is using the "default" client, ie the first configured client. That is not the same client as you would get if you use the CommonClassesStrategy.

The purpose of this PR is to enable the debug toolbar for auto discovered clients. It is not the purpose to configure clients for auto discovery.

[sagikazarmark]

It is not the purpose to configure clients for auto discovery.

But this is what actually happens, isn't it? It registers the default client (discovered or manually configured) in the discovery.

[dbu]

could we emulate normal discovery and then wrap the debug client around it? or otherwise have a configuration option to explicitly activate the feature, at which point the user is aware what client they are using. we could then even allow to specify which client to use...

[Nyholm]

But this is what actually happens, isn't it? It registers the default client (discovered or manually configured) in the discovery.

Yes, I'll make sure to update

[sagikazarmark]

I think we can handle this in the strategy internally: when discovery starts set a static flag to true then call the discovery again. If the flag is already true, do not return candidates and allow the discovery to resolve a candidate, then wrap the client, clear the cache (?) and set the flag to false. Dunno if this could work.

[sagikazarmark]

So what about this one? I would love to tag a new release after #84, but this one is a blocker IMO.

[Nyholm]

See https://github.com/php-http/HttplugBundle/tree/dev-conf-discovery

Im currently working on it.