Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] Prepending extension configs with file loaders#52843
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
058e3da to1f62898Compare
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm just wondering about recursive imports: are they also reversed?
Uh oh!
There was an error while loading.Please reload this page.
1f62898 tob93e450Compareyceruto commentedDec 1, 2023
The behavior is set up for the whole process, so yes, all extension configs imported recursively will be prepended too. However, unless there are split configs of the same extension distributed into several files matching the same config key, it doesn't matter. |
yceruto commentedDec 1, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Which is expected if you're importing from the prepend method. |
nicolas-grekas commentedDec 4, 2023
Maybe you already answered but just to be sure: if I have a yaml file that imports two other files, I'd expect them to be loaded in top-down order, is that going to be the case? |
yceruto commentedDec 4, 2023
I will confirm with a test case to ensure we know what the behavior will be. |
src/Symfony/Component/DependencyInjection/Tests/Fixtures/config/packages/ping.yamlShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/DependencyInjection/Tests/Extension/AbstractExtensionTest.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM (small rebase needed)
f1f602a to66bf43aCompareyceruto commentedJan 2, 2024
Thanks Nicolas! rebased done. |
yceruto commentedJan 14, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Friendly ping @symfony/mergers, there is a BC behavior break on the table here; it would be great to have a double check in case we miss something. |
66bf43a to80e97eaCompare80e97ea tofffad85CompareUh oh!
There was an error while loading.Please reload this page.
fffad85 toc97b569Comparec97b569 to8e2b365Compareyceruto commentedMar 16, 2024
Hey@nicolas-grekas, I rebased, rechecked, and updated another part where |
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just some minor CS things and 🚀
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
8e2b365 to4d731beComparenicolas-grekas commentedMar 25, 2024
Anyone @symfony/mergers ? (after a small rebase) |
4d731be to6ffd706Comparenicolas-grekas commentedApr 3, 2024
Thank you@yceruto. |
…(OskarStark)This PR was squashed before being merged into the 7.1 branch.Discussion----------[DependencyInjection] Document BC break in UPGRADE file| Q | A| ------------- | ---| Branch? | 7.1| Bug fix? | no| New feature? | no| Deprecations? | no| Issues | Follows#52843| License | MITCommits-------055892d [DependencyInjection] Document BC break in UPGRADE file
…ities (yceruto)This PR was merged into the 7.1 branch.Discussion----------[DependencyInjection] Updating prepend extension capabilitiesFixes#19735I'm also suggesting removing the append example in the prepend extension method. As explained in PRsymfony/symfony#52843 and related issues, it's flawed and should be discouraged from the official documentation.I'll create a new PR to target the lower branch and remove it there too.Commits-------25c5adf Updating prepend extension capabilities
…nerConfigurator (MatTheCat)This PR was merged into the 7.1 branch.Discussion----------[DependencyInjection] Process PHP configs using the ContainerConfigurator| Q | A| ------------- | ---| Branch? | 7.1| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues |Fix#54852| License | MITSince#52843, `ContainerConfigurator::extension` is not called anymore which means `ParamConfigurator`s are no longer processed and converted to strings, and make nodes validation fail: e.g. `framework.secret` would receive an `EnvConfigurator` and throw because it is not a scalar.Commits-------e6dc6be [DependencyInjection] Process PHP configs using the ContainerConfigurator
…pend extension method (yceruto)This PR was merged into the 7.1 branch.Discussion----------[DependencyInjection] Fix importing PHP configs in the prepend extension method| Q | A| ------------- | ---| Branch? | 7.1| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues |Fix#57936| License | MITI missed this nesting case during#52843.Commits-------94df570 Fix importing PHP config in prepend extension method
Uh oh!
There was an error while loading.Please reload this page.
#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"
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 useContainerConfigurator::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.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 theprependExtension()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,XmlFileLoaderandPhpFileLoaderthrough a new boolean constructor argument:These changes only affect the loading strategy for extension configs; everything else keeps working as before.
Cheers!