Drupal8.x and Symfony/Aura – DrupalCon 2014 talk video and slides

Series:

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');
        }
    }
}

Drupal8.x and Symfony – Interfaces and Serializer Component: Part VI Series

First a note on Interfaces. I did a regexp search on Drupal core code for

construct\((.*)Interface

in order to find out about some extension points and looking at the output start scanning which, where, and how some extension points were available. I started to look at this for situations where you would want to override some services that core provides. Not the usual case, but is always good to know how extensible the new Drupal can be.

I mean in the extreme of things, one should be able to bypass most of the stuff and run almost Symfony on a Drupal carcass. I still want to know if this is possible via say a module, because that would potentially be very good news for those coming from Symfony. Though we probably would leverage less from Drupal, we would gain the possibility of supporting a distribution that is Drupal and at the same time is Symfony or at best Symfonyc.

Ok now onto the serializer module. Notice I don’t say component. The serializer component is not in a Core folder but is under Component, however those classes are just 3 classes really, they are called Serialization and not Serializer. These are Json, PhpSerialize, and Yaml and they all implement SerializationInterface.

One one hand the difference and reason why there is a serializer module and a Serialization component is that Serialization component really does not use anything from Symfony except the Yaml dumper to serialize to Yaml. These 3 services are even registered into the container however they are never used, with the exception of PhpSerialize. PhpSerialize is also really a wrapper for PHP default serialization using the serialize and unserialize PHP native methods.

On the other hand the Serializer core module registers a whole architecture that inherits its basis from Symfony’s (`basic`) Serializer component. These module was ported almost a year ago and it is very complete. It has a complete service provider, compiler passes to register all encoders and normalizers, resolvers for detecting what type of objects are being serialized, and of course encoders and normalizers.

Here is a link and a snapshot of Drupal’s serializer module:

Screenshot 2014-05-28 00.53.19

The problem is that I couldn’t find a reference of usage of this module in Core or in other modules. I read a bit on the related tickets and saw that there is some integration going on but saw a bit out dated information or maybe it is work incomplete or stalled. The library, aka Serializer module, is good to use on its own, does not have a UI, and is very flexible.

It is very interesting how Drupal is using the Serializer. In the Symfony world, the serializer is not used much, because most people like to use the JMS Serializer which goes beyond with more features but hooks your whole implementation to a complexity level that in most cases you don’t want/need. I am using so far the regular basic serializer and extending it in my projects so that I don’t use JMS Serializer and I have good reasons because it keeps simple things simple.

I really comment the usage of Serializer in this Drupal module. It may not be useful to you outside of Drupal but it is an interesting case of adapting the Serializer component to your project.

Screenshot 2014-05-28 01.10.22

Drupal8.x and Symfony – Controller Practices: Part V Series

So today I ran into Drupal Controllers core component or folder. And I found this ContentFormControllerSubscriber under the Subscriber folder:

/**
 * Subscriber for setting wrapping form logic.
 */
class ContentFormControllerSubscriber implements EventSubscriberInterface, ContainerAwareInterface {
 
  use ContainerAwareTrait;

The trait here basically has the setter method to inject and class variable to hold the container. Something that is really a violation of DI into a service locator. See Paul Jones for more information on this. Right from the very beginning I am always suspicious of abuse on traits usage. It is actually a smell to me, or well most of the times. It is easy to detect this on developers :).

Taking a look at the constructor of this subscriber I saw it was injecting already the services needed:

  public function __construct(
    ClassResolverInterface $class_resolver,
    ControllerResolverInterface $controller_resolver,
    FormBuilderInterface $form_builder
  ) {
    $this->classResolver = $class_resolver;
    $this->controllerResolver = $controller_resolver;
    $this->formBuilder = $form_builder;
  }

So based on the request format, this subscriber will set the _controller field in the request. So it will of course get the request from the event dispatched, then detect whether this event is a form submission, then if it is proceeds to handle it as such, a form submission, and attempts to wrap and encapsulate everything inside a _content field which is the product of a specific controller invoked, the HtmlFormController.

In other words, if you submit a form in Drupal, this subscriber will take your request, identify it and put everything into the Request _content field.

In fewer words, for every form on your app it will end up processed under a _content field. The work of choosing which controller will handle your form has been simplified. You are not in control because Drupal wants you to reuse their form system.

Until here, nothing bad, however I see a direct instantiation of HtmlFormController:

  public function onRequestDeriveFormWrapper(GetResponseEvent $event) {
    $request = $event->getRequest();
 
    if ($form = $request->attributes->get('_form')) {
      $wrapper = new HtmlFormController($this->classResolver, $this->controllerResolver, $this->container, $form, $this->formBuilder);
      $request->attributes->set('_content', array($wrapper, 'getContentResult'));
    }
  }

What happened here, explains why this Subscriber had to use a trait and why it had to be injected the whole container. It is imo wrong to inject the container if you are just passing it, why the HtmlFormController was not rather injected as a service. I don’t know, I can only think there is still a good reason. In the case that the HtmlFormController would have been injected we would not have needed to inject or use any traits nor inject the whole container. The container is not used anywhere else, it is just passed straight as an argument to the HtmlFormController. The HtmlFormController is not used anywhere else but here. It was created mainly for this subscriber and to detect and channelize form submissions to Drupal’s form system.

Here is the service definition of our ContentFormControllerSubscriber:

  route_content_form_controller_subscriber:
    class: Drupal\Core\EventSubscriber\ContentFormControllerSubscriber
    arguments: ['@class_resolver', '@controller_resolver', '@form_builder']
    parent: container.trait
    tags:
      - { name: event_subscriber }

  container.trait:
    abstract: true
    calls:
      - [setContainer, ['@service_container']]

Seeing this does not cheer my spirits, but oh well, it is good to know that you could do this on Symfony service definitions. The DI component allows you to do things like that even though they will harm your design later on by basically making all your services that use this HtmlFormController or similar to have to use the dreaded trait and worst to inject the whole container.

At least let’s hope the container gets used inside HtmlFormController… nope! I tried to look hard and maybe it is used in some class that extends this, however, I could not find it. It just adorns the constructor of that class gets assigned and then that is it.

I also skim through lines saying:

* @todo Make this a trait in PHP 5.4.

At first I thought, let’s send a PR, however, I am not going to do it because I have started to dislike how traits are being used in Drupal core. Basically I don’t see them as a way to clean up classes. I see it as a smell. They could only be useful when they are better than interfaces, but in most cases good design uses very good interfaces so there is no need for traits, they should only go in implementations and where there is an actual use case with many methods, not just one setter and especially the setter container! Traits sometimes are a quick shortcut for not thinking hard enough on the refactor you are doing, once in another file and with the wrong name the code becomes blur. There are instances in which I would use them, but so far the usage I have seen in this codebase does not motivate me to do the same. I have seen Traits in other packages and they are fine, just that not always it is the case.

Let’s pick another one, AjaxController.

  controller.ajax:
    class: Drupal\Core\Controller\AjaxController
    arguments: ['@controller_resolver', '@ajax_response_renderer']

However oh surprise when we see the class:

class AjaxController implements ContainerAwareInterface {
 
  use ContainerAwareTrait;

It injects the container again, but never uses it and it is a service all well defined and round. I tried to look for usages of it and did not find one, maybe it is reserved to be extended or handled in a certain way, but let me point that the container injection did not seem as though it was thought well if it is not being used at all. Things are quick to inject and that is the problem. Doing damage is easy, repairing the damage is of course hard. Maybe the scope of things were sketched but not seen until implemented and that could explain many injections done. The job of each class has become more clear so it is now time to clean them up and get rid as much as possible of the container injection.

I hope I had not made many mistakes here and that the usages are justified, if so let me recant and correct me please. I would be thankful to learn from you.

Again totally undeserved to be flying to Austin or presenting a talk there, even working or even breathing with joy. Such grace!

your friend, hope this helps you! Feel free to support me! thanks!

Drupal8.x and Symfony – Asset Component: Part IV Series

Tonight I took a look at the Asset component. I was a bit reluctant because by the looks of it I thought of it being large, many files and folders. It has a lot of implementation specific classes and things relating to js that did not ring too much on the backend keyword.

However after spending some time on #irc channel #drupal-contribute I was aided by nod_ about the important efforts happening in Drupal currently. First of all I have to say there is one person that deserves a lot of attention and that is sdboyer. He had started the work of including Assetic way back almost more than a year ago. He has put so much work that after Robloach plugged assetic to the composer.json he, Sam, has been pounding it for so long that it is just outstanding. He has taken upon himself a very complex integration challenge.

Drupal is a mess in terms of assets. There are many things that range from the fact that its assets had a mechanism based on weights for being ordered, the business logic is a mess and procedural, it totally breaks encapsulation in early attempts to solve this problem, and more.

Sam explained all along well and there had been periods of time that he is so busy, almost months. He has however always come back to pound it like a pro.

So let’s explain this. Assetic, let’s take a look at the library and its basic usage (from the docu):

use Assetic\Asset\AssetCollection;
use Assetic\Asset\FileAsset;
use Assetic\Filter\Yui\JsCompressorFilter as YuiCompressorFilter;
 
$js = new AssetCollection(array(
    new FileAsset(__DIR__.'/jquery.js'),
    new FileAsset(__DIR__.'/application.js'),
), array(
    new YuiCompressorFilter('/path/to/yuicompressor.jar'),
));
 
header('Content-Type: application/js');
echo $js->dump();

And in the template:

<script src="/assets/javascripts.php"></script>

The concepts are very simple, you have a collection of assets which are dumpable resources, they take objectified resources and filters individually and collectively.

You dump it and voila!

Drupal situation however turns things a bit uglier, because Drupal does a lot more on top of just dumping some collections. Drupal handles the assets and assign them order, yeah it does matter in Drupal. As expected Drupal will try to reuse Assetic by wrapping the base classes and add what is lacking. Initially there were some attempts to hack into base classes by turning the properties protected instead of private, however the denial of this turned out to be better. Drupal now extends this class, well it is a long history and it is all recorder here in https://drupal.org/node/1762204.

I am not sure if the diff is based on patches or on sdboyer fork at this point but I created a diff on github here https://github.com/cordoval/drupal-1/pull/1/files. This is another problem for reviewing long work, they are including the composer.lock and what not which makes no sense. So I cleaned up and now the diff on github is more manageable and easier to review. I believe though the patch is the one that contains everything.

Long story short, I strongly believe in github flow because the latest diff is always the current diff and state of things. You add commits and that shows immediately and hides comments that were addressed already. You don’t have to review a 182 comment long page for just finding out what you want to work on. For such a work as this few people will actually collaborate and Github is way way better for this.

Anyway, going back to the topic, I am looking forward to Kriswallsmith’s talk on this topic on DrupalCon2014:

Screenshot 2014-05-26 02.14.51

I hope I don’t miss it and find out what is Assetic doing to plug best in Drupal.

The coming components to scrutinize are according to the image below on Symfony usage:

image

EventSubscriber and Routing!