8000 [RFC][DX] Add possibility to provide bundle configuration (definition) inside the bundle class · Issue #40259 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[RFC][DX] Add possibility to provide bundle configuration (definition) inside the bundle class #40259

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

Closed
mahono opened this issue Feb 20, 2021 · 14 comments · Fixed by #43701
Closed
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) RFC RFC = Request For Comments (proposals about features that you want to be discussed) Status: Waiting feedback

Comments

@mahono
Copy link
mahono commented Feb 20, 2021

Description
With all the recent changes towrds configuring things using PHP I wonder why not providing the possibility to provide bundle configuration directly inside the bundle class. Currently developers need to create an extension class and then a Resources/services.php file to return the configuration definition.

I think it could be a great improvement to provide an empty method in Symfony\Component\HttpKernel\Bundle\Bundle that can be overridden to just return that same configuration you would return in the Resources/services.php file.

Example
From developer perspective it could look like this in a bundle class:

class ByAwesomeBundle extends Bundle
{
    protected function configureContainer(ContainerConfigurator $container)
    {
        // Do whatever you like to configure the container for the bundle.
    }
}

Or I'm not 100% sure how the system works maybe the method needs to return Closure to work with the container building system.

I just wanted to report this idea. :o)

@carsonbot carsonbot added DX DX = Developer eXperience (anything that improves the experience of using Symfony) RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Feb 20, 2021
@yceruto
Copy link
Member
yceruto commented Feb 20, 2021

What about Bundle::build(ContainerBuilder $container) method? Although it's not the fancy ContainerConfigurator you can do whatever you want concerning DI directly.

8000

@gggeek
Copy link
gggeek commented Feb 21, 2021

Sorry, but this could easily lead to configuration becoming more magic and less understandable, as using more php instead of declarative languages (yaml, xml) allows developers to embed more logic in the "creation of the final configuration values".
This in turns makes it hard to the developers who have to use/modify/extend those bundles to both understand what is going on and tweak it.
While it could be a nice option for 'project code', I am not sure it is a good idea for 'library code'...

@ahundiak
Copy link
Contributor
ahundiak commented Feb 22, 2021

Conceptually, I have always felt that AppBundle had basically been replaced with src/Kernel.php. So besides a configureContainer method, it might be interesting to add a configureRoutes(RoutingConfigurator $routes) method as well. You could even go crazy and allow implementing things like the CompilerPassInterface.

@Nyholm
Copy link
Member
Nyholm commented Feb 22, 2021

I might be slightly off topic, but here is an idea:

Currently, to create a bundle, you need a AcmeHelloBundle class, an Extension and a Configuration. You can define your services in the Extension::load() or with some addition php/yaml/xml resources.

You could however make this process a bit simpler by adding everything in the AcmeHelloBundle class. Working example:

namespace Acme;

use Symfony\Component\Config\Definition\Builder\TreeBuilder;
use Symfony\Component\Config\Definition\ConfigurationInterface;
use Symfony\Component\Config\Definition\Processor;
use Symfony\Component\Config\FileLocator;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Extension\ConfigurationExtensionInterface;
use Symfony\Component\DependencyInjection\Extension\ExtensionInterface;
use Symfony\Component\HttpKernel\Bundle\Bundle;
use Symfony\Component\DependencyInjection\Loader;

class AcmeHelloBundle extends Bundle implements ExtensionInterface, ConfigurationExtensionInterface, ConfigurationInterface
{
    public function getConfigTreeBuilder()
    {
        $treeBuilder = new TreeBuilder($this->getAlias());
        $root = $treeBuilder->getRootNode();

        $root
            ->ch
8000
ildren()
                ->scalarNode('some_option')->isRequired()->cannotBeEmpty()->end()
            ->end();

        return $treeBuilder;
    }

    public function load(array $rawConfigs, ContainerBuilder $container)
    {
        $processor = new Processor();
        $config = $processor->processConfiguration($this, $rawConfigs);
        $loader = new Loader\YamlFileLoader($container, new FileLocator(__DIR__.'/config'));
        $loader->load('services.yaml');

        // Do something with $config['some_option']
        // Add service definitions
    }

    public function getConfiguration(array $config, ContainerBuilder $container)
    {
        return $this;
    }

    public function getContainerExtension()
    {
        return $this;
    }

