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] Fix FC with Config v8#62138
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
Conversation
f107568 intosymfony:7.4Uh oh!
There was an error while loading.Please reload this page.
| if (\is_bool($generator)) { | ||
| $prepend =$generator; | ||
| $generator =null; | ||
| } |
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.
should we trigger a deprecation in theelse branch ?
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.
The issue is that we should still support config builders, so we'd always trigger that one (unless we go with an extra silencing argument, but as I wrote the other comment, this might be good enough in practice.)
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.
We can trigger a deprecation in 8.1
| $container->registerExtension(new \AcmeExtension()); | ||
| $container->prependExtensionConfig('acme', ['foo' =>'bar']); | ||
| $loader =newPhpFileLoader($container,newFileLocator(\dirname(__DIR__).'/Fixtures/config'),'prod',newConfigBuilderGenerator(sys_get_temp_dir()),true); | ||
| $loader =newPhpFileLoader($container,newFileLocator(\dirname(__DIR__).'/Fixtures/config'),'prod',prepend:true); |
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.
Tests should not use named parameters. This hides unintentional BC breaks that would be detected due to tests being forced to be updated, by making tests used the public API.
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.
Yeah, I did a quick FC layer, that's true :)
I think all this is confidential enough to make it this way.
We can always improve later on.
Required for#62113