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.

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');
        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());
        $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());
        $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->assertEquals('foo', (string) $aliases['another_alias_for_foo']);
    public function testNonArrayTagThrowsException()
        $loader = new YamlFileLoader($container = new ContainerBuilder());
        try {
            $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 {
            $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 {
            $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


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:

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

    abstract: true
      - [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.

    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:


EventSubscriber and Routing!

Drupal8.x and Symfony Developing Minding BC on Upgrades – Ajax Core Component: Part III Series

Screenshot 2014-05-24 05.02.33

So today I grabbed the Ajax core component. At first it seemed daunting because I did not know what I was going to find in so many flat directory structure. But after a bit of analysis I discovered that the many XCommand classes were really very simple implementations of a commond XCommandInterface. So the first thing I did was to move all those classes into a Command directory. These commands have nothing to do with the Command classes as we know them from Symfony.

Let me explain. It seems like in Drupal the requirement to communicate via JSON responses and in particular Ajax responses needed to be standardized. Drupal needs this because on this bus for ajax communications all other moving parts that are ajax related on the front end will revolve their actions. These actions are called commands, and it is pretty much information useful on the front side, possibly used by the javascript that runs on the front end.

So the ajax system, as we call it, comprises of many JavaScript libraries that are served and then the hub or bus for communications that is server side and therefore have to have a type of container or standard place where to put information and where to read information when doing JSON communications, that is request and responses that pertain to dynamic calls from the client.

In this ajax system, the part in PHP, server side, and that pertain to the Ajax core component, there are only 4 moving pieces namely: the AjaxResponse, the AjaxResponseRenderer, an AjaxSubscriber, and the Commands.

AjaxResponse is an extension of the Symfony’s JsonResponse where the commands are computed, rendered with the Drupal inner renderer functions that deal with arrays and the part that is still not OO, but that is wrapped, so that we can end up with a JsonResponse object that has a drupalized-array-of-commands payload on the JsonResponse.

The extension of the JsonResponse breaks the BC promise from Symfony in that it overrides a public method on a non api class. I did the PR to drupal here https://drupal.org/node/2273717 and filed an issue in Symfony asking for workarounds here https://github.com/symfony/symfony/issues/10985.

// AjaxResponse extends JsonResponse
   * {@inheritdoc}
   * Sets the response's data to be the array of AJAX commands.
  public function prepare(Request $request) {
    return $this;
   * Sets the rendered AJAX right before the response is prepared.
   * @param \Symfony\Component\HttpFoundation\Request $request
   *   The request object.
  public function prepareResponse(Request $request) {
    if ($this->data == '{}') {

We will see what happens. For certain at least there may have been a need to replug that call to the parent::prepare($request) that was missing. If not well the refactor of the tests seemed to me reasonable and also the folder organization. Drupal still have some ugly organization, think of flat folder structure and you get a pretty good picture. Another nuance regarding this is that digging inside let’s you see that there are 2 layers, one where it is all OO and the other where the drupal_x calls are still creeping in.

I have found some tickets that were opened and discussed but then closed and nothing was done. There were good chances to refactor and remove those calls but there seems to be a lack of interest in getting rid completely of these hooks and array manipulation. The array manipulations obscure the parts of the class where they are cornered. Even in the tests, you have to see these &getCommand ampersands just to, and I don’t know the real reason, to have access to an internal array inside the class. This is poor OO and it is a crutch on the side of the Drupal developers that they probably need to deal with. This causes impacts on every side though, tests that are not optimal, thereby violating the principles of closed classes or unit testing.

The next element is the AjaxResponseRenderer, it basically builds an AjaxResponse by calling conditionally and adding some specific commands, and then calling the monster of drupal_render to convert these commands further into the payload to send inside the AjaxResponse. Nothing to see here, just the render command and well what I have explained above.

The AjaxSubscriber is a bit interesting, it lacks a test, so I don’t know what is its use case, however it seems to set the format of an incoming Request. This is weird as it sets drupal fields that relate to application/vnd.drupal-x, maybe to ensure by forcing these into every incoming request for a specific call pattern. Smells like a workaround and not a robust solution.

Then comes the Commands, there is a diverse selection of commands and use cases. From adding some css into the page, adding more markup, setting dialogs, modals, invoking some js, etc. There are commands for that, you name it!

So that is it for this component. I hope you have had fun reading this, I stayed a bit late just to get this out.

Please think about supporting me. Thank you and mind I am doing this all with a lot of excitement and passion for developing.

undeservedly doing this with joy, your friend @cordoval