Skip to content

[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

Merged
merged 1 commit into from
Mar 1, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ composer.lock
phpspec.yml
phpunit.xml
puli.json
puli.phar
1 change: 1 addition & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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"

- composer require symfony/symfony:${SYMFONY_VERSION} --no-update
- travis_retry composer update ${COMPOSER_FLAGS} --prefer-source --no-interaction

Expand Down
85 changes: 85 additions & 0 deletions DependencyInjection/Compiler/DiscoveryPass.php
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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The factory service itself is the same.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the $factory is a "httplug.factory". A service that works as a factory class for our *Factory classes. (MessageFactory, ClientFactory etc.. )

Copy link
Member

Choose a reason for hiding this comment

The 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)
;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this solution!

4 changes: 1 addition & 3 deletions DependencyInjection/HttplugExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -48,14 +47,14 @@ 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);
}
}

foreach ($config['main_alias'] as $type => $id) {
$container->setAlias(sprintf('httplug.%s', $type), $id);
}

$this->configurePlugins($container, $config['plugins']);
$this->configureClients($container, $config);
}
Expand Down Expand Up @@ -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)
Expand Down
8 changes: 8 additions & 0 deletions HttplugBundle.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

namespace Http\HttplugBundle;

use Http\HttplugBundle\DependencyInjection\Compiler\DiscoveryPass;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\HttpKernel\Bundle\Bundle;

/**
Expand All @@ -10,4 +12,10 @@
*/
class HttplugBundle extends Bundle
{
public function build(ContainerBuilder $container)
{
parent::build($container);

$container->addCompilerPass(new DiscoveryPass());
}
}
107 changes: 107 additions & 0 deletions HttplugFactory.php
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;
}
}
25 changes: 0 additions & 25 deletions Resources/config/discovery.xml

This file was deleted.

8 changes: 7 additions & 1 deletion Tests/Resources/app/AppKernel.php
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to insert the PuliBundle before HttplugBundle?
If the order is important we must document it.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

exactly, one of the principal reasons to use a compiler pass. you can even
in the extension class see which bundles are instantiated, but not their
config


return $bundles;
}

/**
Expand Down
76 changes: 76 additions & 0 deletions Tests/Unit/DependencyInjection/Compiler/DiscoveryPassTest.php
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()
Copy link
Member Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These cases are already tested.

{
$this->compile();
}
}
Loading