Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged

Conversation

@yceruto
Copy link
Member

@ycerutoyceruto commentedDec 1, 2023
edited
Loading

QA
Branch?7.1
Bug fix?no
New feature?yes
Deprecations?no
IssuesFix#52789
LicenseMIT

#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.

class AcmeFooBundleextends AbstractBundle{publicfunctionprependExtension(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 likewhen@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.

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 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 ofContainerConfigurator::import() here, which was previously ineffective.

This capability is also available forYamlFileLoader,XmlFileLoader andPhpFileLoader through a new boolean constructor argument:

class AcmeFooBundleextends AbstractBundle{publicfunctionprependExtension(ContainerConfigurator$container,ContainerBuilder$builder):void    {// Before// - It can't be used for prepending config purposes// After$loader =newYamlFileLoader($builder,newFileLocator(__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!

solverat reacted with thumbs up emoji
@ycerutoyceruto added this to the7.1 milestoneDec 1, 2023
@carsonbotcarsonbot changed the title[DependencyInjection] Prepending extension configs with file loadersPrepending extension configs with file loadersDec 1, 2023
@carsonbotcarsonbot changed the titlePrepending extension configs with file loaders[DependencyInjection] Prepending extension configs with file loadersDec 1, 2023
@ycerutoycerutoforce-pushed theprepend_extension_config branch from058e3da to1f62898CompareDecember 1, 2023 07:14
@ycerutoyceruto added the DXDX = Developer eXperience (anything that improves the experience of using Symfony) labelDec 1, 2023
Copy link
Member

@nicolas-grekasnicolas-grekas left a 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?

@ycerutoycerutoforce-pushed theprepend_extension_config branch from1f62898 tob93e450CompareDecember 1, 2023 14:06
@yceruto
Copy link
MemberAuthor

I'm just wondering about recursive imports: are they also reversed?

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
Copy link
MemberAuthor

yceruto commentedDec 1, 2023
edited
Loading

The behavior is set up for the whole process, so yes, all extension configs imported recursively will be prepended too.

Which is expected if you're importing from the prepend method.

@nicolas-grekas
Copy link
Member

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
Copy link
MemberAuthor

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?

I will confirm with a test case to ensure we know what the behavior will be.

Copy link
Member

@nicolas-grekasnicolas-grekas left a 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)

@yceruto
Copy link
MemberAuthor

Thanks Nicolas! rebased done.

@yceruto
Copy link
MemberAuthor

yceruto commentedJan 14, 2024
edited
Loading

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.

GromNaN reacted with thumbs up emoji

@ycerutoycerutoforce-pushed theprepend_extension_config branch fromfffad85 toc97b569CompareFebruary 9, 2024 15:29
@ycerutoycerutoforce-pushed theprepend_extension_config branch fromc97b569 to8e2b365CompareMarch 16, 2024 16:11
@yceruto
Copy link
MemberAuthor

Hey@nicolas-grekas, I rebased, rechecked, and updated another part wherewhen@env wasn't working as expected. I added another test case to cover it. So, after a while, this looks ready to me. Wdyt?

Copy link
Member

@nicolas-grekasnicolas-grekas left a 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 🚀

@ycerutoycerutoforce-pushed theprepend_extension_config branch from8e2b365 to4d731beCompareMarch 18, 2024 13:18
@nicolas-grekas
Copy link
Member

Anyone @symfony/mergers ? (after a small rebase)

@ycerutoycerutoforce-pushed theprepend_extension_config branch from4d731be to6ffd706CompareMarch 25, 2024 20:45
@ycerutoyceruto added the ❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze" labelApr 2, 2024
@nicolas-grekas
Copy link
Member

Thank you@yceruto.

yceruto reacted with hooray emoji

@nicolas-grekasnicolas-grekas merged commit4ce4e5e intosymfony:7.1Apr 3, 2024
@ycerutoyceruto deleted the prepend_extension_config branchApril 3, 2024 20:57
fabpot added a commit that referenced this pull requestApr 5, 2024
…(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
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull requestApr 15, 2024
…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
@fabpotfabpot mentioned this pull requestMay 2, 2024
nicolas-grekas added a commit that referenced this pull requestMay 17, 2024
…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
nicolas-grekas added a commit that referenced this pull requestAug 7, 2024
…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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Labels

BC BreakDependencyInjectionDXDX = Developer eXperience (anything that improves the experience of using Symfony)Feature❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze"Status: Reviewed

Projects

None yet

Milestone

7.1

Development

Successfully merging this pull request may close these issues.

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

3 participants

@yceruto@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp