Symfony2: Some things I don’t like about Bundles

This article could have been titled “Ten things I hate about bundles”, but that would be too harsh. The things I am going to describe, are not things I hate, but things I don’t like. Besides, when I count them, I don’t come to ten things…

Bundle extension discovery

A Symfony2 bundle can have an Extension class, which allows you to load service definitions from configuration files or define services for the application in a more dynamic way. From the Symfony documentation:

[The Extension] class should live in the DependencyInjection directory of your bundle and its name should be constructed by replacing the Bundle suffix of the Bundle class name with Extension. For example, the Extension class of AcmeHelloBundle would be called AcmeHelloExtension [...]

When you conform to the naming convention, your bundle’s Extension class will automatically be recognized. But how? A quick look in the Kernel class reveals to us that it calls the method getContainerExtension() on each registered bundle (which is an object extending implementing BundleInterface):

namespace Symfony\Component\HttpKernel;
...
abstract class Kernel implements KernelInterface, TerminableInterface
{
    protected function prepareContainer(ContainerBuilder $container)
    {
        ...
        foreach ($this->bundles as $bundle) {
            if ($extension = $bundle->getContainerExtension()) {
                $container->registerExtension($extension);
                ...
            }
            ...
        }
        ...
    }
}

If the getContainerExtension() method of a bundle returns anything, it is assumed to be an instance of ExtensionInterface, in other words a service container extension.

My bundles almost always look like this:

namespace Matthias\Bundle\DemoBundle;

use Symfony\Component\HttpKernel\Bundle\Bundle;

class MatthiasDemoBundle extends Bundle
{
}

There is no implementation for getContainerExtension() there, so it must be in the parent class. And it is:

namespace Symfony\Component\HttpKernel\Bundle;
...
abstract class Bundle extends ContainerAware implements BundleInterface
{
    ...
    public function getContainerExtension()
    {
        if (null === $this->extension) {
            $basename = preg_replace('/Bundle$/', '', $this->getName());

            $class = $this->getNamespace().'\\DependencyInjection\\'.$basename.'Extension';
            if (class_exists($class)) {
                $extension = new $class();

                // check naming convention
                $expectedAlias = Container::underscore($basename);
                if ($expectedAlias != $extension->getAlias()) {
                    throw new \LogicException(sprintf(
                        'The extension alias for the default extension of a '.
                        'bundle must be the underscored version of the '.
                        'bundle name ("%s" instead of "%s")',
                        $expectedAlias, $extension->getAlias()
                    ));
                }

                $this->extension = $extension;
            } else {
                $this->extension = false;
            }
        }

        if ($this->extension) {
            return $this->extension;
        }
    }
}

My goodness, this is some ugly code, but more imporantly: this is magic! My extension class is nowhere explicitly used, nor registered as the one and only service container extension for my bundle. It is simply infered from the existence of a file with the expected name, containing the expected class (there is not even a check for the right interface), that I have a service container extension for my bundle! I don’t like that at all.

Naming conventions

Even worse: the naming conventions prevent you from easily moving your Bundle and Extension class to another namespace (like you probably have noticed yourself).

I can move Matthias\Bundle\DemoBundle\MatthiasDemoBundle to Matthias\Bundle\TestBundle\MatthiasTestBundle with a simple search-and-replace, but I can not just move its DependencyInjection\MatthiasDemoExtension class to Matthias\Bundle\TestBundle\DependencyInjection, The class itself has to be renamed to MatthiasTestExtension, or it won’t be recognized anymore.

Also somewhat annoying: the Bundle::getContainerExtension() puts a constraint on the alias of the service container extension: when my bundle is called MatthiasTestBundle, its alias should be matthias_test. But there is no real need for this, it is just a policy to prevent developers from overriding each other’s (or worse: the framework’s) bundle configuration.

This last rule is enforced in quite a strange way. Remember where the exception is being thrown? Yes, inside my own bundle class! I can easily override getContainerExtension() and skip the validation of the alias of my service container extension…

Registering service container extensions yourself

Because of the bundle magic described above, I like to implement the getContainerExtension() method myself and return an instance of the extension. The name of this class can be anything I like.

namespace Matthias\Bundle\TestBundle;

use Matthias\Bundle\TestBundle\DependencyInjection\CanBeAnything;

class MatthiasTestBundle extends Bundle
{
    public function getContainerExtension()
    {
        return new CanBeAnything();
    }
}

Now creation logic is also entirely on my side, where I like it to be.

Naming conventions, part two

As mentioned above, an extension has an alias, which you can retrieve by calling its method getAlias(). The standard implementation of this method is:

namespace Symfony\Component\DependencyInjection\Extension;
...
abstract class Extension implements ExtensionInterface, ConfigurationExtensionInterface
{
    ...
    public function getAlias()
    {
        $className = get_class($this);
        if (substr($className, -9) != 'Extension') {
            throw new BadMethodCallException('This extension does not follow the naming convention; you must overwrite the getAlias() method.');
        }
        $classBaseName = substr(strrchr($className, '\\'), 1, -9);

        return Container::underscore($classBaseName);
    }
    ...
}

