add profile client #136

fbourigault · 2017-04-01T21:57:37Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	Partially #108, Partially #93, Partially #135
Documentation	N/A
License	MIT

What's in this PR?

This add tracking of request target url and method. I choose to decorate the Client at the lowest possible level (i.e not the PluginClient, but the inner one) to be able to catch what goes in and out the Client and avoid false informations because of the registered plugins.

Technically, when the profiling is enabled, each client factory is decorated with a ProfileClientFactory which returns the original client decorated with a ProfileClient.

This is what I suggested for #135

Why?

Those new collected informations are required to write a better profiler.

To Do

Unit tests
Update changelog

Nyholm

Thank you for this PR!

Nyholm · 2017-04-02T06:26:11Z

Collector/ProfileClient.php

+     */
+    public function __construct($client, Collector $collector)
+    {
+        if ($client instanceof HttpAsyncClient && $client instanceof HttpClient) {


You can remove all this logic by using the FlexibleHttpClient

Should I use it directly without testing anything or should I use a condition to juste use FlexibleHttpClient when only one of both interface is implemented?

I moved this logic to the ProfileClientFactory and now FlexibleHttpClient is used.

Nyholm · 2017-04-02T06:27:33Z

Collector/ProfileClientFactory.php

+    public function __construct($factory, Collector $collector)
+    {
+        if (!$factory instanceof ClientFactory && !is_callable($factory)) {
+            throw new \RuntimeException(sprintf('Second argument to PluginClientFactory::createPluginClient must be a "%s" or a callable.', ClientFactory::class));


First argument...

Nyholm · 2017-04-02T06:28:51Z

Collector/ProfileClient.php

+use Http\Client\HttpClient;
+use Psr\Http\Message\RequestInterface;
+
+class ProfileClient implements HttpClient, HttpAsyncClient


Maybe a class comment that explains the purpose of this new client.

fbourigault · 2017-04-02T14:46:59Z

I added the ProfileClientTest.

fbourigault · 2017-04-02T14:49:39Z

I would do extensive tests about my code but how can I create a client using a callable instead of a factory with the bundle?

dbu

great, thanks! i only added some comments on phpdoc and code clarity.

re callable: maybe this could help? http://stackoverflow.com/questions/29792862/how-can-i-inject-callable-into-symfon2-dependency-injection-container-service - i assume you can do the same but configure a class as factory class.

that said, not sure if you need a callable - you can also set a service to a fixture class or something, if you need that instead.

dbu · 2017-04-03T06:55:31Z

Collector/ProfileClient.php

+     */
+    public function __construct($client, Collector $collector)
+    {
+        if (!$client instanceof HttpClient || !$client instanceof HttpAsyncClient) {


i would write this as if (!($client instanceof HttpClient && $client instanceof HttpAsyncClient)) { to make it more explicit. (just rewriting the expression, the logic is the same)

dbu · 2017-04-03T06:56:14Z

Collector/ProfileClient.php

+    private $collector;
+
+    /**
+     * @param HttpClient|HttpAsyncClient $client


this indicates that the client can be either, which is not correct. it needs to implement both. no idea if there even is a syntax for that, but the description should say that.

dbu · 2017-04-03T06:56:57Z

Collector/ProfileClient.php

+use Psr\Http\Message\RequestInterface;
+
+/**
+ * The ProfileClient decorates any HttpClient (or HttpAsyncClient) to gather target url and response status code.


description is not accurate, it decorates a FlexibleClient or something else implementing both interfaces

dbu · 2017-04-03T07:16:47Z

Collector/ProfileClientFactory.php

+    {
+        $client = is_callable($this->factory) ? $this->factory($config) : $this->factory->createClient($config);
+
+        if (!$client instanceof HttpClient || !$client instanceof HttpAsyncClient) {


same remark as above, i think !(instanceof && instanceof) would be easier to read

dbu · 2017-04-03T07:17:49Z

Tests/Unit/Collector/ProfileClientTest.php

+    }
+}
+
+interface ClientInterface extends HttpClient, HttpAsyncClient


or just mock flexible client? not sure whats better.

FlexibleHttpClient is final!

does that prevent from doing $this->createMock(FlexiblehttpClient::class) ?

argh, thats quite stupid. ok then this is fine.

That's the drawback of the final keyword. But this case is special as we have to Mock both HttpClient and HttpAsyncClient interface.

Should this interface moved in an other package (in the tests section)?

EDIT: having this interface here makes it not autoloadable which is good!

yeah i think its right to have it here. its not something that anybody should rely on.

fbourigault · 2017-04-03T12:47:13Z

From my POV, this is ready :)

Nyholm

Great. Im happy with this PR.

Good work and good review by David!

greg0ire · 2017-04-03T13:24:54Z

Collector/ProfileClient.php

+    private $collector;
+
+    /**
+     * @param HttpClient|HttpAsyncClient $client    The client to profile. Client must implement both HttpClient and


How about introducing a new interface extending both interfaces and type hinting against that?

That may be convenient but it does not comply with the Interface Segregation principle.

I know we talked about this before and decided not to, but I cannot find that thread atm.

Found it on Slack: https://php-http.slack.com/archives/C0D2U9SR3/p1487721978001035

(register here: http://slack.httplug.io/)

The ISP states not to make big interfaces, so that people don't need to implement all of the methods inside. In your case, they still can choose to implement either inherited interface, so ISP wouldn't be violated here IMO. And if you do need both methods, it looks like the best bay to express that need.

To be honest, I don't understand what inversion of control has to do with this, but I trust you have your (maybe fairly complicated) reasons. Plus, I wasn't aware there were other capabilities, and if an interface is introduced for every combination, this could get quite messy… the real thing to fix here might be php itself , to add the possibility to type hint against several interfaces :P

More about this here

Nyholm self-requested a review April 1, 2017 21:57

Nyholm requested changes Apr 2, 2017

View reviewed changes

dbu reviewed Apr 3, 2017

View reviewed changes

add profile client

511380d

Nyholm approved these changes Apr 3, 2017

View reviewed changes

Nyholm merged commit 6c8dc49 into php-http:master Apr 3, 2017

fbourigault deleted the profile-client branch April 3, 2017 12:59

greg0ire reviewed Apr 3, 2017

View reviewed changes

fbourigault mentioned this pull request Apr 19, 2017

Fix ProfileClient creation with callable factory #152

Merged

add profile client #136

add profile client #136

Uh oh!

Conversation

fbourigault commented Apr 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in this PR?

Why?

To Do

Uh oh!

Nyholm left a comment

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!

fbourigault commented Apr 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fbourigault commented Apr 2, 2017

Uh oh!

dbu left a comment

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!

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!

fbourigault Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbourigault commented Apr 3, 2017

Uh oh!

Nyholm left a comment

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!

greg0ire Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fbourigault commented Apr 1, 2017 •

edited

Loading

fbourigault commented Apr 2, 2017 •

edited

Loading

fbourigault Apr 3, 2017 •

edited

Loading

greg0ire Apr 3, 2017 •

edited

Loading

greg0ire Apr 3, 2017 •

edited

Loading