Skip to content
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

[HttpKernel] Simplifying Bundle/Extension config definition #43701

Merged
merged 1 commit into from Mar 30, 2022

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Oct 25, 2021

Q A
Branch? 6.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #40259, #42647, #43080
License MIT
Doc PR -

This PR aims to simplify DI extension/configuration definitions at the Bundle level (based on @Nyholm #40259 (comment))

Currently, the services and configuration definitions have to deal with some conventions:

  • Create the DependencyInjection/ directory
  • Create the DependencyInjection/Configuration.php class to define the bundle config.
  • Create the DependencyInjection/FooExtension.php extension class and extend from Extension
  • In the ExtensionInterface::load() method to implement we have to:
    • Process the bundle configuration yourself Configuration, Processor, etc.
    • Create the specific *FileLoader & FileLocator instances to import services definition (have to deal with bundle path)
  • Prepend/append configs for other extensions requires implementing PrependExtensionInterface.
  • Redefine Bundle::$name to change the extension alias.

Although it may not be a big problem to follow all these conventions (indeed, we have been doing it for years) it's true that there are limitations and it requires extra work to achieve them.

Note: The following improvements don't pretend to deprecate the actual convention (at least so far) but simplify it with some benefits.


To start using the following improvements your bundle must extend from the new abstract class AbstractBundle to autoconfigure all hooks and make this possible inside a bundle class.

The first improvement offers the possibility to configure your bundle DI extension within the bundle class itself using loadExtension() method and the fluent ContainerConfigurator helper:

class FooBundle extends AbstractBundle
{
    public function loadExtension(array $config, ContainerConfigurator $container, ContainerBuilder $builder): void
    {
        $container->parameters()
            ->set('foo', $config['foo']);

        $container->import('../config/services.php');

        if ('bar' === $config['foo']) {
            $container->services()
                ->set(Parser::class);
        }
    }
}

This new method loadExtension() (a same goal that ExtensionInterface::load()) contains now all new benefits you currently love for service definition/import/etc. Keep in mind that this configurator still works with a temporal container, so you can't access any extension config at this point (as before). And, the $config argument is the bundle's Configuration that you usually process on ExtensionInterface::load() but here it's given to you already merged and processed (ready to use).


The next improvement comes when you want to prepend/append an extension config before all extensions are loaded & merged, then use the prependExtension() method:

class FooBundle extends AbstractBundle
{
    public function prependExtension(ContainerConfigurator $container, ContainerBuilder $builder): void
    {
        // prepend
        $builder->prependExtensionConfig('framework', [
            'cache' => ['prefix_seed' => 'foo/bar'],
        ]);

        // append
        $container->extension('framework', [
            'cache' => ['prefix_seed' => 'foo/bar'],
        ])

        // append from file
        $container->import('../config/packages/cache.php');
    }
}

This is the improved alternative to PrependExtensionInterface that you normally implement on extension classes. But using this method has bonus points, you can now use the ContainerConfigurator to append an extension config from an external file in any format (including the new PHP fluent-config feature).


Another improvement is about Configuration definition. Here you can manage it directly within the bundle class using the configuration() method with new possibilities:

class FooBundle extends AbstractBundle
{
    public function configure(DefinitionConfigurator $definition): void
    {
        // loads config definition from a file
        $definition->import('../config/definition.php');

        // loads config definition from multiple files (when it's too long you can split it)
        $definition->import('../config/definition/*.php');

        // defines config directly when it's short
        $definition->rootNode()
            ->children()
                ->scalarNode('foo')->defaultValue('bar')->end()
            ->end()
        ;
    }
}

You don't have to create the TreeBuilder instance yourself anymore and remember the proper extension alias. Instead, you will use a new DefinitionConfigurator with the possibility to import configuration definitions from an external PHP file, and this config file can now live outside the src/ directory of the bundle if desired:

// Acme/FooBundle/config/definition.php

use Symfony\Component\Config\Definition\Configurator\DefinitionConfigurator;

return static function (DefinitionConfigurator $definition) {
    $definition->rootNode()
        ->children()
            ->scalarNode('foo')->defaultValue('bar')->end()
        ->end()
    ;
};

And why not, you could also split your definition into several files if it's too long, or simply define the config directly in the method if it's short.


Last but not least you can change the extension alias by redefining a new property that now belongs to the MicroBundle class:

class AcmeFooBundle extends AbstractBundle
{
    protected string $extensionAlias = 'foo'; // alias used during the extension config loading

    // ...
}

The default alias will be determined from your bundle name (in this case acme_foo), so the new way allows you to change that alias without either touching your bundle name or overriding any method.


Note: The same feature has been implemented in a new AbstractExtension class for those applications applying the bundle-less approach and want to define configuration through an extension.

Combining all these benefits I believe we gain a more simplified bundle structure while decreasing the learning curve.

@carsonbot carsonbot added Status: Needs Review DependencyInjection Deprecation DX Feature HttpKernel labels Oct 25, 2021
@carsonbot carsonbot added this to the 5.4 milestone Oct 25, 2021
@carsonbot carsonbot changed the title [HttpKernel][DependencyInjection][DX] Simplifying DI extension/config definition [DependencyInjection][HttpKernel] Simplifying DI extension/config definition Oct 25, 2021
@ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Oct 25, 2021

I like this goal :) did you see #42754 also? I believe we can get rid of a lot of boilerplate generally, making everything first-class.