This function also checks if we have followed the naming convention for extension classes. Since we have chosen to skip the naming convention check in the bundle class, we might just as well skip the check in the extension class too, and just implement the getAlias() method ourselves:

namespace Matthias\Bundle\TestBundle\DependencyInjection;

use Symfony\Component\HttpKernel\DependencyInjection\Extension;

class CanBeAnything extends Extension
{
    public function getAlias()
    {
        return 'matthias_test';
    }
}

Perfectly fine! Now moving a Bundle or Extension class won’t break any existing configuration under the key matthias_test in config.yml and the likes.

Duplicate knowledge: the extension alias

As you may know, when you create a Configuration class for your bundle, you have to provide the configuration key, also known as the service container alias, as the name of the root node of the TreeBuilder instance:

namespace Matthias\Bundle\TestBundle\DependencyInjection;

use Symfony\Component\Config\Definition\Builder\TreeBuilder;
use Symfony\Component\Config\Definition\ConfigurationInterface;

class Configuration implements ConfigurationInterface
{
    public function getConfigTreeBuilder()
    {
        $treeBuilder = new TreeBuilder();

        // there it is: the service container alias!
        $rootNode = $treeBuilder->root('matthias_test');

        ...

        return $treeBuilder;
    }
}

It has always bothered me that this is somehow duplicate knowledge: it should not be necessary to use the exact same string here, which can also be retrieved by calling getAlias() on the Extension class. But the Configuration class can not do this: it has no access to the service container extension. Instead, it has to be the other way around: the Extension needs to provide its alias to the Configuration:

namespace Matthias\Bundle\TestBundle\DependencyInjection;

use Symfony\Component\DependencyInjection\ContainerBuilder;

class CanBeAnything extends Extension
{
    public function load(array $config, ContainerBuilder $container)
    {
        $configuration = new Configuration($this->getAlias());

        $processedConfig = $this
            ->processConfiguration($configuration, $config);
    }

    public function getAlias()
    {
        return 'matthias_test';
    }
}

Then the Configuration class needs to look something like this:

namespace Matthias\Bundle\TestBundle\DependencyInjection;

class Configuration implements ConfigurationInterface
{
    private $alias;

    public function __construct($alias)
    {
        $this->alias = $alias;
    }

    public function getConfigTreeBuilder()
    {
        $treeBuilder = new TreeBuilder();

        // no more duplication!
        $rootNode = $treeBuilder->root($this->alias);

        ...

        return $treeBuilder;
    }
}

In conclusion

Well, it takes some extra steps, but only a few, and very easy ones. They will make it much easier to move bundles to another namespace without many other things breaking. They will also free you from naming conventions that would have otherwise been enforced.

Maybe most importantly: they finally remove some magic from bundles, that has been there as some kind of a reminiscence of the old days of symfony 1. It is not very modern to automatically load a file that is located in a certain directory and has a certain name.

(The same goes for console commands, for also registered automagically. See one of my previous post for more about this.)

One final remark: I still recommend you to conform to these conventions:

- Your service container alias/configuration root should correspond to the name of your bundle, to prevent naming collissions.
- Your Extension and Configuration classes should still be in the DependencyInjection directory/namespace.

These are good conventions, and they make it easy for any developer to understand and work with your bundle.

Posted in Configuration, Dependency injection container, Symfony2 | 20 Comments

