-
Notifications
You must be signed in to change notification settings - Fork 50
[WIP] Remove discovery dependency #55
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,3 +5,4 @@ composer.lock | |
phpspec.yml | ||
phpunit.xml | ||
puli.json | ||
puli.phar |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
<?php | ||
|
||
namespace Http\HttplugBundle\DependencyInjection\Compiler; | ||
|
||
use Http\Client\HttpClient; | ||
use Http\HttplugBundle\HttplugFactory; | ||
use Http\Message\MessageFactory; | ||
use Http\Message\StreamFactory; | ||
use Http\Message\UriFactory; | ||
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface; | ||
use Symfony\Component\DependencyInjection\ContainerBuilder; | ||
use Symfony\Component\DependencyInjection\Exception\RuntimeException; | ||
use Symfony\Component\DependencyInjection\Reference; | ||
|
||
/** | ||
* Adds fallback and discovery services. | ||
* | ||
* @author Márk Sági-Kazár <mark.sagikazar@gmail.com> | ||
*/ | ||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this? if we need several factories, will this not prevent the creation of the other factories? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The factory service itself is the same. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, ok, got it. but then we could call that variable simple puliFactoryLoaded instead of "useDiscovery" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wow, you are too quick.. |
||
$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) | ||
; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this solution! |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
<?php | ||
|
||
namespace Http\HttplugBundle; | ||
|
||
use Puli\Discovery\Api\Discovery; | ||
use Puli\Discovery\Binding\ClassBinding; | ||
use Webmozart\Expression\Expr; | ||
|
||
/** | ||
* This class is a wrapper around Puli discovery. | ||
* | ||
* @author Márk Sági-Kazár <mark.sagikazar@gmail.com> | ||
*/ | ||
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; | ||
} | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to insert the PuliBundle before HttplugBundle? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, rethinking this: it does not, since all compilers are loaded after all extensions did. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
exactly, one of the principal reasons to use a compiler pass. you can even |
||
|
||
return $bundles; | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
<?php | ||
|
||
namespace Http\HttplugBundle\Tests\Unit\DependencyInjection\Compiler; | ||
|
||
use Http\Client\HttpClient; | ||
use Http\HttplugBundle\DependencyInjection\Compiler\DiscoveryPass; | ||
use Http\HttplugBundle\HttplugFactory; | ||
use Http\Message\MessageFactory; | ||
use Http\Message\StreamFactory; | ||
use Http\Message\UriFactory; | ||
use Matthias\SymfonyDependencyInjectionTest\PhpUnit\AbstractCompilerPassTestCase; | ||
use Symfony\Component\DependencyInjection\ContainerBuilder; | ||
use Symfony\Component\DependencyInjection\Definition; | ||
|
||
/** | ||
* @author Márk Sági-Kazár <mark.sagikazar@gmail.com> | ||
*/ | ||
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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this is a good idea. What are the symfony best practices for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I only know that Symfony best practices does not tell you anything about this. There is not event a cookbook entry about how you test configuration, extension or your compiler passes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well, unit tests should be straightforward. and the functional test is already covered if we do anything where we use the factories. so the problem here is that if we run this test we get an error, right? do we not install the puli things on travis, would this not work? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not about installation. In this test, a container is built with only this compiler pass. And there is a burnt-in test which tests the thing against an empty container. If there is any httplug service which is not defined nor discovery is installed, the intended behaviour is to get an exception during compile. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is discovery not installed on travis? if its optional, i would add a check and skip the tests if discovery is not available. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discovery is removed in this PR. Do you mean Puli? Yes, Puli is installed, but this test only tests this specific compiler pass. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry. jetlagged brain in action ;-) right. so i think we should have one test that makes sure we get a decent error message when the puli bundle is not available and nothing is configured explicitly. and 2 more tests, one for explicit configuration and one for when puli is available. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These cases are already tested. |
||
{ | ||
$this->compile(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping @dbu
We download puli.phar here. We do also require "puli/symfony-bundle"