Use flex client wrapper for profiling if needed #209
Conversation
Collector/ProfileClient.php
Outdated
| @@ -47,23 +47,18 @@ class ProfileClient implements HttpClient, HttpAsyncClient | |||
| private $eventNames = []; | |||
|
|
|||
| /** | |||
| * @param HttpClient|HttpAsyncClient $client The client to profile. Client must implement both HttpClient and | |||
| * HttpAsyncClient interfaces. | |||
| * @param HttpClient|HttpAsyncClient $client The client to profile. Client must implement both HttpClient or | |||
There was a problem hiding this comment.
implement HttpClient or HttpAsyncClient
Tests/Functional/Issue206.php
Outdated
|
|
||
| class Issue206 extends WebTestCase | ||
| { | ||
| public function testCustomClientStillAvailable() |
There was a problem hiding this comment.
You should assert something in this test to avoid adding a risky test.
Tests/Functional/Issue206.php
Outdated
| @@ -22,5 +22,8 @@ public function testCustomClientStillAvailable() | |||
| // Create a client | |||
| $myCustomClient = new HttpMethodsClient(HttpClientDiscovery::find(), MessageFactoryDiscovery::find()); | |||
| $pluginClient = (new PluginClientFactory())->createClient($myCustomClient, []); | |||
|
|
|||
| // If we get to this line, no exceptions has been thrown. | |||
There was a problem hiding this comment.
Is this still relevant as we removed the throw statement from ProfileClient. Maybe you could use NSA to check that the $client inside $pluginClient is a FlexibleHttpClient.
|
This one was left aside since ages :/ Could you rebase to check that build is still green? This is required for next release. |
|
Im done with this PR. We should use the flexClient. This will fix #206. |
dbu
changed the title
|
Thank you! |
| * @param Collector $collector | ||
| * @param Formatter $formatter | ||
| * @param Stopwatch $stopwatch | ||
| */ | ||
| public function __construct($client, Collector $collector, Formatter $formatter, Stopwatch $stopwatch) | ||
| { | ||
| if (!($client instanceof HttpClient && $client instanceof HttpAsyncClient)) { | ||
| throw new \RuntimeException(sprintf( |
There was a problem hiding this comment.
Shouldn't the exception still be thrown if neither of the interfaces is implemented? I know that the FlexibleHttpClient throws anyway, but I am not sure if relying on that behaviour would be a good idea and also could the stack trace be confusing for users.
There was a problem hiding this comment.
Yes, an exception are thrown, But yes. It could be a good idea to throw en exception here as well.
fix #206
FYI @fbourigault