20 Responses to Symfony2: Some things I don’t like about Bundles

  • # October 29, 2013 at 22:47

    a rapid count on headings reach to 5 :) but i am reading more slowly this time

    cordoval

    Reply

  • # October 30, 2013 at 07:48

    I think the most problematic point is that the HttpKernel instance is too much coupled to Bundles. For me it would make sense when the Kernel itself (not the HttpKernel) and the Bundle workflow would be extracted to one seperate component or to the FrameworkBundle itself which needs those conventions made in your mentioned base classes. Those conventions are not needed outside of an Symfony application and so should not be included in the component.
    Some days ago I had to introduce the HttpKernel and the DIC in some plain PHP project and this was a bit painful. I had to introduce Bundles too that but they were not needed in this project.
    The HttpKernel should not rely on any Bundle workflow. It should receive a request and return it into a response. The (application) Kernel should introduce Bundles and extend this workflow.

    Frank

    Reply

    • # October 30, 2013 at 08:40

      The HttpKernel has no knowledge of the bundles whatsoever, so you are probably confusing something here. The best proof is that it is used by Silex and Drupal, where there is no notion of bundles.

      Fabien

      Reply

      • # October 30, 2013 at 09:55

        Thanks for clarifying that point, Fabien. The Kernel class implements the HttpKernelInterface, but it lets HTTP requests be handled by another instance of HttpKernelInterface:

        namespace Symfony\Component\HttpKernel;
        ...
        class Kernel
        {
            ...
            public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQUEST, $catch = true)
            {
                ...
                return $this->getHttpKernel()->handle($request, $type, $catch);
            }
        }
        

        Frank, you are right about the component having two major use-cases: an application kernel with bundles and an HTTP kernel. These could have been separated, following the Common Reuse Principle. See also http://butunclebob.com/ArticleS.UncleBob.PrinciplesOfOod and my upcoming book Principles of PHP Package Design.

        Matthias Noback

        Reply

      • # October 30, 2013 at 10:35

        Yes, sorry about this confusion. I had a typo in my comment and meant the HttpKernel component in common is too coupled with Bundles. The HttpKernel class itself is of course completely fine and has no needless dependencies! The Kernel class however introduces Bundles as they are needed e.g. for building up the complete container. This logic I would not assume in the HttpKernel component but instead in another additional “Kernel” component which relies on HttpKernel. In this way the HttpKernel would be totally free from the Bundle dependency.

        Frank

        Reply

    • # February 25, 2014 at 15:08

      +1

      Spomky

      Reply

  • # October 30, 2013 at 07:50

    Sorry i meant of course “that the HttpKernel component is too much coupled to Bundles.”

    Frank

    Reply

  • # October 30, 2013 at 09:02

    This is not the first time someone writes about this topic.

    You must know that this “magic” was introduced late in the Symfony2 dev process (in 14aa95ba218c9fd6d02e770e279ed854314fea75 to be precise — 02/15/2011), so it’s not something that is reminiscent from symfony1.

    Also, it was introduced after a lot of debates and for very good reasons, some of them are mentioned in this post. IIRC, better error reporting (when someone makes a typo in the extension name in a configuration file), ease of documentation and the possibility to automate things (the configuration entry is auto-discoverable), and ease of bundle creation were the main reasons to introduce this concept. And as you mentioned yourself, changing/overriding/getting rid of this magic is as easy as it can get.

    Unfortunately, I’m not able to find the discussion about this topic right now (was it done during an IRC meeting, on the mailing-list or on a PR?), but it took a lot of time to come that that conclusion, which was the best possible compromise.

    That said, if someone come up with a better way, I’m open to any suggestions.

    Fabien

    Reply

    • # October 30, 2013 at 10:05

      Thanks for your elaborate response, Fabien. I think the conventions are great (as I’ve mentioned at the end of the article) – it would become a mess otherwise. They are probably the best way to keep sanity – I remember these conventions helped me when I started working with Bundles two years ago. It flattens the learning curve quite a lot, since you don’t have to worry about extension and configuration classes and how to register them, but you can just start adding service definitions to the service container.
      I was surprised to see how easy it was to override the logic from the base classes with something that is also correct, but more explicit and less automatic. This is what I like about Symfony2: that not too many things happen automatically, you need to be explicit about almost everything.
      If you happen to find the place and time of the discussion about this subject, I’d be happy to receive it.

      Matthias Noback

      Reply

  • # October 30, 2013 at 17:19

    Exactly what you describe always makes renaming bundles a messy business

    I’d +1 on a PR in 3.0 that would make the Bundle::getContainerExtension and Extension::getAlias methods abstract, and a PR to update the bundle generator command to fill in the blanks with sensible defaults

    Bart van den Burg

    Reply

    • # October 31, 2013 at 10:09

      That would be nice indeed!

      Matthias Noback

      Reply

  • # November 1, 2013 at 07:13

    I think this post could also be titled, Become or Make your Symfony walk the 2 miles or work for you, or become more flexible. I think your 3 or 4 advices can be implemented in every company that is now more mature in Symfony and needs to have deeper understanding and flexibility. To me the commands thing looks as critical. Let’s see where this ends in github ticket. I wonder if these tips don’t conflict with anything compilerpasses related or other.

    Thanks!

    cordova

    Reply

    • # November 1, 2013 at 08:28

      Nice :) I think these last couple of posts are indeed the way to go for more advanced users, with long-lasting projects where code has to move sometimes, to reflect changes in the organization. This article is also meant to explain some of the inner workings that developers should know about, instead of just relying on.

      Matthias Noback

      Reply

  • # December 12, 2013 at 20:19

    Nice post Mattias, reducing the duplicate know of the bundle/extension alias seems like a good idea but I found an issue with the implementation shown in the post which is that it breaks the config:dump-reference command for your bundle unless you implement getConfiguration in your extension. This is because the base Extension->getConfiguration method will fail to return an instance of the Configuration class if it has a constructor (since it has no way of knowing what arguments it might expect).

    https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Extension/Extension.php#L93

    This is a problem that is easy to miss if you start using the method advocated here since you won’t come across it until you try to call the config:dump-reference command on one of your own bundles. You may want to update the extension code to include an implementation of getConfiguration in the extension which returns a new Configuration instance.

    Ben Glassman

    Reply

    • # December 15, 2013 at 13:40

      Hi Ben,
      Thanks, that’s an excellent suggestion (also something I didn’t know about!).

      Matthias Noback

      Reply

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>