Drupal8.x and Symfony/Aura – The Importance of Testing and Decoupling: Part VII Series

Screenshot 2014-05-29 03.57.34

So all started with an issue on github https://github.com/symfony/symfony/pull/10920. You don’t have to turn there, basically one core Drupal developer asked about whether we at Symfony wanted some properties changed to protected so he can override them while extending our Drupal custom YamlFileLoader.

This YamlFileLoader is intended to be used as a loader of configuration coming from a file. The job is simple, pass a file, namely a yaml file, and load this specified configuration into the container or container builder which extends the container. The container is passed via the constructor, so the loader pretty much eats up the container builder and starts actually plugging the service or parameter definitions inside this container class.

Simple huh? Well, that task in Symfony is coupled. If you take a look at our YamlFileLoader implementation you will see that the DI class needs not only the Language component! but also the Config component. You cannot use the DI component without composer installing also all the files of these other components. Probably we should be directed to Pimple if we want less coupling. But think again, you cannot use the power of the DI on Symfony alone. If Symfony wants to thrive with more frameworks to integrate it into their core I believe we need to ultimately make decouple interfaces and implementations be true inside our components.

There are probably big disagreements as to the coupling of DependencyInjection component with Config component and even Language component. Things in Symfony when talking about extending it for your own custom in-house framework are not looking so good if you don’t have a cold head for DDD or staying with a mind that says “framework is an implementation detail”. Definitely is easier to follow upstream frameworks that are less coupled, or actually no coupled at all.

Aura2 framework, for instance, does not even need a composer install to run the tests. This is total proof that is totally decoupled, composer becomes useless at that level. It is just amazing. The autoload becomes totally hand made and optimized, that is composer autogenerated autoload is not needed if you are just using the library without composer, which you can. Because the component is decoupled, not only the framework component reaches a 100% test coverage, but the test running flies in no time. You would laugh at how much time it takes to run the whole test suite, yeah ridiculously shorter than the Symfony suite.

But let’s talk about the current state of how to accomplish things in spite of coupling.

So in the ticket above mentioned in github, the solution was not switch the visibility of properties. I believe if we require visibility change in our methods or properties then we should at least smell bad practices. And we need to correct or understand there is a use case we did not envision. For that we need to break BC or try not to, but if we have to we have to.

In the above case we at Drupal created a totally decoupled custom YamlFileLoader, maybe wrongly formatted with Drupal CS :), but it is there. However it had no tests until now. Here is where the meat comes in, “test all things”! We should test especially such things as this because it will help us keep track of upstream changes.

We have tests coming from the Symfony YamlFileLoader.
We have features described there, if there is any feature not described there that we rely on, we should ask for tests to be written!
We should then copy tests, not implementations. If we cannot use Symfony classes, then we should at least just use Symfony interfaces if they exist and implement. We should demand interfaces for things that we implement that are paramount in the way things are put together in Symfony. And when we provide our own implementations that are different than Symfony we should write tests to verify the behavior we have is up to date with new improvements or modifications upstream coming straight from tests, not from implementations necessarily.

I am glad that things like https://github.com/symfony/symfony/pull/10790/files were actually closed and never accepted. In other cases optimizations are good in code when it is well tested https://github.com/symfony/symfony/pull/10312/files. Never ever fall into the trap of optimize code that is not well tested. I say sacrifice readability in well tested code alone, never sacrifice readability in code that is not well tested.

This is not the first time that Drupal asks for interfaces. Asking for interfaces and decoupling is a good idea and it was demonstrated in https://github.com/symfony/symfony/pull/6096 successfully.

It is also not the first time that Drupal has pushed Symfony to write tests where it lacked tests https://github.com/symfony/symfony/pull/10536.

As a community, we :) Drupal, need a lot to learn about testing and we are on our way, we should also change our mindset from bugfixing to send failing tests for PRs and developing core with TDD in mind. We can benefit others when doing tests, we may not have in the past even though our changes were good https://github.com/symfony/symfony/pull/10195/files but we can change.

The power of questions https://github.com/symfony/symfony/issues/9595 on the Open Source world is out there. And this is why I am insisting Drupal moves to Github. It is no different and there is no excuse. I know there maybe a lot of fear and even trolls and angry people. We will try to make everybody happy, but ultimately Dries need to call it and I hope he will.
It is ok if you disagree. I am not about that, I am about code and passion :)

Totally undeserving much favor,

your friend @cordoval.

<?php
 
namespace Drupal\Tests\Core\DependencyInjection;
 
use Drupal\Core\DependencyInjection\YamlFileLoader;
use Drupal\Core\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Reference;
 
class YamlFileLoaderTest extends \PHPUnit_Framework_TestCase
{
    public function testLoadFile()
    {
        $loader = new YamlFileLoader($container = new ContainerBuilder());
        $r = new \ReflectionObject($loader);
        $m = $r->getMethod('loadFile');
        $m->setAccessible(true);
 
        foreach (array('nonvalid1', 'nonvalid2') as $fixture) {
            try {
                $m->invoke($loader, __DIR__.'/Fixtures/yaml/'.$fixture.'.yml');
                $this->fail('->loadFile() throws an InvalidArgumentException if the loaded file does not validate');
            } catch (\Exception $e) {
                $this->assertInstanceOf('\InvalidArgumentException', $e, '->load() throws an InvalidArgumentException if the loaded file does not validate');
                if ('nonvalid1' === $fixture) {
                    $this->assertStringMatchesFormat('The service file "%s" is not valid: it contains invalid keys %s. Services have to be added under "services" and Parameters under "parameters".', $e->getMessage(), '->load() throws an InvalidArgumentException if the loaded file does not validate');
                }
                if ('nonvalid2' === $fixture) {
                    $this->assertStringMatchesFormat('The service file "%s" is not valid: it is not an array.', $e->getMessage(), '->load() throws an InvalidArgumentException if the loaded file does not validate');
                }
            }
        }
    }
 
