From 3280a1f71ece525c5b2140449ab781832ad973a5 Mon Sep 17 00:00:00 2001 From: Tobias Nyholm Date: Wed, 20 Jul 2016 18:03:07 +0200 Subject: [PATCH 1/7] Refactor the HttplugExtension --- ClientFactory/PluginClientFactory.php | 18 +- DependencyInjection/HttplugExtension.php | 197 +++++++++------------- DependencyInjection/ProfilerExtension.php | 85 ++++++++++ Resources/config/data-collector.xml | 1 - Resources/config/services.xml | 2 + 5 files changed, 176 insertions(+), 127 deletions(-) create mode 100644 DependencyInjection/ProfilerExtension.php diff --git a/ClientFactory/PluginClientFactory.php b/ClientFactory/PluginClientFactory.php index f0ff28dc..68c49b31 100644 --- a/ClientFactory/PluginClientFactory.php +++ b/ClientFactory/PluginClientFactory.php @@ -13,15 +13,21 @@ final class PluginClientFactory { /** - * @param Plugin[] $plugins - * @param ClientFactory $factory - * @param array $config config to the client factory - * @param array $pluginClientOptions config forwarded to the PluginClient + * @param Plugin[] $plugins + * @param ClientFactory|callable $factory + * @param array $config config to the client factory + * @param array $pluginClientOptions config forwarded to the PluginClient * * @return PluginClient */ - public static function createPluginClient(array $plugins, ClientFactory $factory, array $config, array $pluginClientOptions = []) + public static function createPluginClient(array $plugins, $factory, array $config, array $pluginClientOptions = []) { - return new PluginClient($factory->createClient($config), $plugins, $pluginClientOptions); + if ($factory instanceof ClientFactory) { + return new PluginClient($factory->createClient($config), $plugins, $pluginClientOptions); + } elseif (is_callable($factory)) { + return new PluginClient($factory($config), $plugins, $pluginClientOptions); + } + + throw new \RuntimeException(sprintf('Second argument to PluginClientFactory::createPluginClient must be a "%s" or a callale.', ClientFactory::class)); } } diff --git a/DependencyInjection/HttplugExtension.php b/DependencyInjection/HttplugExtension.php index b38d0222..7b4b5abf 100644 --- a/DependencyInjection/HttplugExtension.php +++ b/DependencyInjection/HttplugExtension.php @@ -5,11 +5,10 @@ use Http\Client\Common\FlexibleHttpClient; use Http\Client\Common\HttpMethodsClient; use Http\Client\Common\Plugin\AuthenticationPlugin; -use Http\Client\Common\PluginClient; use Http\Discovery\HttpAsyncClientDiscovery; use Http\Discovery\HttpClientDiscovery; use Http\HttplugBundle\ClientFactory\DummyClient; -use Http\HttplugBundle\Collector\DebugPlugin; +use Http\HttplugBundle\ClientFactory\PluginClientFactory; use Http\Message\Authentication\BasicAuth; use Http\Message\Authentication\Bearer; use Http\Message\Authentication\Wsse; @@ -39,21 +38,6 @@ public function load(array $configs, ContainerBuilder $container) $loader->load('services.xml'); $loader->load('plugins.xml'); - $toolbar = is_bool($config['toolbar']['enabled']) ? $config['toolbar']['enabled'] : $container->hasParameter('kernel.debug') && $container->getParameter('kernel.debug'); - - if ($toolbar) { - $loader->load('data-collector.xml'); - - if (!empty($config['toolbar']['formatter'])) { - // Add custom formatter - $container->getDefinition('httplug.collector.debug_collector') - ->replaceArgument(0, new Reference($config['toolbar']['formatter'])); - } - - $container->getDefinition('httplug.formatter.full_http_message') - ->addArgument($config['toolbar']['captured_body_length']); - } - foreach ($config['classes'] as $service => $class) { if (!empty($class)) { $container->register(sprintf('httplug.%s.default', $service), $class); @@ -66,8 +50,13 @@ public function load(array $configs, ContainerBuilder $container) } $this->configurePlugins($container, $config['plugins']); - $this->configureClients($container, $config, $toolbar); - $this->configureAutoDiscoveryClients($container, $config, $toolbar); + $serviceIds = $this->configureClients($container, $config); + $autoServiceIds = $this->configureAutoDiscoveryClients($container, $config); + + $toolbar = is_bool($config['toolbar']['enabled']) ? $config['toolbar']['enabled'] : $container->hasParameter('kernel.debug') && $container->getParameter('kernel.debug'); + if ($toolbar) { + (new ProfilerExtension())->load($config, $container, array_unique(array_merge($serviceIds, $autoServiceIds))); + } } /** @@ -75,12 +64,13 @@ public function load(array $configs, ContainerBuilder $container) * * @param ContainerBuilder $container * @param array $config - * @param bool $enableCollector + * + * @raturn array with client service names */ - private function configureClients(ContainerBuilder $container, array $config, $enableCollector) + private function configureClients(ContainerBuilder $container, array $config) { - // If we have a client named 'default' - $first = isset($config['clients']['default']) ? 'default' : null; + $serviceIds = []; + $first = null; foreach ($config['clients'] as $name => $arguments) { if ($first === null) { @@ -88,23 +78,19 @@ private function configureClients(ContainerBuilder $container, array $config, $e $first = $name; } - $this->configureClient($container, $name, $arguments, $enableCollector); + $serviceIds[] = $this->configureClient($container, $name, $arguments); } // If we have clients configured if ($first !== null) { - if ($first !== 'default') { + // If we do not have a client named 'default' + if (!isset($config['clients']['default'])) { // Alias the first client to httplug.client.default $container->setAlias('httplug.client.default', 'httplug.client.'.$first); } - } elseif ($enableCollector) { - $serviceIdDebugPlugin = $this->registerDebugPlugin($container, 'default'); - // No client was configured. Make sure to configure the auto discovery client with the PluginClient. - $container->register('httplug.client', PluginClient::class) - ->addArgument(new Reference('httplug.client.default')) - ->addArgument([]) - ->addArgument(['debug_plugins' => [new Reference($serviceIdDebugPlugin)]]); } + + return $serviceIds; } /** @@ -212,46 +198,34 @@ private function configureAuthentication(ContainerBuilder $container, array $con * @param ContainerBuilder $container * @param string $name * @param array $arguments - * @param bool $enableCollector + * + * @return string The service id of the client. */ - private function configureClient(ContainerBuilder $container, $name, array $arguments, $enableCollector) + private function configureClient(ContainerBuilder $container, $name, array $arguments) { $serviceId = 'httplug.client.'.$name; - $def = $container->register($serviceId, DummyClient::class); - - // If there are no plugins nor should we use the data collector - if (empty($arguments['plugins']) && !$enableCollector) { - $def->setFactory([new Reference($arguments['factory']), 'createClient']) - ->addArgument($arguments['config']); - } else { - $def - ->setFactory('Http\HttplugBundle\ClientFactory\PluginClientFactory::createPluginClient') - ->addArgument( - array_map( - function ($id) { - return new Reference($id); - }, - $arguments['plugins'] - ) + $definition = $container->register($serviceId, DummyClient::class); + $definition->setFactory([PluginClientFactory::class, 'createPluginClient']) + ->addArgument( + array_map( + function ($id) { + return new Reference($id); + }, + $arguments['plugins'] ) - ->addArgument(new Reference($arguments['factory'])) - ->addArgument($arguments['config']) - ; - - if ($enableCollector) { - $serviceIdDebugPlugin = $this->registerDebugPlugin($container, $name); - $def->addArgument(['debug_plugins' => [new Reference($serviceIdDebugPlugin)]]); + ) + ->addArgument(new Reference($arguments['factory'])) + ->addArgument($arguments['config']) + ->addArgument([]) + ; - // tell the plugin journal what plugins we used - $container->getDefinition('httplug.collector.plugin_journal') - ->addMethodCall('setPlugins', [$name, $arguments['plugins']]); - } - } + // Tell the plugin journal what plugins we used + $container->getDefinition('httplug.collector.plugin_journal') + ->addMethodCall('setPlugins', [$name, $arguments['plugins']]); /* * Decorate the client with clients from client-common */ - if ($arguments['flexible_client']) { $container->register($serviceId.'.flexible', FlexibleHttpClient::class) ->addArgument(new Reference($serviceId.'.flexible.inner')) @@ -265,91 +239,74 @@ function ($id) { ->setPublic(false) ->setDecoratedService($serviceId); } - } - /** - * Create a new plugin service for this client. - * - * @param ContainerBuilder $container - * @param string $name - * - * @return string - */ - private function registerDebugPlugin(ContainerBuilder $container, $name) - { - $serviceIdDebugPlugin = 'httplug.client.'.$name.'.debug_plugin'; - $container->register($serviceIdDebugPlugin, DebugPlugin::class) - ->addArgument(new Reference('httplug.collector.debug_collector')) - ->addArgument($name) - ->setPublic(false); - - return $serviceIdDebugPlugin; + return $serviceId; } /** - * Make sure we inject the debug plugin for clients found by auto discovery. + * Make the user can select what client is used for auto discovery. If none is provided, a service will be created + * by finding a client using auto discovery. * * @param ContainerBuilder $container * @param array $config - * @param bool $enableCollector - */ - private function configureAutoDiscoveryClients(ContainerBuilder $container, array $config, $enableCollector) + * + * @return array of service ids. + * */ + private function configureAutoDiscoveryClients(ContainerBuilder $container, array $config) { + $serviceIds = []; + $httpClient = $config['discovery']['client']; - if ($httpClient === 'auto') { - $httpClient = $this->registerAutoDiscoverableClientWithDebugPlugin( - $container, - 'client', - [HttpClientDiscovery::class, 'find'], - $enableCollector - ); - } elseif ($httpClient) { + if (!empty($httpClient)) { + if ($httpClient === 'auto') { + $httpClient = $this->registerAutoDiscoverableClient( + $container, + 'auto_discovered_client', + [HttpClientDiscovery::class, 'find'] + ); + } + + $serviceIds[] = $httpClient; $httpClient = new Reference($httpClient); } $asyncHttpClient = $config['discovery']['async_client']; - if ($asyncHttpClient === 'auto') { - $asyncHttpClient = $this->registerAutoDiscoverableClientWithDebugPlugin( - $container, - 'async_client', - [HttpAsyncClientDiscovery::class, 'find'], - $enableCollector - ); - } elseif ($asyncHttpClient) { + if (!empty($asyncHttpClient)) { + if ($asyncHttpClient === 'auto') { + $asyncHttpClient = $this->registerAutoDiscoverableClient( + $container, + 'auto_discovered_async', + [HttpAsyncClientDiscovery::class, 'find'] + ); + } + $serviceIds[] = $asyncHttpClient; $asyncHttpClient = new Reference($httpClient); } $container->getDefinition('httplug.strategy') ->addArgument($httpClient) ->addArgument($asyncHttpClient); + + return $serviceIds; } /** + * Find a client with auto discovery and return a service Reference to it. + * * @param ContainerBuilder $container * @param string $name * @param callable $factory - * @param bool $enableCollector * - * @return Reference + * @return string service id */ - private function registerAutoDiscoverableClientWithDebugPlugin(ContainerBuilder $container, $name, $factory, $enableCollector) + private function registerAutoDiscoverableClient(ContainerBuilder $container, $name, $factory) { - $definition = $container->register('httplug.auto_discovery_'.$name.'.pure', DummyClient::class); - $definition->setPublic(false); - $definition->setFactory($factory); - - $pluginDefinition = $container - ->register('httplug.auto_discovery_'.$name.'.plugin', PluginClient::class) - ->setPublic(false) - ->addArgument(new Reference('httplug.auto_discovery_'.$name.'.pure')) - ->addArgument([]) - ; - - if ($enableCollector) { - $serviceIdDebugPlugin = $this->registerDebugPlugin($container, 'auto_discovery_'.$name); - $pluginDefinition->addArgument(['debug_plugins' => [new Reference($serviceIdDebugPlugin)]]); - } + $serviceId = 'httplug.auto_discovery.'.$name; + $definition = $container->register($serviceId, DummyClient::class); + $definition + ->setFactory([PluginClientFactory::class, 'createPluginClient']) + ->setArguments([[], $factory, [], []]); - return new Reference('httplug.auto_discovery_'.$name.'.plugin'); + return $serviceId; } } diff --git a/DependencyInjection/ProfilerExtension.php b/DependencyInjection/ProfilerExtension.php new file mode 100644 index 00000000..ac60906a --- /dev/null +++ b/DependencyInjection/ProfilerExtension.php @@ -0,0 +1,85 @@ + + * @author Tobias Nyholm + */ +class ProfilerExtension +{ + /** + * @param array $config Processed configuration + * @param ContainerBuilder $container + * @param array $clientServiceIds ids of all clients + */ + public function load(array $config, ContainerBuilder $container, array $clientServiceIds) + { + $loader = new XmlFileLoader($container, new FileLocator(__DIR__.'/../Resources/config')); + $loader->load('data-collector.xml'); + + if (!empty($config['toolbar']['formatter'])) { + // Add custom formatter + $container->getDefinition('httplug.collector.debug_collector') + ->replaceArgument(0, new Reference($config['toolbar']['formatter'])); + } + + $container->getDefinition('httplug.formatter.full_http_message') + ->addArgument($config['toolbar']['captured_body_length']); + + foreach ($clientServiceIds as $clientId) { + $pluginClientDefinition = $container->getDefinition($clientId); + $serviceIdDebugPlugin = $this->registerDebugPlugin($container, $clientId); + + $argument = $this->mergeDebugPluginArguments($pluginClientDefinition->getArgument(3), [new Reference($serviceIdDebugPlugin)]); + $pluginClientDefinition->replaceArgument(3, $argument); + } + } + + /** + * Create a new plugin service for this client. + * + * @param ContainerBuilder $container + * @param string $name + * + * @return string + */ + private function registerDebugPlugin(ContainerBuilder $container, $name) + { + $serviceIdDebugPlugin = $name.'.debug_plugin'; + $container->register($serviceIdDebugPlugin, DebugPlugin::class) + ->addArgument(new Reference('httplug.collector.debug_collector')) + ->addArgument(substr($name, strrpos($name, '.')+1)) + ->setPublic(false); + + return $serviceIdDebugPlugin; + } + + /** + * @param array $existing + * @param array $newArgument + * + * @return array + */ + private function mergeDebugPluginArguments($existing, $newArgument) + { + if (empty($existing)) { + $mergedArgument = ['debug_plugins' => $newArgument]; + } elseif (empty($existing['debug_plugins'])) { + $mergedArgument['debug_plugins'] = $newArgument; + } else { + $mergedArgument['debug_plugins'] = array_merge($existing['debug_plugins'], $newArgument); + } + + return $mergedArgument; + } +} diff --git a/Resources/config/data-collector.xml b/Resources/config/data-collector.xml index d1815dfd..fa2d8f07 100644 --- a/Resources/config/data-collector.xml +++ b/Resources/config/data-collector.xml @@ -4,7 +4,6 @@ xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd"> - diff --git a/Resources/config/services.xml b/Resources/config/services.xml index b5ad3efc..0fb9f5dc 100644 --- a/Resources/config/services.xml +++ b/Resources/config/services.xml @@ -8,6 +8,8 @@ + + From 0619c0afe3e900b0208be92a6e2bc33d12bdf22d Mon Sep 17 00:00:00 2001 From: Tobias Nyholm Date: Wed, 20 Jul 2016 18:10:36 +0200 Subject: [PATCH 2/7] minor refactor --- ClientFactory/PluginClientFactory.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/ClientFactory/PluginClientFactory.php b/ClientFactory/PluginClientFactory.php index 68c49b31..908e0172 100644 --- a/ClientFactory/PluginClientFactory.php +++ b/ClientFactory/PluginClientFactory.php @@ -23,11 +23,13 @@ final class PluginClientFactory public static function createPluginClient(array $plugins, $factory, array $config, array $pluginClientOptions = []) { if ($factory instanceof ClientFactory) { - return new PluginClient($factory->createClient($config), $plugins, $pluginClientOptions); + $client = $factory->createClient($config); } elseif (is_callable($factory)) { - return new PluginClient($factory($config), $plugins, $pluginClientOptions); + $client = $factory($config); + } else { + throw new \RuntimeException(sprintf('Second argument to PluginClientFactory::createPluginClient must be a "%s" or a callale.', ClientFactory::class)); } - throw new \RuntimeException(sprintf('Second argument to PluginClientFactory::createPluginClient must be a "%s" or a callale.', ClientFactory::class)); + return new PluginClient($client, $plugins, $pluginClientOptions); } } From b661a82b876d5eefb1863a377e61292f60a609b0 Mon Sep 17 00:00:00 2001 From: Tobias Nyholm Date: Wed, 20 Jul 2016 18:12:41 +0200 Subject: [PATCH 3/7] style fix --- DependencyInjection/ProfilerExtension.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DependencyInjection/ProfilerExtension.php b/DependencyInjection/ProfilerExtension.php index ac60906a..0622e54a 100644 --- a/DependencyInjection/ProfilerExtension.php +++ b/DependencyInjection/ProfilerExtension.php @@ -58,7 +58,7 @@ private function registerDebugPlugin(ContainerBuilder $container, $name) $serviceIdDebugPlugin = $name.'.debug_plugin'; $container->register($serviceIdDebugPlugin, DebugPlugin::class) ->addArgument(new Reference('httplug.collector.debug_collector')) - ->addArgument(substr($name, strrpos($name, '.')+1)) + ->addArgument(substr($name, strrpos($name, '.') + 1)) ->setPublic(false); return $serviceIdDebugPlugin; From 395100656366069de44df55962d000dc9ac144b5 Mon Sep 17 00:00:00 2001 From: Tobias Nyholm Date: Thu, 21 Jul 2016 17:46:51 +0200 Subject: [PATCH 4/7] typo --- DependencyInjection/HttplugExtension.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DependencyInjection/HttplugExtension.php b/DependencyInjection/HttplugExtension.php index 7b4b5abf..6b3aa8ef 100644 --- a/DependencyInjection/HttplugExtension.php +++ b/DependencyInjection/HttplugExtension.php @@ -65,7 +65,7 @@ public function load(array $configs, ContainerBuilder $container) * @param ContainerBuilder $container * @param array $config * - * @raturn array with client service names + * @return array with client service names */ private function configureClients(ContainerBuilder $container, array $config) { From a1aae484b7d3aa934df1baf4b90c8a7145779204 Mon Sep 17 00:00:00 2001 From: Tobias Nyholm Date: Thu, 21 Jul 2016 18:01:07 +0200 Subject: [PATCH 5/7] Minor fixes --- ClientFactory/PluginClientFactory.php | 2 +- DependencyInjection/HttplugExtension.php | 2 +- DependencyInjection/ProfilerExtension.php | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/ClientFactory/PluginClientFactory.php b/ClientFactory/PluginClientFactory.php index 908e0172..9213f878 100644 --- a/ClientFactory/PluginClientFactory.php +++ b/ClientFactory/PluginClientFactory.php @@ -27,7 +27,7 @@ public static function createPluginClient(array $plugins, $factory, array $confi } elseif (is_callable($factory)) { $client = $factory($config); } else { - throw new \RuntimeException(sprintf('Second argument to PluginClientFactory::createPluginClient must be a "%s" or a callale.', ClientFactory::class)); + throw new \RuntimeException(sprintf('Second argument to PluginClientFactory::createPluginClient must be a "%s" or a callable.', ClientFactory::class)); } return new PluginClient($client, $plugins, $pluginClientOptions); diff --git a/DependencyInjection/HttplugExtension.php b/DependencyInjection/HttplugExtension.php index 6b3aa8ef..24bc2335 100644 --- a/DependencyInjection/HttplugExtension.php +++ b/DependencyInjection/HttplugExtension.php @@ -251,7 +251,7 @@ function ($id) { * @param array $config * * @return array of service ids. - * */ + */ private function configureAutoDiscoveryClients(ContainerBuilder $container, array $config) { $serviceIds = []; diff --git a/DependencyInjection/ProfilerExtension.php b/DependencyInjection/ProfilerExtension.php index 0622e54a..4ee235a3 100644 --- a/DependencyInjection/ProfilerExtension.php +++ b/DependencyInjection/ProfilerExtension.php @@ -40,7 +40,7 @@ public function load(array $config, ContainerBuilder $container, array $clientSe $pluginClientDefinition = $container->getDefinition($clientId); $serviceIdDebugPlugin = $this->registerDebugPlugin($container, $clientId); - $argument = $this->mergeDebugPluginArguments($pluginClientDefinition->getArgument(3), [new Reference($serviceIdDebugPlugin)]); + $argument = $this->mergeDebugPluginArguments([new Reference($serviceIdDebugPlugin)], $pluginClientDefinition->getArgument(3)); $pluginClientDefinition->replaceArgument(3, $argument); } } @@ -65,12 +65,12 @@ private function registerDebugPlugin(ContainerBuilder $container, $name) } /** - * @param array $existing * @param array $newArgument + * @param array $existing * * @return array */ - private function mergeDebugPluginArguments($existing, $newArgument) + private function mergeDebugPluginArguments(array $newArgument, array $existing = []) { if (empty($existing)) { $mergedArgument = ['debug_plugins' => $newArgument]; From 4f76044dc770552498fe44299be9efafac889ecf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1rk=20S=C3=A1gi-Kaz=C3=A1r?= Date: Sat, 23 Jul 2016 21:52:18 +0200 Subject: [PATCH 6/7] Merge Profiling back to main extension Separating profiling into a separate class does not make the code cleaner or more readable. In this commit: - Profiling moved back to main extension - Toolbar (profiling) defaults to kernel.debug --- CHANGELOG.md | 7 + DependencyInjection/Configuration.php | 32 +++- DependencyInjection/HttplugExtension.php | 152 +++++++++++++----- DependencyInjection/ProfilerExtension.php | 85 ---------- Resources/config/data-collector.xml | 2 + Resources/config/services.xml | 2 - .../Fixtures/config/toolbar_auto.yml | 3 + .../DependencyInjection/ConfigurationTest.php | 7 +- .../HttplugExtensionTest.php | 7 + 9 files changed, 159 insertions(+), 138 deletions(-) delete mode 100644 DependencyInjection/ProfilerExtension.php create mode 100644 Tests/Resources/Fixtures/config/toolbar_auto.yml diff --git a/CHANGELOG.md b/CHANGELOG.md index 8948f8a9..ccf6681f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,13 @@ # Change Log +## UNRELEASED + +### Deprecated + +- `auto` value in `toolbar.enabled` config + + ## 1.2.2 - 2016-07-19 ### Fixed diff --git a/DependencyInjection/Configuration.php b/DependencyInjection/Configuration.php index 90653dcd..d8430a5e 100644 --- a/DependencyInjection/Configuration.php +++ b/DependencyInjection/Configuration.php @@ -19,6 +19,23 @@ */ class Configuration implements ConfigurationInterface { + /** + * Whether to use the debug mode. + * + * @see https://github.com/doctrine/DoctrineBundle/blob/v1.5.2/DependencyInjection/Configuration.php#L31-L41 + * + * @var bool + */ + private $debug; + + /** + * @param bool $debug + */ + public function __construct($debug) + { + $this->debug = (bool) $debug; + } + /** * {@inheritdoc} */ @@ -75,12 +92,17 @@ public function getConfigTreeBuilder() ->end() ->arrayNode('toolbar') ->addDefaultsIfNotSet() - ->info('Extend the debug profiler with inforation about requests.') + ->info('Extend the debug profiler with information about requests.') ->children() - ->enumNode('enabled') - ->info('If "auto" (default), the toolbar is activated when kernel.debug is true. You can force the toolbar on and off by changing this option.') - ->values([true, false, 'auto']) - ->defaultValue('auto') + ->booleanNode('enabled') // @deprecated value auto in 1.3.0 + ->beforeNormalization() + ->ifString() + ->then(function ($v) { + return 'auto' === $v ? $this->debug : $v; + }) + ->end() + ->info('Turn the toolbar on or off. Defaults to kernel debug mode.') + ->defaultValue($this->debug) ->end() ->scalarNode('formatter')->defaultNull()->end() ->scalarNode('captured_body_length') diff --git a/DependencyInjection/HttplugExtension.php b/DependencyInjection/HttplugExtension.php index 24bc2335..0e2ef183 100644 --- a/DependencyInjection/HttplugExtension.php +++ b/DependencyInjection/HttplugExtension.php @@ -9,6 +9,7 @@ use Http\Discovery\HttpClientDiscovery; use Http\HttplugBundle\ClientFactory\DummyClient; use Http\HttplugBundle\ClientFactory\PluginClientFactory; +use Http\HttplugBundle\Collector\DebugPlugin; use Http\Message\Authentication\BasicAuth; use Http\Message\Authentication\Bearer; use Http\Message\Authentication\Wsse; @@ -38,6 +39,7 @@ public function load(array $configs, ContainerBuilder $container) $loader->load('services.xml'); $loader->load('plugins.xml'); + // Register default services foreach ($config['classes'] as $service => $class) { if (!empty($class)) { $container->register(sprintf('httplug.%s.default', $service), $class); @@ -49,14 +51,27 @@ public function load(array $configs, ContainerBuilder $container) $container->setAlias(sprintf('httplug.%s', $type), $id); } - $this->configurePlugins($container, $config['plugins']); - $serviceIds = $this->configureClients($container, $config); - $autoServiceIds = $this->configureAutoDiscoveryClients($container, $config); + // Configure toolbar + if ($config['toolbar']['enabled']) { + $loader->load('data-collector.xml'); + + if (!empty($config['toolbar']['formatter'])) { + // Add custom formatter + $container + ->getDefinition('httplug.collector.debug_collector') + ->replaceArgument(0, new Reference($config['toolbar']['formatter'])) + ; + } - $toolbar = is_bool($config['toolbar']['enabled']) ? $config['toolbar']['enabled'] : $container->hasParameter('kernel.debug') && $container->getParameter('kernel.debug'); - if ($toolbar) { - (new ProfilerExtension())->load($config, $container, array_unique(array_merge($serviceIds, $autoServiceIds))); + $container + ->getDefinition('httplug.formatter.full_http_message') + ->addArgument($config['toolbar']['captured_body_length']) + ; } + + $this->configurePlugins($container, $config['plugins']); + $this->configureClients($container, $config); + $this->configureAutoDiscoveryClients($container, $config); } /** @@ -64,12 +79,9 @@ public function load(array $configs, ContainerBuilder $container) * * @param ContainerBuilder $container * @param array $config - * - * @return array with client service names */ private function configureClients(ContainerBuilder $container, array $config) { - $serviceIds = []; $first = null; foreach ($config['clients'] as $name => $arguments) { @@ -78,7 +90,7 @@ private function configureClients(ContainerBuilder $container, array $config) $first = $name; } - $serviceIds[] = $this->configureClient($container, $name, $arguments); + $this->configureClient($container, $name, $arguments, $config['toolbar']['enabled']); } // If we have clients configured @@ -89,8 +101,6 @@ private function configureClients(ContainerBuilder $container, array $config) $container->setAlias('httplug.client.default', 'httplug.client.'.$first); } } - - return $serviceIds; } /** @@ -198,14 +208,29 @@ private function configureAuthentication(ContainerBuilder $container, array $con * @param ContainerBuilder $container * @param string $name * @param array $arguments - * - * @return string The service id of the client. + * @param bool $profiling */ - private function configureClient(ContainerBuilder $container, $name, array $arguments) + private function configureClient(ContainerBuilder $container, $name, array $arguments, $profiling) { $serviceId = 'httplug.client.'.$name; - $definition = $container->register($serviceId, DummyClient::class); - $definition->setFactory([PluginClientFactory::class, 'createPluginClient']) + + $pluginClientOptions = []; + + if ($profiling) { + // Tell the plugin journal what plugins we used + $container + ->getDefinition('httplug.collector.plugin_journal') + ->addMethodCall('setPlugins', [$name, $arguments['plugins']]) + ; + + $debugPluginServiceId = $this->registerDebugPlugin($container, $serviceId); + + $pluginClientOptions['debug_plugins'] = [new Reference($debugPluginServiceId)]; + } + + $container + ->register($serviceId, DummyClient::class) + ->setFactory([PluginClientFactory::class, 'createPluginClient']) ->addArgument( array_map( function ($id) { @@ -216,31 +241,30 @@ function ($id) { ) ->addArgument(new Reference($arguments['factory'])) ->addArgument($arguments['config']) - ->addArgument([]) + ->addArgument($pluginClientOptions) ; - // Tell the plugin journal what plugins we used - $container->getDefinition('httplug.collector.plugin_journal') - ->addMethodCall('setPlugins', [$name, $arguments['plugins']]); /* * Decorate the client with clients from client-common */ if ($arguments['flexible_client']) { - $container->register($serviceId.'.flexible', FlexibleHttpClient::class) + $container + ->register($serviceId.'.flexible', FlexibleHttpClient::class) ->addArgument(new Reference($serviceId.'.flexible.inner')) ->setPublic(false) - ->setDecoratedService($serviceId); + ->setDecoratedService($serviceId) + ; } if ($arguments['http_methods_client']) { - $container->register($serviceId.'.http_methods', HttpMethodsClient::class) + $container + ->register($serviceId.'.http_methods', HttpMethodsClient::class) ->setArguments([new Reference($serviceId.'.http_methods.inner'), new Reference('httplug.message_factory')]) ->setPublic(false) - ->setDecoratedService($serviceId); + ->setDecoratedService($serviceId) + ; } - - return $serviceId; } /** @@ -249,45 +273,44 @@ function ($id) { * * @param ContainerBuilder $container * @param array $config - * - * @return array of service ids. */ private function configureAutoDiscoveryClients(ContainerBuilder $container, array $config) { - $serviceIds = []; - $httpClient = $config['discovery']['client']; + if (!empty($httpClient)) { if ($httpClient === 'auto') { $httpClient = $this->registerAutoDiscoverableClient( $container, 'auto_discovered_client', - [HttpClientDiscovery::class, 'find'] + [HttpClientDiscovery::class, 'find'], + $config['toolbar']['enabled'] ); } - $serviceIds[] = $httpClient; $httpClient = new Reference($httpClient); } $asyncHttpClient = $config['discovery']['async_client']; + if (!empty($asyncHttpClient)) { if ($asyncHttpClient === 'auto') { $asyncHttpClient = $this->registerAutoDiscoverableClient( $container, 'auto_discovered_async', - [HttpAsyncClientDiscovery::class, 'find'] + [HttpAsyncClientDiscovery::class, 'find'], + $config['toolbar']['enabled'] ); } - $serviceIds[] = $asyncHttpClient; - $asyncHttpClient = new Reference($httpClient); + + $asyncHttpClient = new Reference($asyncHttpClient); } - $container->getDefinition('httplug.strategy') + $container + ->getDefinition('httplug.strategy') ->addArgument($httpClient) - ->addArgument($asyncHttpClient); - - return $serviceIds; + ->addArgument($asyncHttpClient) + ; } /** @@ -296,17 +319,58 @@ private function configureAutoDiscoveryClients(ContainerBuilder $container, arra * @param ContainerBuilder $container * @param string $name * @param callable $factory + * @param bool $profiling * * @return string service id */ - private function registerAutoDiscoverableClient(ContainerBuilder $container, $name, $factory) + private function registerAutoDiscoverableClient(ContainerBuilder $container, $name, $factory, $profiling) { $serviceId = 'httplug.auto_discovery.'.$name; - $definition = $container->register($serviceId, DummyClient::class); - $definition + + $pluginClientOptions = []; + + if ($profiling) { + $debugPluginServiceId = $this->registerDebugPlugin($container, $serviceId); + + $pluginClientOptions['debug_plugins'] = [new Reference($debugPluginServiceId)]; + } + + $container + ->register($serviceId, DummyClient::class) ->setFactory([PluginClientFactory::class, 'createPluginClient']) - ->setArguments([[], $factory, [], []]); + ->setArguments([[], $factory, [], $pluginClientOptions]) + ; return $serviceId; } + + /** + * Create a new plugin service for this client. + * + * @param ContainerBuilder $container + * @param string $serviceId + * + * @return string + */ + private function registerDebugPlugin(ContainerBuilder $container, $serviceId) + { + $serviceIdDebugPlugin = $serviceId.'.debug_plugin'; + + $container + ->register($serviceIdDebugPlugin, DebugPlugin::class) + ->addArgument(new Reference('httplug.collector.debug_collector')) + ->addArgument(substr($serviceId, strrpos($serviceId, '.') + 1)) + ->setPublic(false) + ; + + return $serviceIdDebugPlugin; + } + + /** + * {@inheritdoc} + */ + public function getConfiguration(array $config, ContainerBuilder $container) + { + return new Configuration($container->getParameter('kernel.debug')); + } } diff --git a/DependencyInjection/ProfilerExtension.php b/DependencyInjection/ProfilerExtension.php deleted file mode 100644 index 4ee235a3..00000000 --- a/DependencyInjection/ProfilerExtension.php +++ /dev/null @@ -1,85 +0,0 @@ - - * @author Tobias Nyholm - */ -class ProfilerExtension -{ - /** - * @param array $config Processed configuration - * @param ContainerBuilder $container - * @param array $clientServiceIds ids of all clients - */ - public function load(array $config, ContainerBuilder $container, array $clientServiceIds) - { - $loader = new XmlFileLoader($container, new FileLocator(__DIR__.'/../Resources/config')); - $loader->load('data-collector.xml'); - - if (!empty($config['toolbar']['formatter'])) { - // Add custom formatter - $container->getDefinition('httplug.collector.debug_collector') - ->replaceArgument(0, new Reference($config['toolbar']['formatter'])); - } - - $container->getDefinition('httplug.formatter.full_http_message') - ->addArgument($config['toolbar']['captured_body_length']); - - foreach ($clientServiceIds as $clientId) { - $pluginClientDefinition = $container->getDefinition($clientId); - $serviceIdDebugPlugin = $this->registerDebugPlugin($container, $clientId); - - $argument = $this->mergeDebugPluginArguments([new Reference($serviceIdDebugPlugin)], $pluginClientDefinition->getArgument(3)); - $pluginClientDefinition->replaceArgument(3, $argument); - } - } - - /** - * Create a new plugin service for this client. - * - * @param ContainerBuilder $container - * @param string $name - * - * @return string - */ - private function registerDebugPlugin(ContainerBuilder $container, $name) - { - $serviceIdDebugPlugin = $name.'.debug_plugin'; - $container->register($serviceIdDebugPlugin, DebugPlugin::class) - ->addArgument(new Reference('httplug.collector.debug_collector')) - ->addArgument(substr($name, strrpos($name, '.') + 1)) - ->setPublic(false); - - return $serviceIdDebugPlugin; - } - - /** - * @param array $newArgument - * @param array $existing - * - * @return array - */ - private function mergeDebugPluginArguments(array $newArgument, array $existing = []) - { - if (empty($existing)) { - $mergedArgument = ['debug_plugins' => $newArgument]; - } elseif (empty($existing['debug_plugins'])) { - $mergedArgument['debug_plugins'] = $newArgument; - } else { - $mergedArgument['debug_plugins'] = array_merge($existing['debug_plugins'], $newArgument); - } - - return $mergedArgument; - } -} diff --git a/Resources/config/data-collector.xml b/Resources/config/data-collector.xml index fa2d8f07..d42e93e5 100644 --- a/Resources/config/data-collector.xml +++ b/Resources/config/data-collector.xml @@ -4,6 +4,8 @@ xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd"> + + diff --git a/Resources/config/services.xml b/Resources/config/services.xml index 0fb9f5dc..b5ad3efc 100644 --- a/Resources/config/services.xml +++ b/Resources/config/services.xml @@ -8,8 +8,6 @@ - - diff --git a/Tests/Resources/Fixtures/config/toolbar_auto.yml b/Tests/Resources/Fixtures/config/toolbar_auto.yml new file mode 100644 index 00000000..75ba9668 --- /dev/null +++ b/Tests/Resources/Fixtures/config/toolbar_auto.yml @@ -0,0 +1,3 @@ +httplug: + toolbar: + enabled: auto diff --git a/Tests/Unit/DependencyInjection/ConfigurationTest.php b/Tests/Unit/DependencyInjection/ConfigurationTest.php index 15764cbe..5fd30e15 100644 --- a/Tests/Unit/DependencyInjection/ConfigurationTest.php +++ b/Tests/Unit/DependencyInjection/ConfigurationTest.php @@ -11,6 +11,8 @@ */ class ConfigurationTest extends AbstractExtensionConfigurationTestCase { + private $debug = true; + protected function getContainerExtension() { return new HttplugExtension(); @@ -18,7 +20,7 @@ protected function getContainerExtension() protected function getConfiguration() { - return new Configuration(); + return new Configuration($this->debug); } public function testEmptyConfiguration() @@ -38,7 +40,7 @@ public function testEmptyConfiguration() ], 'clients' => [], 'toolbar' => [ - 'enabled' => 'auto', + 'enabled' => true, 'formatter' => null, 'captured_body_length' => 0, ], @@ -93,6 +95,7 @@ public function testEmptyConfiguration() 'config/empty.yml', 'config/empty.xml', 'config/empty.php', + 'config/toolbar_auto.yml', ]); foreach ($formats as $format) { diff --git a/Tests/Unit/DependencyInjection/HttplugExtensionTest.php b/Tests/Unit/DependencyInjection/HttplugExtensionTest.php index 54a2bd8f..2e0484d5 100644 --- a/Tests/Unit/DependencyInjection/HttplugExtensionTest.php +++ b/Tests/Unit/DependencyInjection/HttplugExtensionTest.php @@ -11,6 +11,13 @@ */ class HttplugExtensionTest extends AbstractExtensionTestCase { + protected function setUp() + { + parent::setUp(); + + $this->setParameter('kernel.debug', true); + } + protected function getContainerExtensions() { return [ From e25ca31faeb182449342df7c95161b095c4ae3b6 Mon Sep 17 00:00:00 2001 From: Tobias Nyholm Date: Sun, 24 Jul 2016 12:25:17 +0200 Subject: [PATCH 7/7] added note about plugin client --- CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ccf6681f..bbbd0d38 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,11 +3,14 @@ ## UNRELEASED +### Changed + +- All clients are registered with the PluginClient. (even in production) + ### Deprecated - `auto` value in `toolbar.enabled` config - ## 1.2.2 - 2016-07-19 ### Fixed