I tend to find the wiring as done in #43080 rather elegant, eg. no need for a new MicroBundleInterface IMHO.

@yceruto yceruto requested a review from nicolas-grekas Oct 25, 2021
@yceruto
Copy link
Member Author

@yceruto yceruto commented Oct 25, 2021

I like this goal :) did you see #42754 also? I believe we can get rid of a lot of boilerplate generally, making everything first-class.

Hi @ro0NL I did, at first glance it makes sense, let me think a bit more about it.

I tend to find the wiring as done in #43080 rather elegant, eg. no need for a new MicroBundleInterface IMHO.

Yep, I didn't want to add a new interface, but it should be simpler than working around the extension interface yourself. Also, because I needed a chance to instantiate the ContainerConfigurator. From the DX point of view, it's faster and intuitive.

An alternative I was thinking about is creating a new ExtensionBundle or MicroBundle subclass of Bundle, so we could get rid of this new interface and the required trait as well.

@wouterj
Copy link
Member

@wouterj wouterj commented Oct 25, 2021

Quick note (that doesn't invalidate any of this PR - I like the way this PR heads into): while these are based on "convention", they are all configured within the Bundle and XxxExtension classes themselves. I.e. you can customize both configuration and extension class name(space), see e.g. the SecurityBundle of Symfony (which uses MainConfiguration).

@ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Oct 25, 2021

From the DX point of view, it's faster and intuitive.

Perhaps just a MicroBundleTrait is sufficient ... to shortcut as done in #43080

But i'd consider this a final step :)

I mean, why aim for "bundle extends extension", rather than composition (using anon. classes)?

@yceruto
Copy link
Member Author

@yceruto yceruto commented Oct 25, 2021

I updated the implementation to remove the Interface dependency, so it's only about using the MicroBundleTrait. It feels better now :)

@yceruto yceruto force-pushed the feature/micro_bundle branch 10 times, most recently from 872e995 to aa65319 Compare Oct 26, 2021
@yceruto yceruto force-pushed the feature/micro_bundle branch 2 times, most recently from b892742 to 8f84f84 Compare Oct 26, 2021
@yceruto
Copy link
Member Author

@yceruto yceruto commented Oct 26, 2021

  • I updated the proposal, now it's specific to MicroBundleTrait only
  • Basic tests added

@yceruto
Copy link
Member Author

@yceruto yceruto commented Mar 2, 2022

FYI: This new feature is already available for Symfony 5.4 in this separate package https://github.com/yceruto/micro-symfony. Also, with MicroExtension class for bundle-less approach.

I will update this proposal to include MicroExtension too.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

LGTM! I just have some minor comments.

@yceruto
Copy link
Member Author

@yceruto yceruto commented Mar 4, 2022

Thanks @nicolas-grekas for your review.

I will update this proposal to include MicroExtension too.

I'm not completely sure about MicroExtension yet, so this is ready on my side.

Copy link
Member

@Nyholm Nyholm left a comment

I do like the fact that MicroBundle removes a lot of the boilerplate code.

Just a small question on a thing I haven’t seen before.

@yceruto yceruto changed the title [HttpKernel] Simplifying Bundle extension/config definition [HttpKernel] Simplifying Bundle/Extension config definition Mar 11, 2022
@yceruto yceruto force-pushed the feature/micro_bundle branch 2 times, most recently from 2c822d0 to 1c56490 Compare Mar 11, 2022
@yceruto
Copy link
Member Author

@yceruto yceruto commented Mar 11, 2022

I confirmed the usefulness of MicroExtension class with other developers, it's especially useful for those applications applying the bundle-less approach. They still need to define an extension to create the app configuration (probably more than one definition in big projects), so here we go with the same benefits as MicroBundle.

Update!

  • Added MicroExtension class
  • Deprecated ConfigurableExtension class in favor of MicroExtension
  • Refactoring solution to avoid code duplication

This is ready for review again.

UPGRADE-6.1.md Outdated Show resolved Hide resolved
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

One more round of review :)

@yceruto yceruto force-pushed the feature/micro_bundle branch 3 times, most recently from 914defd to 4118c39 Compare Mar 27, 2022
fabpot
fabpot approved these changes Mar 30, 2022
interface ConfigurableExtensionInterface extends ConfigurableInterface
{
/**
* Allow an extension to prepend the extension configurations.
Copy link
Member

@fabpot fabpot Mar 30, 2022

Choose a reason for hiding this comment

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

Suggested change
* Allow an extension to prepend the extension configurations.
* Allows an extension to prepend the extension configurations.

@fabpot
Copy link
Member

@fabpot fabpot commented Mar 30, 2022

Thank you @yceruto.

@fabpot fabpot merged commit 4e6b803 into symfony:6.1 Mar 30, 2022
6 of 9 checks passed
@yceruto yceruto deleted the feature/micro_bundle branch Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX Feature HttpKernel Status: Reviewed
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

9 participants