Skip to content

[DependencyInjection] Prepend extension config with ContainerConfigurator #52636

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

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Nov 17, 2023

Q A
Branch? 7.1
Bug fix? no
New feature? yes
Deprecations? no
Issues -
License MIT

I found this by fixing an issue in a bundle that was trying to prepend extension configs using $container->extension('namespace', [...]) in AbstractBundle::prependExtension(), which indeed was appending that config instead of prepending it. Most importantly, the append strategy requires the extension namespace to be loaded beforehand, which is not required when prepend is used.

This title DX improvement helps to avoid the confusion between ContainerConfigurator $container and ContainerBuilder $builder to prepend extension config by allowing ContainerConfigurator to do the same now.

Example:

class AcmeFooBundle extends AbstractBundle
{
    public function prependExtension(ContainerConfigurator $container, ContainerBuilder $builder): void
    {
        // Before (only way)
        $builder->prependExtensionConfig('namespace', ['foo' => 'bar']);

        // After (also this way) passing "true" as the 3rd parameter
        $container->extension('namespace', ['foo' => 'bar'], true);
    }
}

Instead of adding a new $prepend argument to the existing method, I could create a new method, e.g., ContainerConfigurator::prependExtension(). What do you prefer?

This also helps when you want to prepend several or large configs in your bundle or extension class. Actually, using just $container->import('...') doesn't work because internally it will always append the configs, unless you do the following hidden trick below.

// acme-bundle/config/packages/configs.php
use Symfony\Component\DependencyInjection\ContainerBuilder;

return static function (ContainerBuilder $container) {
    $container->prependExtensionConfig('namespace', ['large' => 'config', ...]);
};

If you type ContainerBuilder instead of ContainerConfigurator in the external PHP config file, the builder instance will be passed instead, allowing you to use the prependExtensionConfig() method.

But with this proposal, it's simpler as you can keep using ContainerConfigurator to prepend extension configs without doing any tactic.

@yceruto yceruto added this to the 7.1 milestone Nov 17, 2023
@carsonbot carsonbot added Status: Needs Review DependencyInjection DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature labels Nov 17, 2023
@carsonbot carsonbot changed the title [DI][DX] add argument to prepend extension config [DependencyInjection] add argument to prepend extension config Nov 17, 2023
@yceruto yceruto changed the title [DependencyInjection] add argument to prepend extension config [DependencyInjection] Prepend extension config with ContainerConfigurator Nov 17, 2023
@yceruto
Copy link
Member Author

yceruto commented Nov 17, 2023

Ideally, I would like to import configs from the AbstractBundle::prependExtension() method this way:

$container->import('../config/packages/framework.yaml', prepend: true); // or using a new method

But the impact is currently too high, as this prepend value should pass across multiple interfaces and processes until reaching the final ContainerBuilder::loadFromExtension() invocation.

@yceruto
Copy link
Member Author

yceruto commented Nov 17, 2023

I was also thinking about a new marker in the config file, similar to when@env, but using it for the importing config strategy, e.g.:

import@prepend:
    framework:
        messenger: ...

And then prepend that config instead of appending. Even with less impact on the code, I don't like it too much as the config itself shouldn't be aware of the importing strategy.

Similarly, in the imports section:

imports:
    - { resource: 'config.yaml', prepend: true }

But still, I'm not sure as you will need to create two config files to achieve that and again the prepend value needs to by pass some loading interfaces where this prepend value doesn't mean anything.

@nicolas-grekas
Copy link
Member

the append strategy requires the extension namespace to be loaded beforehand, which is not required when prepend is used.

Can you please refresh my mind about this: why don't we check the namespace when prepending?

@yceruto
Copy link
Member Author

yceruto commented Nov 17, 2023

Can you please refresh my mind about this: why don't we check the namespace when prepending?

It's on purpose because the extension might not even exist. This feature was initially created to provide preset settings in bundles, so if the extension doesn't exist in your app, that prepended config is ignored.

@yceruto
Copy link
Member Author

yceruto commented Nov 17, 2023

Also because the extension order according to the bundles.php.

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.

Got it, makes sense to me also.

@nicolas-grekas
Copy link
Member

Thank you @yceruto.

@nicolas-grekas nicolas-grekas merged commit 0693c5a into symfony:7.1 Nov 20, 2023
@yceruto yceruto deleted the feature/prepend_extension branch November 20, 2023 12:49
nicolas-grekas added a commit that referenced this pull request Apr 3, 2024
…h file loaders (yceruto)

This PR was merged into the 7.1 branch.

Discussion
----------

[DependencyInjection] Prepending extension configs with file loaders

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | Fix #52789
| License       | MIT

#52636 continuation.

This is another try to simplify even more the prepending extension config strategy for Bundles and Extensions.

Feature: "Import and prepend an extension configuration from an external YAML, XML or PHP file"

> Useful when you need to pre-configure some functionalities in your app, such as mapping some DBAL types provided by your bundle, or simply pre-configuring some default values automatically when your bundle is installed, without losing the ability to override them if desired in userland.

```php
class AcmeFooBundle extends AbstractBundle
{
    public function prependExtension(ContainerConfigurator $container, ContainerBuilder $builder): void
    {
        // Before
        $config = Yaml::parse(file_get_contents(__DIR__.'/../config/doctrine.yaml'));
        $builder->prependExtensionConfig('doctrine', $config['doctrine']);

        // After
        $container->import('../config/doctrine.yaml');
    }
}
```
The "Before" section is limited to importing/parsing this kind of configuration by hand; it doesn't support features like `when@env`, recursive importing, or defining parameters/services in the same file. Further, you can't simply use `ContainerConfigurator::import()` or any `*FileLoader::load()` here, as they will append the extension config instead of prepending it, defeating the prepend method purpose and breaking your app's config strategy. Thus, you are forced to use `$builder->prependExtensionConfig()` as the only way.

> This is because if you append any extension config using `$container->import()`, then you won't be able to change that config in your app as it's merged with priority. If anyone is doing that currently, it shouldn't be intentional.

Therefore, the "After" proposal changes that behavior for `$container->import()`. *Now, all extension configurations encountered in your external file will be prepended instead. BUT, this will only happen here, at this moment, during the `prependExtension()` method.*

Of course, this little behavior change is a "BC break". However, it is a behavior that makes more sense than the previous one, enabling the usage of `ContainerConfigurator::import()` here, which was previously ineffective.

This capability is also available for `YamlFileLoader`, `XmlFileLoader` and `PhpFileLoader` through a new boolean constructor argument:
```php
class AcmeFooBundle extends AbstractBundle
{
    public function prependExtension(ContainerConfigurator $container, ContainerBuilder $builder): void
    {
        // Before
        // - It can't be used for prepending config purposes

        // After
        $loader = new YamlFileLoader($builder, new FileLocator(__DIR__.'/../config/'), prepend: true);
        $loader->load('doctrine.yaml'); // now it prepends extension configs as default behavior
        // or
        $loader->import('prepend/*.yaml');
    }
}
```
These changes only affect the loading strategy for extension configs; everything else keeps working as before.

Cheers!

Commits
-------

6ffd706 prepend extension configs with file loaders
symfony-splitter pushed a commit to symfony/dependency-injection that referenced this pull request Apr 3, 2024
…h file loaders (yceruto)

This PR was merged into the 7.1 branch.

Discussion
----------

[DependencyInjection] Prepending extension configs with file loaders

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | Fix #52789
| License       | MIT

symfony/symfony#52636 continuation.

This is another try to simplify even more the prepending extension config strategy for Bundles and Extensions.

Feature: "Import and prepend an extension configuration from an external YAML, XML or PHP file"

> Useful when you need to pre-configure some functionalities in your app, such as mapping some DBAL types provided by your bundle, or simply pre-configuring some default values automatically when your bundle is installed, without losing the ability to override them if desired in userland.

```php
class AcmeFooBundle extends AbstractBundle
{
    public function prependExtension(ContainerConfigurator $container, ContainerBuilder $builder): void
    {
        // Before
        $config = Yaml::parse(file_get_contents(__DIR__.'/../config/doctrine.yaml'));
        $builder->prependExtensionConfig('doctrine', $config['doctrine']);

        // After
        $container->import('../config/doctrine.yaml');
    }
}
```
The "Before" section is limited to importing/parsing this kind of configuration by hand; it doesn't support features like `when@env`, recursive importing, or defining parameters/services in the same file. Further, you can't simply use `ContainerConfigurator::import()` or any `*FileLoader::load()` here, as they will append the extension config instead of prepending it, defeating the prepend method purpose and breaking your app's config strategy. Thus, you are forced to use `$builder->prependExtensionConfig()` as the only way.

