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

Allow prepending extension config in DI Loader (XML/YAML) #52789

Closed
solverat opened this issue Nov 29, 2023 · 3 comments · Fixed by #52843
Closed

Allow prepending extension config in DI Loader (XML/YAML) #52789

solverat opened this issue Nov 29, 2023 · 3 comments · Fixed by #52843

Comments

@solverat
Copy link

Description

Given: A Bundle extension with an implemented PrependExtensionInterface

public function prepend(ContainerBuilder $container): void
{
    $loader = new YamlFileLoader($container, new FileLocator(__DIR__ . '/../../config'));
 
    if ($specialConditionTriggers) {
        $loader->load('my_config_and_services.yaml');
    }
}

The Problem: The Loader (in this example, I'm using the YAML file loader) calls loadFromExtension, and so the configuration part gets appended:

$this->container->loadFromExtension($namespace, $values);
}

Adding a new argument to the loader methods load() is probably not the best solution regarding BC, but maybe we could add a second load method? For example:

public function prependLoad(mixed $resource, string $type = null) {}

For now, I need to split up those configuration / service files, then I also have to parse the configuration manually to finally use prependExtensionConfig in my extension. This feels a bit odd because there is a service that could actually do it for me. :)

Example

No response

@nicolas-grekas
Copy link
Member

/cc @yceruto maybe?

@yceruto
Copy link
Member

yceruto commented Nov 30, 2023

Thanks Nicolas for the ping.

Indeed, adding a new argument to existing load() methods is complex because the value must travel across different strategies/processes before reaching the place to load/prepend that config, so the impact would be too high.

I was struggling recently with this topic while I was adding this related improvement in 7.1 #52636 and couldn't find a fancy way to prepend config from an external file.

Perhaps a new method, as you suggested, @solverat, could be a solution to setting up the prepending strategy for the entire loading operation.

This feature definitely got my attention, so I will continue researching it.

@yceruto
Copy link
Member

yceruto commented Dec 1, 2023

Here we go #52843

nicolas-grekas added a commit that referenced this issue 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
javiereguiluz added a commit to symfony/symfony-docs that referenced this issue Apr 15, 2024
…pend extension method (yceruto)

This PR was merged into the 6.4 branch.

Discussion
----------

[DependencyInjection] Discourage appending config on prepend extension method

Related to symfony/symfony#52789 and #19738

Commits
-------

4e82adb Discourage append config on prepend extension method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants