From 4463f6c38f0e7ce362d9ece92cac7425e4f5f673 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1rk=20S=C3=A1gi-Kaz=C3=A1r?= Date: Mon, 29 Feb 2016 10:14:58 +0100 Subject: [PATCH] Remove discovery dependency, fixes #40, closes #54 POC how to not be dependent on discovery Fix unit tests Fix factory method Remove duplicated service Move discovery to compiler pass Applied fixes from StyleCI Fix puli executable Use class constant Fix puli command order Fix lowest dependency issue Fix HHVM tests Improve tests --- .gitignore | 1 + .travis.yml | 1 + .../Compiler/DiscoveryPass.php | 85 ++++++++++++++ DependencyInjection/HttplugExtension.php | 4 +- HttplugBundle.php | 8 ++ HttplugFactory.php | 107 ++++++++++++++++++ Resources/config/discovery.xml | 25 ---- Tests/Resources/app/AppKernel.php | 8 +- .../Compiler/DiscoveryPassTest.php | 76 +++++++++++++ .../HttplugExtensionTest.php | 17 --- composer.json | 7 +- 11 files changed, 291 insertions(+), 48 deletions(-) create mode 100644 DependencyInjection/Compiler/DiscoveryPass.php create mode 100644 HttplugFactory.php delete mode 100644 Resources/config/discovery.xml create mode 100644 Tests/Unit/DependencyInjection/Compiler/DiscoveryPassTest.php diff --git a/.gitignore b/.gitignore index 97015cb1..d8e81907 100644 --- a/.gitignore +++ b/.gitignore @@ -5,3 +5,4 @@ composer.lock phpspec.yml phpunit.xml puli.json +puli.phar diff --git a/.travis.yml b/.travis.yml index 31e46b6b..2af3d2dd 100644 --- a/.travis.yml +++ b/.travis.yml @@ -34,6 +34,7 @@ before_install: - travis_retry composer self-update install: + - wget https://github.com/puli/cli/releases/download/1.0.0-beta10/puli.phar && chmod +x puli.phar - composer require symfony/symfony:${SYMFONY_VERSION} --no-update - travis_retry composer update ${COMPOSER_FLAGS} --prefer-source --no-interaction diff --git a/DependencyInjection/Compiler/DiscoveryPass.php b/DependencyInjection/Compiler/DiscoveryPass.php new file mode 100644 index 00000000..579a03b5 --- /dev/null +++ b/DependencyInjection/Compiler/DiscoveryPass.php @@ -0,0 +1,85 @@ + + */ +final class DiscoveryPass implements CompilerPassInterface +{ + /** + * Fallback services and classes. + * + * @var array + */ + private $services = [ + 'client' => HttpClient::class, + 'message_factory' => MessageFactory::class, + 'uri_factory' => UriFactory::class, + 'stream_factory' => StreamFactory::class, + ]; + + /** + * {@inheritdoc} + */ + public function process(ContainerBuilder $container) + { + $useDiscovery = false; + + foreach ($this->services as $service => $class) { + $serviceId = sprintf('httplug.%s.default', $service); + + if (false === $container->has($serviceId)) { + // Register and create factory for the first time + if (false === $useDiscovery) { + $this->registerFactory($container); + + $factory = [ + new Reference('httplug.factory'), + 'find', + ]; + + $useDiscovery = true; + } + + $definition = $container->register($serviceId, $class); + + $definition->setFactory($factory); + $definition->addArgument($class); + } + } + } + + /** + * @param ContainerBuilder $container + * + * @throws RuntimeException + */ + private function registerFactory(ContainerBuilder $container) + { + if (false === $container->has('puli.discovery')) { + throw new RuntimeException( + 'You need to install puli/symfony-bundle or add configuration at httplug.classes in order to use this bundle. Refer to http://docs.php-http/en/latest/integrations/index.html' + ); + } + + $definition = $container->register('httplug.factory', HttplugFactory::class); + + $definition + ->addArgument(new Reference('puli.discovery')) + ->setPublic(false) + ; + } +} diff --git a/DependencyInjection/HttplugExtension.php b/DependencyInjection/HttplugExtension.php index 82f86df7..ca41fd1f 100644 --- a/DependencyInjection/HttplugExtension.php +++ b/DependencyInjection/HttplugExtension.php @@ -33,7 +33,6 @@ public function load(array $configs, ContainerBuilder $container) $loader->load('services.xml'); $loader->load('plugins.xml'); - $loader->load('discovery.xml'); $enabled = is_bool($config['toolbar']['enabled']) ? $config['toolbar']['enabled'] : $container->hasParameter('kernel.debug') && $container->getParameter('kernel.debug'); if ($enabled) { @@ -48,7 +47,6 @@ public function load(array $configs, ContainerBuilder $container) foreach ($config['classes'] as $service => $class) { if (!empty($class)) { - $container->removeDefinition(sprintf('httplug.%s.default', $service)); $container->register(sprintf('httplug.%s.default', $service), $class); } } @@ -56,6 +54,7 @@ public function load(array $configs, ContainerBuilder $container) foreach ($config['main_alias'] as $type => $id) { $container->setAlias(sprintf('httplug.%s', $type), $id); } + $this->configurePlugins($container, $config['plugins']); $this->configureClients($container, $config); } @@ -174,7 +173,6 @@ private function configurePluginByName($name, Definition $definition, array $con /** * @param ContainerBuilder $container - * @param Definition $parent * @param array $config */ private function configureAuthentication(ContainerBuilder $container, array $config) diff --git a/HttplugBundle.php b/HttplugBundle.php index 082d32f4..b5402c52 100644 --- a/HttplugBundle.php +++ b/HttplugBundle.php @@ -2,6 +2,8 @@ namespace Http\HttplugBundle; +use Http\HttplugBundle\DependencyInjection\Compiler\DiscoveryPass; +use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\HttpKernel\Bundle\Bundle; /** @@ -10,4 +12,10 @@ */ class HttplugBundle extends Bundle { + public function build(ContainerBuilder $container) + { + parent::build($container); + + $container->addCompilerPass(new DiscoveryPass()); + } } diff --git a/HttplugFactory.php b/HttplugFactory.php new file mode 100644 index 00000000..fe8922da --- /dev/null +++ b/HttplugFactory.php @@ -0,0 +1,107 @@ + + */ +final class HttplugFactory +{ + /** + * @var Discovery + */ + private $discovery; + + /** + * @param Discovery $discovery + */ + public function __construct(Discovery $discovery) + { + $this->discovery = $discovery; + } + + /** + * Creates a class of type. + * + * @param string $type + * + * @return object + */ + public function find($type) + { + $class = $this->findOneByType($type); + + // TODO: use doctrine instantiator? + return new $class(); + } + + /** + * Finds a class of type and resolves optional dependency conditions. + * + * @param string $type + * + * @return string + * + * @throws \RuntimeException if no class is found. + */ + private function findOneByType($type) + { + /** @var ClassBinding[] $bindings */ + $bindings = $this->discovery->findBindings( + $type, + Expr::isInstanceOf('Puli\Discovery\Binding\ClassBinding') + ); + + foreach ($bindings as $binding) { + if ($binding->hasParameterValue('depends')) { + $dependency = $binding->getParameterValue('depends'); + + if (false === $this->evaluateCondition($dependency)) { + continue; + } + } + + return $binding->getClassName(); + } + + throw new \RuntimeException(sprintf('Class binding of type "%s" is not found', $type)); + } + + /** + * Evaulates conditions to boolean. + * + * @param mixed $condition + * + * @return bool + * + * TODO: review this method + */ + protected function evaluateCondition($condition) + { + if (is_string($condition)) { + // Should be extended for functions, extensions??? + return class_exists($condition); + } elseif (is_callable($condition)) { + return $condition(); + } elseif (is_bool($condition)) { + return $condition; + } elseif (is_array($condition)) { + $evaluatedCondition = true; + + // Immediately stop execution if the condition is false + for ($i = 0; $i < count($condition) && false !== $evaluatedCondition; ++$i) { + $evaluatedCondition &= $this->evaluateCondition($condition[$i]); + } + + return $evaluatedCondition; + } + + return false; + } +} diff --git a/Resources/config/discovery.xml b/Resources/config/discovery.xml deleted file mode 100644 index 0969e8ad..00000000 --- a/Resources/config/discovery.xml +++ /dev/null @@ -1,25 +0,0 @@ - - - - - - - - - - - - - - - - - - - diff --git a/Tests/Resources/app/AppKernel.php b/Tests/Resources/app/AppKernel.php index ea794e3c..9cc31844 100644 --- a/Tests/Resources/app/AppKernel.php +++ b/Tests/Resources/app/AppKernel.php @@ -10,10 +10,16 @@ class AppKernel extends Kernel */ public function registerBundles() { - return [ + $bundles = [ new \Symfony\Bundle\FrameworkBundle\FrameworkBundle(), new \Http\HttplugBundle\HttplugBundle(), ]; + + if (false === defined('HHVM_VERSION')) { + $bundles[] = new \Puli\SymfonyBundle\PuliBundle(); + } + + return $bundles; } /** diff --git a/Tests/Unit/DependencyInjection/Compiler/DiscoveryPassTest.php b/Tests/Unit/DependencyInjection/Compiler/DiscoveryPassTest.php new file mode 100644 index 00000000..f39167af --- /dev/null +++ b/Tests/Unit/DependencyInjection/Compiler/DiscoveryPassTest.php @@ -0,0 +1,76 @@ + + */ +final class DiscoveryPassTest extends AbstractCompilerPassTestCase +{ + protected function registerCompilerPass(ContainerBuilder $container) + { + $container->addCompilerPass(new DiscoveryPass()); + } + + public function testDiscoveryFallbacks() + { + $this->setDefinition('puli.discovery', new Definition('Puli\Discovery\Api\Discovery')); + + $this->compile(); + + $this->assertContainerBuilderHasService('httplug.factory', HttplugFactory::class); + + $this->assertContainerBuilderHasService('httplug.client.default', HttpClient::class); + $this->assertContainerBuilderHasService('httplug.message_factory.default', MessageFactory::class); + $this->assertContainerBuilderHasService('httplug.uri_factory.default', UriFactory::class); + $this->assertContainerBuilderHasService('httplug.stream_factory.default', StreamFactory::class); + } + + public function testDiscoveryPartialFallbacks() + { + $this->setDefinition('puli.discovery', new Definition('Puli\Discovery\Api\Discovery')); + $this->setDefinition('httplug.client.default', new Definition('Http\Adapter\Guzzle6\Client')); + + $this->compile(); + + $this->assertContainerBuilderHasService('httplug.factory', HttplugFactory::class); + + $this->assertContainerBuilderHasService('httplug.client.default', 'Http\Adapter\Guzzle6\Client'); + $this->assertContainerBuilderHasService('httplug.message_factory.default', MessageFactory::class); + $this->assertContainerBuilderHasService('httplug.uri_factory.default', UriFactory::class); + $this->assertContainerBuilderHasService('httplug.stream_factory.default', StreamFactory::class); + } + + public function testNoDiscoveryFallbacks() + { + $this->setDefinition('httplug.client.default', new Definition(HttpClient::class)); + $this->setDefinition('httplug.message_factory.default', new Definition(MessageFactory::class)); + $this->setDefinition('httplug.uri_factory.default', new Definition(UriFactory::class)); + $this->setDefinition('httplug.stream_factory.default', new Definition(StreamFactory::class)); + + $this->compile(); + + $this->assertContainerBuilderNotHasService('httplug.factory'); + } + + /** + * Overridden test as we have dependencies in this compiler pass. + * + * @test + * @expectedException \RuntimeException + */ + public function compilation_should_not_fail_with_empty_container() + { + $this->compile(); + } +} diff --git a/Tests/Unit/DependencyInjection/HttplugExtensionTest.php b/Tests/Unit/DependencyInjection/HttplugExtensionTest.php index 8b492188..b88d795b 100644 --- a/Tests/Unit/DependencyInjection/HttplugExtensionTest.php +++ b/Tests/Unit/DependencyInjection/HttplugExtensionTest.php @@ -24,11 +24,6 @@ public function testConfigLoadDefault() foreach (['client', 'message_factory', 'uri_factory', 'stream_factory'] as $type) { $this->assertContainerBuilderHasAlias("httplug.$type", "httplug.$type.default"); } - - $this->assertContainerBuilderHasService('httplug.client.default', 'Http\Client\HttpClient'); - $this->assertContainerBuilderHasService('httplug.message_factory.default', 'Http\Message\MessageFactory'); - $this->assertContainerBuilderHasService('httplug.uri_factory.default', 'Http\Message\UriFactory'); - $this->assertContainerBuilderHasService('httplug.stream_factory.default', 'Http\Message\StreamFactory'); } public function testConfigLoadClass() @@ -39,14 +34,7 @@ public function testConfigLoadClass() ], ]); - foreach (['client', 'message_factory', 'uri_factory', 'stream_factory'] as $type) { - $this->assertContainerBuilderHasAlias("httplug.$type", "httplug.$type.default"); - } - $this->assertContainerBuilderHasService('httplug.client.default', 'Http\Adapter\Guzzle6\Client'); - $this->assertContainerBuilderHasService('httplug.message_factory.default', 'Http\Message\MessageFactory'); - $this->assertContainerBuilderHasService('httplug.uri_factory.default', 'Http\Message\UriFactory'); - $this->assertContainerBuilderHasService('httplug.stream_factory.default', 'Http\Message\StreamFactory'); } public function testConfigLoadService() @@ -63,10 +51,5 @@ public function testConfigLoadService() foreach (['client', 'message_factory', 'uri_factory', 'stream_factory'] as $type) { $this->assertContainerBuilderHasAlias("httplug.$type", "my_{$type}_service"); } - - $this->assertContainerBuilderHasService('httplug.client.default', 'Http\Client\HttpClient'); - $this->assertContainerBuilderHasService('httplug.message_factory.default', 'Http\Message\MessageFactory'); - $this->assertContainerBuilderHasService('httplug.uri_factory.default', 'Http\Message\UriFactory'); - $this->assertContainerBuilderHasService('httplug.stream_factory.default', 'Http\Message\StreamFactory'); } } diff --git a/composer.json b/composer.json index 2b56d21b..e99419f4 100644 --- a/composer.json +++ b/composer.json @@ -13,7 +13,6 @@ ], "require": { "php": ">=5.5", - "php-http/discovery": "^0.8", "php-http/client-implementation": "^1.0", "php-http/message-factory": "^1.0.2", "php-http/plugins": "^1.0", @@ -26,7 +25,11 @@ "puli/composer-plugin": "1.0.0-beta9", "symfony/symfony": "^2.7|^3.0", "polishsymfonycommunity/symfony-mocker-container": "^1.0", - "matthiasnoback/symfony-dependency-injection-test": "^0.7" + "matthiasnoback/symfony-dependency-injection-test": "^0.7", + "puli/symfony-bundle": "^1.0@beta,>1.0.0-beta7" + }, + "suggest": { + "puli/symfony-bundle": "Used for automatic service discovery" }, "autoload": { "psr-4": {