> This is because if you append any extension config using `$container->import()`, then you won't be able to change that config in your app as it's merged with priority. If anyone is doing that currently, it shouldn't be intentional.

Therefore, the "After" proposal changes that behavior for `$container->import()`. *Now, all extension configurations encountered in your external file will be prepended instead. BUT, this will only happen here, at this moment, during the `prependExtension()` method.*

Of course, this little behavior change is a "BC break". However, it is a behavior that makes more sense than the previous one, enabling the usage of `ContainerConfigurator::import()` here, which was previously ineffective.

This capability is also available for `YamlFileLoader`, `XmlFileLoader` and `PhpFileLoader` through a new boolean constructor argument:
```php
class AcmeFooBundle extends AbstractBundle
{
    public function prependExtension(ContainerConfigurator $container, ContainerBuilder $builder): void
    {
        // Before
        // - It can't be used for prepending config purposes

        // After
        $loader = new YamlFileLoader($builder, new FileLocator(__DIR__.'/../config/'), prepend: true);
        $loader->load('doctrine.yaml'); // now it prepends extension configs as default behavior
        // or
        $loader->import('prepend/*.yaml');
    }
}
```
These changes only affect the loading strategy for extension configs; everything else keeps working as before.

Cheers!

Commits
-------

6ffd70662e prepend extension configs with file loaders
symfony-splitter pushed a commit to symfony/http-kernel that referenced this pull request Apr 3, 2024
…h file loaders (yceruto)

This PR was merged into the 7.1 branch.

Discussion
----------

[DependencyInjection] Prepending extension configs with file loaders

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | Fix #52789
| License       | MIT

symfony/symfony#52636 continuation.

This is another try to simplify even more the prepending extension config strategy for Bundles and Extensions.

Feature: "Import and prepend an extension configuration from an external YAML, XML or PHP file"

> Useful when you need to pre-configure some functionalities in your app, such as mapping some DBAL types provided by your bundle, or simply pre-configuring some default values automatically when your bundle is installed, without losing the ability to override them if desired in userland.

```php
class AcmeFooBundle extends AbstractBundle
{
    public function prependExtension(ContainerConfigurator $container, ContainerBuilder $builder): void
    {
        // Before
        $config = Yaml::parse(file_get_contents(__DIR__.'/../config/doctrine.yaml'));
        $builder->prependExtensionConfig('doctrine', $config['doctrine']);

        // After
        $container->import('../config/doctrine.yaml');
    }
}
```
The "Before" section is limited to importing/parsing this kind of configuration by hand; it doesn't support features like `when@env`, recursive importing, or defining parameters/services in the same file. Further, you can't simply use `ContainerConfigurator::import()` or any `*FileLoader::load()` here, as they will append the extension config instead of prepending it, defeating the prepend method purpose and breaking your app's config strategy. Thus, you are forced to use `$builder->prependExtensionConfig()` as the only way.

> This is because if you append any extension config using `$container->import()`, then you won't be able to change that config in your app as it's merged with priority. If anyone is doing that currently, it shouldn't be intentional.

Therefore, the "After" proposal changes that behavior for `$container->import()`. *Now, all extension configurations encountered in your external file will be prepended instead. BUT, this will only happen here, at this moment, during the `prependExtension()` method.*

Of course, this little behavior change is a "BC break". However, it is a behavior that makes more sense than the previous one, enabling the usage of `ContainerConfigurator::import()` here, which was previously ineffective.

This capability is also available for `YamlFileLoader`, `XmlFileLoader` and `PhpFileLoader` through a new boolean constructor argument:
```php
class AcmeFooBundle extends AbstractBundle
{
    public function prependExtension(ContainerConfigurator $container, ContainerBuilder $builder): void
    {
        // Before
        // - It can't be used for prepending config purposes

        // After
        $loader = new YamlFileLoader($builder, new FileLocator(__DIR__.'/../config/'), prepend: true);
        $loader->load('doctrine.yaml'); // now it prepends extension configs as default behavior
        // or
        $loader->import('prepend/*.yaml');
    }
}
```
These changes only affect the loading strategy for extension configs; everything else keeps working as before.

Cheers!

Commits
-------

6ffd70662e prepend extension configs with file loaders
@fabpot fabpot mentioned this pull request May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DependencyInjection DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants