Skip to content

Don't use PluginClient for discovered clients #200

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 26, 2017
Merged

Don't use PluginClient for discovered clients #200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
073285c
don't use PluginClient for discovered clients
fbourigault Aug 18, 2017
3f862c6
add blank line before break instruction
fbourigault Aug 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion Collector/ProfileClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,16 @@ public function __construct($client, Collector $collector, Formatter $formatter,
*/
public function sendAsyncRequest(RequestInterface $request)
{
$activateStack = true;
$stack = $this->collector->getActiveStack();
if ($stack === null) {
//When using a discovered client not wrapped in a PluginClient, we don't have a stack from StackPlugin. So
//we create our own stack and activate it!
$stack = new Stack('Default', $this->formatter->formatRequest($request));
$this->collector->addStack($stack);
$this->collector->activateStack($stack);
$activateStack = false;
}

$this->collectRequestInformations($request, $stack);
$event = $this->stopwatch->start($this->getStopwatchEventName($request));
Expand All @@ -98,7 +107,10 @@ public function sendAsyncRequest(RequestInterface $request)
return $this->client->sendAsyncRequest($request)->then($onFulfilled, $onRejected);
} finally {
$event->stop();
$this->collector->activateStack($stack);
if ($activateStack) {
//We only activate the stack when created by the StackPlugin.
$this->collector->activateStack($stack);
}
}
}

Expand All @@ -108,6 +120,12 @@ public function sendAsyncRequest(RequestInterface $request)
public function sendRequest(RequestInterface $request)
{
$stack = $this->collector->getActiveStack();
if ($stack === null) {
//When using a discovered client not wrapped in a PluginClient, we don't have a stack from StackPlugin. So
//we create our own stack but don't activate it.
$stack = new Stack('Default', $this->formatter->formatRequest($request));
$this->collector->addStack($stack);
}

$this->collectRequestInformations($request, $stack);
$event = $this->stopwatch->start($this->getStopwatchEventName($request));
Expand Down
16 changes: 14 additions & 2 deletions DependencyInjection/HttplugExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,37 +149,45 @@ private function configurePluginByName($name, Definition $definition, array $con
->replaceArgument(0, new Reference($config['cache_pool']))
->replaceArgument(1, new Reference($config['stream_factory']))
->replaceArgument(2, $config['config']);

break;
case 'cookie':
$definition->replaceArgument(0, new Reference($config['cookie_jar']));

break;
case 'decoder':
$definition->addArgument([
'use_content_encoding' => $config['use_content_encoding'],
]);

break;
case 'history':
$definition->replaceArgument(0, new Reference($config['journal']));

break;
case 'logger':
$definition->replaceArgument(0, new Reference($config['logger']));
if (!empty($config['formatter'])) {
$definition->replaceArgument(1, new Reference($config['formatter']));
}

break;
case 'redirect':
$definition->addArgument([
'preserve_header' => $config['preserve_header'],
'use_default_for_multiple' => $config['use_default_for_multiple'],
]);

break;
case 'retry':
$definition->addArgument([
'retries' => $config['retry'],
]);

break;
case 'stopwatch':
$definition->replaceArgument(0, new Reference($config['stopwatch']));

break;

/* client specific plugins */
Expand All @@ -191,12 +199,14 @@ private function configurePluginByName($name, Definition $definition, array $con
$definition->replaceArgument(1, [
'replace' => $config['replace'],
]);

break;
case 'header_append':
case 'header_defaults':
case 'header_set':
case 'header_remove':
$definition->replaceArgument(0, $config['headers']);

break;

default:
Expand All @@ -220,19 +230,23 @@ private function configureAuthentication(ContainerBuilder $container, array $con
case 'bearer':
$container->register($authServiceKey, Bearer::class)
->addArgument($values['token']);

break;
case 'basic':
$container->register($authServiceKey, BasicAuth::class)
->addArgument($values['username'])
->addArgument($values['password']);

break;
case 'wsse':
$container->register($authServiceKey, Wsse::class)
->addArgument($values['username'])
->addArgument($values['password']);

break;
case 'service':
$authServiceKey = $values['service'];

break;
default:
throw new \LogicException(sprintf('Unknown authentication type: "%s"', $values['type']));
Expand Down Expand Up @@ -361,7 +375,6 @@ private function configureAutoDiscoveryClients(ContainerBuilder $container, arra
if ($httpClient !== 'auto') {
$container->removeDefinition('httplug.auto_discovery.auto_discovered_client');
$container->removeDefinition('httplug.collector.auto_discovered_client');
$container->removeDefinition('httplug.auto_discovery.auto_discovered_client.plugin');

if (!empty($httpClient)) {
$container->setAlias('httplug.auto_discovery.auto_discovered_client', $httpClient);
Expand All @@ -373,7 +386,6 @@ private function configureAutoDiscoveryClients(ContainerBuilder $container, arra
if ($asyncHttpClient !== 'auto') {
$container->removeDefinition('httplug.auto_discovery.auto_discovered_async');
$container->removeDefinition('httplug.collector.auto_discovered_async');
$container->removeDefinition('httplug.auto_discovery.auto_discovered_async.plugin');

if (!empty($asyncHttpClient)) {
$container->setAlias('httplug.auto_discovery.auto_discovered_async', $asyncHttpClient);
Expand Down
25 changes: 2 additions & 23 deletions Resources/config/data-collector.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,41 +27,20 @@
</service>

<!-- Discovered clients -->
<service id="httplug.collector.auto_discovered_client" class="Http\HttplugBundle\Collector\ProfileClient" decorates="httplug.auto_discovery.auto_discovered_client" decoration-priority="2" public="false">
<service id="httplug.collector.auto_discovered_client" class="Http\HttplugBundle\Collector\ProfileClient" decorates="httplug.auto_discovery.auto_discovered_client" public="false">
<argument type="service" id="httplug.collector.auto_discovered_client.inner"/>
<argument type="service" id="httplug.collector.collector"/>
<argument type="service" id="httplug.collector.formatter"/>
<argument type="service" id="debug.stopwatch"/>
</service>

<service id="httplug.collector.auto_discovered_async" class="Http\HttplugBundle\Collector\ProfileClient" decorates="httplug.auto_discovery.auto_discovered_async" decoration-priority="2" public="false">
<service id="httplug.collector.auto_discovered_async" class="Http\HttplugBundle\Collector\ProfileClient" decorates="httplug.auto_discovery.auto_discovered_async" public="false">
<argument type="service" id="httplug.collector.auto_discovered_async.inner"/>
<argument type="service" id="httplug.collector.collector"/>
<argument type="service" id="httplug.collector.formatter"/>
<argument type="service" id="debug.stopwatch"/>
</service>

<service id="httplug.auto_discovery.auto_discovered_client.plugin" class="Http\Client\Common\PluginClient" decorates="httplug.auto_discovery.auto_discovered_client" public="false">
<argument type="service" id="httplug.auto_discovery.auto_discovered_client.plugin.inner"/>
<argument type="collection">
<argument type="service">
<service parent="httplug.plugin.stack">
<argument>auto_discovered_client</argument>
</service>
</argument>
</argument>
</service>
<service id="httplug.auto_discovery.auto_discovered_async.plugin" class="Http\Client\Common\PluginClient" decorates="httplug.auto_discovery.auto_discovered_async" public="false">
<argument type="service" id="httplug.auto_discovery.auto_discovered_async.plugin.inner"/>
<argument type="collection">
<argument type="service">
<service parent="httplug.plugin.stack">
<argument>auto_discovered_async</argument>
</service>
</argument>
</argument>
</service>

<!-- ClientFactories -->
<service id="httplug.collector.factory.auto" class="Http\HttplugBundle\Collector\ProfileClientFactory" decorates="httplug.factory.auto" public="false">
<argument type="service" id="httplug.collector.factory.auto.inner"/>
Expand Down
8 changes: 0 additions & 8 deletions Resources/config/services.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,6 @@
<factory class="Http\Discovery\HttpAsyncClientDiscovery" method="find" />
</service>

<!-- Decorate discovered clients with PluginClient -->
<service id="httplug.auto_discovery.auto_discovered_client.plugin" class="Http\Client\Common\PluginClient" decorates="httplug.auto_discovery.auto_discovered_client" decoration-priority="1" public="false">
<argument type="service" id="httplug.auto_discovery.auto_discovered_client.plugin.inner"/>
</service>
<service id="httplug.auto_discovery.auto_discovered_async.plugin" class="Http\Client\Common\PluginClient" decorates="httplug.auto_discovery.auto_discovered_async" decoration-priority="1" public="false">
<argument type="service" id="httplug.auto_discovery.auto_discovered_async.plugin.inner"/>
</service>

<!-- Discovery with autowiring support for Symfony 3.3+ -->
<service id="httplug.message_factory.default" class="Http\Message\MessageFactory">
<factory class="Http\Discovery\MessageFactoryDiscovery" method="find" />
Expand Down
29 changes: 7 additions & 22 deletions Tests/Functional/DiscoveredClientsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,12 @@

namespace Http\HttplugBundle\Tests\Functional;

use Http\Client\Common\PluginClient;
use Http\Client\HttpAsyncClient;
use Http\Client\HttpClient;
use Http\Discovery\HttpAsyncClientDiscovery;
use Http\Discovery\HttpClientDiscovery;
use Http\Discovery\Strategy\CommonClassesStrategy;
use Http\HttplugBundle\Collector\StackPlugin;
use Http\HttplugBundle\Collector\ProfileClient;
use Http\HttplugBundle\Discovery\ConfiguredClientsStrategy;
use Nyholm\NSA;
use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
Expand All @@ -24,9 +23,7 @@ public function testDiscoveredClient()

$service = $container->get('httplug.auto_discovery.auto_discovered_client');

$this->assertInstanceOf(PluginClient::class, $service);
$this->assertInstanceOf(HttpClient::class, NSA::getProperty($service, 'client'));
$this->assertEmpty(NSA::getProperty($service, 'plugins'));
$this->assertInstanceOf(HttpClient::class, $service);
}

public function testDiscoveredAsyncClient()
Expand All @@ -37,9 +34,7 @@ public function testDiscoveredAsyncClient()

$service = $container->get('httplug.auto_discovery.auto_discovered_async');

$this->assertInstanceOf(PluginClient::class, $service);
$this->assertInstanceOf(HttpAsyncClient::class, NSA::getProperty($service, 'client'));
$this->assertEmpty(NSA::getProperty($service, 'plugins'));
$this->assertInstanceOf(HttpAsyncClient::class, $service);
}

public function testDiscoveredClientWithProfilingEnabled()
Expand All @@ -50,13 +45,8 @@ public function testDiscoveredClientWithProfilingEnabled()

$service = $container->get('httplug.auto_discovery.auto_discovered_client');

$this->assertInstanceOf(PluginClient::class, $service);
$this->assertInstanceOf(ProfileClient::class, $service);
$this->assertInstanceOf(HttpClient::class, NSA::getProperty($service, 'client'));

$plugins = NSA::getProperty($service, 'plugins');
$this->assertCount(1, $plugins);
$this->assertInstanceOf(StackPlugin::class, $plugins[0]);
$this->assertEquals('auto_discovered_client', NSA::getProperty($plugins[0], 'client'));
}

public function testDiscoveredAsyncClientWithProfilingEnabled()
Expand All @@ -67,13 +57,8 @@ public function testDiscoveredAsyncClientWithProfilingEnabled()

$service = $container->get('httplug.auto_discovery.auto_discovered_async');

$this->assertInstanceOf(PluginClient::class, $service);
$this->assertInstanceOf(ProfileClient::class, $service);
$this->assertInstanceOf(HttpAsyncClient::class, NSA::getProperty($service, 'client'));

$plugins = NSA::getProperty($service, 'plugins');
$this->assertCount(1, $plugins);
$this->assertInstanceOf(StackPlugin::class, $plugins[0]);
$this->assertEquals('auto_discovered_async', NSA::getProperty($plugins[0], 'client'));
}

/**
Expand All @@ -92,9 +77,9 @@ public function testDiscovery()
$httpClient = $container->get('httplug.auto_discovery.auto_discovered_client');
$httpAsyncClient = $container->get('httplug.auto_discovery.auto_discovered_async');

$this->assertInstanceOf(PluginClient::class, $httpClient);
$this->assertInstanceOf(ProfileClient::class, $httpClient);
$this->assertSame(HttpClientDiscovery::find(), $httpClient);
$this->assertInstanceOf(PluginClient::class, $httpAsyncClient);
$this->assertInstanceOf(ProfileClient::class, $httpAsyncClient);
$this->assertSame(HttpAsyncClientDiscovery::find(), $httpAsyncClient);
}

Expand Down