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] 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 [