    public function testLoadParameters()
    {
        $loader = new YamlFileLoader($container = new ContainerBuilder());
        $loader->load(__DIR__.'/Fixtures/yaml/services2.yml');
        $this->assertEquals(array('foo' => 'bar', 'mixedcase' => array('MixedCaseKey' => 'value'), 'values' => array(true, false, 0, 1000.3), 'bar' => 'foo', 'escape' => '@escapeme', 'foo_bar' => new Reference('foo_bar')), $container->getParameterBag()->all(), '->load() converts YAML keys to lowercase');
    }
 
    public function testLoadServices()
    {
        $loader = new YamlFileLoader($container = new ContainerBuilder());
        $loader->load(__DIR__.'/Fixtures/yaml/services6.yml');
        $services = $container->getDefinitions();
        $this->assertTrue(isset($services['foo']), '->load() parses service elements');
        $this->assertEquals('Symfony\\Component\\DependencyInjection\\Definition', get_class($services['foo']), '->load() converts service element to Definition instances');
        $this->assertEquals('FooClass', $services['foo']->getClass(), '->load() parses the class attribute');
        $this->assertEquals('container', $services['scope.container']->getScope());
        $this->assertEquals('custom', $services['scope.custom']->getScope());
        $this->assertEquals('prototype', $services['scope.prototype']->getScope());
        $this->assertEquals('getInstance', $services['constructor']->getFactoryMethod(), '->load() parses the factory_method attribute');
        $this->assertEquals('%path%/foo.php', $services['file']->getFile(), '->load() parses the file tag');
        $this->assertEquals(array('foo', new Reference('foo'), array(true, false)), $services['arguments']->getArguments(), '->load() parses the argument tags');
        $this->assertEquals('sc_configure', $services['configurator1']->getConfigurator(), '->load() parses the configurator tag');
        $this->assertEquals(array(new Reference('baz'), 'configure'), $services['configurator2']->getConfigurator(), '->load() parses the configurator tag');
        $this->assertEquals(array('BazClass', 'configureStatic'), $services['configurator3']->getConfigurator(), '->load() parses the configurator tag');
        $this->assertEquals(array(array('setBar', array('foo', new Reference('foo'), array(true, false)))), $services['method_call2']->getMethodCalls(), '->load() parses the method_call tag');
        $this->assertEquals('baz_factory', $services['factory_service']->getFactoryService());
 
        $this->assertTrue($services['request']->isSynthetic(), '->load() parses the synthetic flag');
        $this->assertTrue($services['request']->isSynchronized(), '->load() parses the synchronized flag');
        $this->assertTrue($services['request']->isLazy(), '->load() parses the lazy flag');
 
        $aliases = $container->getAliases();
        $this->assertTrue(isset($aliases['alias_for_foo']), '->load() parses aliases');
        $this->assertEquals('foo', (string) $aliases['alias_for_foo'], '->load() parses aliases');
        $this->assertTrue($aliases['alias_for_foo']->isPublic());
        $this->assertTrue(isset($aliases['another_alias_for_foo']));
        $this->assertEquals('foo', (string) $aliases['another_alias_for_foo']);
        $this->assertFalse($aliases['another_alias_for_foo']->isPublic());
    }
 
    public function testNonArrayTagThrowsException()
    {
        $loader = new YamlFileLoader($container = new ContainerBuilder());
        try {
            $loader->load(__DIR__.'/Fixtures/yaml/badtag1.yml');
            $this->fail('->load() should throw an exception when the tags key of a service is not an array');
        } catch (\Exception $e) {
            $this->assertInstanceOf('\InvalidArgumentException', $e, '->load() throws an InvalidArgumentException if the tags key is not an array');
            $this->assertStringStartsWith('Parameter "tags" must be an array for service', $e->getMessage(), '->load() throws an InvalidArgumentException if the tags key is not an array');
        }
    }
 
    public function testTagWithoutNameThrowsException()
    {
        $loader = new YamlFileLoader($container = new ContainerBuilder());
        try {
            $loader->load(__DIR__.'/Fixtures/yaml/badtag2.yml');
            $this->fail('->load() should throw an exception when a tag is missing the name key');
        } catch (\Exception $e) {
            $this->assertInstanceOf('\InvalidArgumentException', $e, '->load() throws an InvalidArgumentException if a tag is missing the name key');
            $this->assertStringStartsWith('A "tags" entry is missing a "name" key for service ', $e->getMessage(), '->load() throws an InvalidArgumentException if a tag is missing the name key');
        }
    }
 
    public function testTagWithAttributeArrayThrowsException()
    {
        $loader = new YamlFileLoader($container = new ContainerBuilder());
        try {
            $loader->load(__DIR__.'/Fixtures/yaml/badtag3.yml');
            $this->fail('->load() should throw an exception when a tag-attribute is not a scalar');
        } catch (\Exception $e) {
            $this->assertInstanceOf('\InvalidArgumentException', $e, '->load() throws an InvalidArgumentException if a tag-attribute is not a scalar');
            $this->assertStringStartsWith('A "tags" attribute must be of a scalar-type for service "foo_service", tag "foo", attribute "bar"', $e->getMessage(), '->load() throws an InvalidArgumentException if a tag-attribute is not a scalar');
        }
    }
}

2 thoughts on “Drupal8.x and Symfony/Aura – The Importance of Testing and Decoupling: Part VII Series

  1. Pingback: Drupal8.x and Symfony/Aura – DrupalCon 2014 talk video and slides | Craft It Online!

Leave a Reply

Your email address will not be published. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>