    public function getXsdValidationBasePath()
    {
        return false;
    }

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

This is okey "light" bundles. For most bundles they can be just simple libraries + recipes and for "heavy" bundles, you probably want the Extension and Configuration in separate classes.

@jvasseur
Copy link
Contributor
jvasseur commented Feb 22, 2021

If I recall correctly, loading code even if not executed adds an small overhead on each requests (it might have changed with preloading) and since the bundles classes are loaded on each requests. This means that if we go this route, the configuration and building code of bundles will be loaded on every requests, even when it's not needed because the container is already built.

@Nyholm
Copy link
Member
Nyholm commented Feb 22, 2021

Excellent observation. You are correct. The Bundle class is loaded into memory at each request, but Configuration and Extension is only loaded when building the container.

I have not measured the impact, but Im sure it is really neglectable. And as you mention, the time penalty will go away with with preloading.

Also note (in case it was not obvious), the code I posted is already working with sf 5.2 and most likely with sf 4.4.

@jonny827
Copy link
jonny827 commented Feb 23, 2021

@gggeek Magic is all perspective.

The way Symfony does configuration is problematic.

Unless I am missing something, In the bundle, I can easily configure services/parameters but not routes or configuration.

There are several use cases that I have where it would be much better to import routes from bundles so that I can manage the route configuration at the bundle level. This is especially helpful when using locale based route configurations like

path:
        en: /about-us
        nl: /over-ons

Routes in bundles are also super helpful for managing host for subdomain usage for multi-tenant, api, dev, mobile version, etc.

For config, I would like to configure Twig and Doctrine, from within bundles as well. /bundle//Resources/config/packages/Twig.yml would be appended to config/packages/Twig.yml. This is similar to how templates are overwritten but in this case it is an append.

It is Symfony, I think configuration options are great including php, yaml, xml! I would like to add a configuration provider so we could store configuration in the database and I could alter configurations using SQL.

If I happen to be missing either of these options (config and routes in bundles) and (configurations stores in DB similar to a user provider) then please. let me know!

Until then, I agree with @mahono that we need to move towards best practices, because what we have right now is not it in my opinion.

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Feb 23, 2021

I can easily configure services/parameters but not routes or configuration.

This is by design: routes belong to the "public API" of web apps. Not to bundles. Bundles should not create routes on their own - the dev should wire the routes for a bundles. Recipes provides working routes out-of-the-box, which is the best DX to me.

I would like to add a configuration provider so we could store configuration in the database and I could alter configurations using SQL.

Since Symfony's container is compiled, it'd mean finding a way to connect to the DB before compiling the container. Since the DB connection is defined as a service, there is a chicken-n-egg situation.
This is still possible, see Drupal. Of course, this comes with its set of compromises, which we won't do in Symfony (use Drupal if you need this).

But this is going a bit off topic :)

@gggeek
Copy link
gggeek commented Feb 23, 2021

@nicolas-grekas sorry for introducing the off-topic topic ;-) I have a tendency to do that in comments to PRs as those provide an extremely convenient, universally available way to give feedback (thanks github for that btw!).
I'd love it if there was an equivalently easy way to discuss about more high-level design decisions, such as what @jonnny827 is doing.
Alas, many projects provide little or no venue for feedback on design, and only allow it on working code / prototypes - and at that moment it feels bad to undo what has already been done :-( Heck, some developers even have a dislike of discussing stuff in abstract...

@wouterj
Copy link
Member
wouterj commented Feb 23, 2021

@gggeek you can start new discussions (concrete & abstract) by creating new issues (https://github.com/symfony/symfony/issues/new/choose) or commenting on issues that are about the topic already. Or join the Symfony slack if you want to have a more conversation-type discussion within the community.

Anyway, back to the topic here.


I think @Nyholm already showed that bundle classes are already able to provide DI configuration. Is there anything to do here?

@jonny827

This comment has been minimized.

@xabbuh
Copy link
Member
xabbuh commented Mar 8, 2021

I agree with @wouterj and @Nyholm. Let's close here.

@yceruto
Copy link
Member
yceruto commented Oct 27, 2021

FYI with #43701 you can manage the DI extension of the bundle within the bundle class itself with all the new stuff you currently like and more:

class AcmeFooBundle extends MicroBundle
{
    public function configuration(DefinitionConfigurator $definition): void
    {
        $definition->import('../config/definition.php');
    }

    public function prependExtension(ContainerConfigurator $container, ContainerBuilder $builder): void
    {
        $container->import('../config/packages/cache.php');
    }

    public function loadExtension(array $config, ContainerConfigurator $container, ContainerBuilder $builder): void
    {
        $container->import('../config/services.php');
    }
}

Further, you can load Config definitions and extension configs from an external file, or if desired, everything can be defined in place.

fabpot added a commit that referenced this issue Mar 30, 2022
…ition (yceruto)

This PR was merged into the 6.1 branch.

Discussion
----------

[HttpKernel] Simplifying Bundle/Extension config definition

| 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:
```php
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:
```php
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:
```php
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:
```php
// 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:
```php
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.

Commits
-------

7e8cf5d Simplifying bundle extension/config definition
symfony-splitter pushed a commit to symfony/dependency-injection that referenced this issue Mar 30, 2022
…ition (yceruto)

This PR was merged into the 6.1 branch.

Discussion
----------

[HttpKernel] Simplifying Bundle/Extension config definition

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

This PR aims to simplify DI extension/configuration definitions at the Bundle level (based on @Nyholm symfony/symfony#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:
```php
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:
```php
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:
```php
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:
```php
// 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:
```php
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.

Commits
-------

7e8cf5df10 Simplifying bundle extension/config definition
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) RFC RFC = Request For Comments (proposals about features that you want to be discussed) Status: Waiting feedback
Projects
None yet
Development

Successfully merging a pull request may close this issue.

0