8000 [HttpKernel] Simplifying Bundle/Extension config definition by yceruto · Pull Request #43701 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpKernel] Simplifying Bundle/Extension config definition #43701

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 30, 2022
Merged

[HttpKernel] Simplifying Bundle/Extension config definition #43701

merged 1 commit into from
Mar 30, 2022

Conversation

yceruto
Copy link
Member
@yceruto yceruto commented Oct 25, 2021 •
edited
Loading
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 configure() 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 be placed 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 AbstractBundle 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 DX = Developer eXperience (anything that improves the experience of using Symfony) 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
@yceruto yceruto force-pushed the feature/micro_bundle branch from 571cb70 to 302b564 Compare October 25, 2021 17:07
@ro0NL
Copy link
Contributor
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
Copy link
Member Author
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 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 commented Oct 25, 2021 •
edited
Loading

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 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 October 26, 2021 03:39
@yceruto yceruto force-pushed the feature/micro_bundle branch 2 times, most recently from b892742 to 8f84f84 Compare October 26, 2021 04:39
@yceruto
Copy link
Member Author
yceruto commented Oct 26, 2021
  • I updated the proposal, now it's specific to MicroBundleTrait only
  • Basic tests added

ro0NL
ro0NL reviewed Oct 26, 2021
View reviewed changes
@yceruto yceruto force-pushed the feature/micro_bundle branch from 45878bd to 7d717b9 Compare March 11, 2022 18:23
@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 March 11, 2022 23:48
@yceruto
Copy link
Member Author
yceruto commented Mar 11, 2022 •
edited
Loading

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.

@yceruto yceruto force-pushed the feature/micro_bundle branch from 1c56490 to cd98d85 Compare March 12, 2022 06:47
GromNaN
GromNaN reviewed Mar 12, 2022
View reviewed changes
@yceruto yceruto force-pushed the feature/micro_bundle branch from a0e462f to 81d58ab Compare March 21, 2022 16:22
nicolas-grekas
nicolas-grekas reviewed Mar 24, 2022
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

One more round of review :)

@yceruto yceruto force-pushed the feature/micro_bundle branch 3 times, most recently from 914defd to 4118c39 Compare March 27, 2022 10:49
@yceruto yceruto force-pushed the feature/micro_bundle branch from 4118c39 to 7e8cf5d Compare March 30, 2022 09:36
@fabpot
Copy link
Member
fabpot commented Mar 30, 2022

Thank you @yceruto.

@fabpot fabpot mentioned this pull request Mar 30, 2022
Closed
@fabpot fabpot merged commit 4e6b803 into symfony:6.1 Mar 30, 2022
@yceruto yceruto deleted the feature/micro_bundle branch March 30, 2022 22:46
@fabpot fabpot mentioned this pull request Apr 15, 2022
Merged
@yceruto yceruto mentioned this pull request May 12, 2022
Merged
javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull request May 27, 2022
@javiereguiluz
minor symfony#16801 [DI] Documenting Abstract Bundle and Extension (y…
6be632e 
…ceruto, wouterj)

This PR was merged into the 6.1 branch.

Discussion
----------

[DI] Documenting Abstract Bundle and Extension

Fixes symfony#16669
PR symfony/symfony#43701

ping `@Nyholm` `@wouterj` as you were interested in this feature/doc. Any suggestion to improve the wording is more than welcome!

---

I also updated other places to be aligned with the best practices doc.
I didn't remove the current convention to create bundles/extensions/configurations because they are still valid until 6.4 becomes the last LTS available. However, it's ok for apps using AbstractExtension.

Commits
-------

7a24f08 typo
c426dbb Fix extension/config paths
c85de08 Revert attribute route
558b02e Rewrite the bundle docs a bit more
7cf9bf4 Add warning
d0cba72 Remove note after update the feature
c2c47a2 Update bundle.rst
c93db0b Jules' review
426e289 Documenting Abstract Bundle and Extension
This was referenced May 28, 2022
Closed
Closed
@dkarlovi dkarlovi mentioned this pull request Jan 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GromNaN GromNaN GromNaN left review comments

@ro0NL ro0NL ro0NL left review comments

@Nyholm Nyholm Nyholm left review comments

@ogizanagi ogizanagi ogizanagi left review comments

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature HttpKernel Status: Reviewed
Projects
None yet
Milestone
6.1
Development

Successfully merging this pull request may close these issues.

[RFC][DX] Add possibility to provide bundle configuration (definition) inside the bundle class
9 participants
@yceruto @ro0NL @wouterj @fabpot @nicolas-grekas @GromNaN @Nyholm @ogizanagi @carsonbot
0