Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] Auto-generateconfig/reference.php to assist in writing and discovering app's configuration#62129
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
5db4679 to6171809Compare| unset($knownEnvs['all']); | ||
| $parameters['.kernel.config_dir'] =$this->getConfigDir(); | ||
| $parameters['.kernel.known_envs'] =$knownEnvs; | ||
| $parameters['.kernel.all_bundles'] =$allBundles; |
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.
these 3 new parameters are needed forPhpConfigReferenceDumpPass
| * | ||
| * @psalm-import-type ImportsConfig from AppReference | ||
| * @psalm-import-type ParametersConfig from AppReference | ||
| * @psalm-import-type ServicesConfig from AppReference |
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.
phpstorm doesn't supportpsalm-import-type properly yet (neither for static analysis nor autocompletion)
/cc@pronskiy
| * parameters?: ParametersConfig, | ||
| * services?: ServicesConfig, | ||
| * test?: TestConfig, | ||
| * ...<'when@dev'|'when@prod'|'when@test', array{ |
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.
phpstan/phpstorm don't support "variadic" shapes yet, this is something psalm does support
/cc@pronskiy and@ondrejmirtes
Also: it could be possible to generate a more accurate shape that'd group enabled bundles per-env. I left this for a future PR as my early experiments did also decrease the readability of the shape. I preferred this one, which is good enough for autocompletion. Stricter validation happens anyway when compiling the config.
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.
Using string literal for the array key is a good idea.
it could be possible to generate a more accurate shape that'd group enabled bundles per-env
That would also skip the "variadic" support issue.
/** * @param array{ * imports?: ImportsConfig, * parameters?: ParametersConfig, * services?: ServicesConfig, * test?: TestConfig, * "when@dev"?: array{ * imports?: ImportsConfig, * parameters?: ParametersConfig, * services?: ServicesConfig, * test?: TestConfig, * } * "when@prod"?: array{ * imports?: ImportsConfig, * parameters?: ParametersConfig, * services?: ServicesConfig, * test?: TestConfig, * } * "when@test"?: array{ * imports?: ImportsConfig, * parameters?: ParametersConfig, * services?: ServicesConfig, * test?: TestConfig, * } * } $config */
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.
Let's leave this for a next PR
| * 'when@dev'?: array<string, RouteConfig|ImportConfig|AliasConfig>, | ||
| * 'when@prod'?: array<string, RouteConfig|ImportConfig|AliasConfig>, | ||
| * 'when@test'?: array<string, RouteConfig|ImportConfig|AliasConfig>, | ||
| * ...<string, ImportConfig|RouteConfig|AliasConfig> |
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.
Autocompletion by the IDE should account for the key of course /cc@pronskiy
| */ | ||
| public function __construct(array $config = []) | ||
| {'.$body.' | ||
| }', ['PARAM_TYPE' => ArrayShapeGenerator::generate($node)]); |
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.
this reverts attaching shapes to config builders and leads to many updates in fixtures
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.
This also means less tests coveringArrayShapeGenerators.
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 should implement tests for the ArrayShapeGenerator directly producing similar coverage then.
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.
Let's improve the coverage in a later PR - help welcome!
| }); | ||
| // Expose AppReference::config() as App::config() | ||
| if (!class_exists(App::class)) { | ||
| class_alias(AppReference::class, App::class); |
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.
This is critical: it solves a 🐔-n-🥚 problem, since theconfig/reference.php is not generated yet the first time the app is compiled. Since thisApp::config() method is an identity one, we don't care if the shape is here or not. So we can just alias classApp to its baseAppReference class.
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.
In fact, theconfig/reference.php file is never loaded by Symfony, even when it exists. However, if someone explicitly requires it, that works the same way, but there is no benefit to doing so. This file is there only for static analysis.
| } | ||
| if (\is_object($result) &&\is_callable($result)) { | ||
| $result =$this->callConfigurator($result,newContainerConfigurator($this->container,$this,$this->instanceof,$path,$resource,$this->env),$path); |
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.
All changes next in this file are reverts of support for dealing with*Config objects as return values, while keeping support for returning arrays directly.
| foreach ($xpath->query('//container:when') ?: []as$root) { | ||
| $knownEnvs[$root->getAttribute('env')] =true; | ||
| } | ||
| $this->container->setParameter('.container.known_envs',$knownEnvs); |
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.
All loaders now populate this new.container.known_envs build-time parameter so that compiler passes could know which alternative envs exist in an app.
| // per-env configuration | ||
| if ($this->env &&isset($content['when@'.$this->env])) { | ||
| if (!\is_array($content['when@'.$this->env])) { | ||
| thrownewInvalidArgumentException(\sprintf('The "when@%s" key should contain an array in "%s". Check your YAML syntax.',$this->env,$path)); |
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.
Check your YAML syntax.
the logic is now used to load more than yaml trees.
| $this->env =null; | ||
| try { | ||
| $this->loadContent($content['when@'.$env],$path); | ||
| }finally { |
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.
resetting the env makes no sense
6171809 toc1467e7Comparesrc/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/PhpConfigReferenceDumpPass.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/PhpConfigReferenceDumpPass.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| } | ||
| namespace Symfony\Component\Routing\Loader\Configurator; |
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.
is this actually valid ? Don't we need to use the syntax with curly braces around the code when wanting to use several namespaces in the same PHP file ?
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.
Yes it's validhttps://www.php.net/manual/en/language.namespaces.definitionmultiple.php, it works with or without curly braces
| } | ||
| unset($knownEnvs['all']); | ||
| $parameters['.kernel.config_dir'] =$this->getConfigDir(); | ||
| $parameters['.kernel.known_envs'] =$knownEnvs; |
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.
shouldn't we usearray_keys($knownEnvs) here, to produce a parameter containing a list of envs instead of having them as keys ? PhpConfigReferenceDumpPass is currently doing it anyway, but we might reuse it in other passes that would need the same.
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 updated to make the parameters simple lists (this requires more processing when adding to the parameter, but I guess this is cheap since the list is short)
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/PhpConfigReferenceDumpPass.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| $this->loadRoutes($collection,$result,$path,$file); | ||
| $loader =newYamlFileLoader($this->locator,$this->env); | ||
| $loader->setResolver($this->resolver ??newLoaderResolver([$this])); | ||
| (new \ReflectionMethod(YamlFileLoader::class,'loadContent'))->invoke($loader,$collection,$result,$path,$file); |
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.
Instead of using reflection to invoke a private method, I suggest extracting an internal object implementing this logic, which would then be used by each loader
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.
There is the same in PhpFileLoader. PHPstorm doesn't like it.
symfony/src/Symfony/Component/Routing/Loader/PhpFileLoader.php
Lines 120 to 141 in742d742
| \Closure::bind(function ()use ($collection,$routes,$path,$file) { | |
| foreach ($routesas$name =>$config) { | |
| if (str_starts_with($when =$name,'when@')) { | |
| if (!$this->env ||'when@'.$this->env !==$name) { | |
| continue; | |
| } | |
| $when .='" when "@'.$this->env; | |
| }elseif (!$configinstanceof RoutesConfig) { | |
| $config = [$name =>$config]; | |
| }elseif (!\is_int($name)) { | |
| thrownewInvalidArgumentException(\sprintf('Invalid key "%s" returned for the "%s" config builder; none or "when@%%env%%" expected in file "%s".',$name,get_debug_type($config),$path)); | |
| } | |
| if ($configinstanceof RoutesConfig) { | |
| $config =$config->routes; | |
| }elseif (!\is_array($config)) { | |
| thrownewInvalidArgumentException(\sprintf('The "%s" key should contain an array in "%s".',$name,$path)); | |
| } | |
| $this->loadContent($collection,$config,$path,$file); | |
| } | |
| },$loader, YamlFileLoader::class)(); |
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.
@stof would you like to make a PR to improve that?
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 leaving this for a future PR. Things are internal so that's fine. PR is big enough :)
Uh oh!
There was an error while loading.Please reload this page.
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.
Awesome 🎊
| * | ||
| * @return array<string, array|bool|string|int|float|\UnitEnum|null> | ||
| */ | ||
| protectedfunctiongetKernelParameters():array |
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.
Projects using this trait and overriding this method will have to call it explicitly.
Just to confirm that making the call toMicroKernelTrait::getKernelParameters() required would be a breaking change. Which is not the case now.
If people overridegetKernelParameters, they have to update theirApp\Kernel class like this to enable the new feature:
namespaceApp;useSymfony\Bundle\FrameworkBundle\Kernel\MicroKernelTrait;useSymfony\Component\HttpKernel\KernelasBaseKernel;class Kernelextends BaseKernel{use MicroKernelTrait { getKernelParametersasprivate getMicroKernelParameters; }protectedfunctiongetKernelParameters():array {$parameters =$this->getMicroKernelParameters();$parameters['kernel.param'] ='custom_kernel_value';return$parameters; }}
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.
Correct. And the pass skips generating thereference.php file if the.kernel.config_dir parameter is missing, so that there's a nice incentive to do the change, but it's not a blocker either.
| * parameters?: ParametersConfig, | ||
| * services?: ServicesConfig, | ||
| * test?: TestConfig, | ||
| * ...<'when@dev'|'when@prod'|'when@test', array{ |
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.
Using string literal for the array key is a good idea.
it could be possible to generate a more accurate shape that'd group enabled bundles per-env
That would also skip the "variadic" support issue.
/** * @param array{ * imports?: ImportsConfig, * parameters?: ParametersConfig, * services?: ServicesConfig, * test?: TestConfig, * "when@dev"?: array{ * imports?: ImportsConfig, * parameters?: ParametersConfig, * services?: ServicesConfig, * test?: TestConfig, * } * "when@prod"?: array{ * imports?: ImportsConfig, * parameters?: ParametersConfig, * services?: ServicesConfig, * test?: TestConfig, * } * "when@test"?: array{ * imports?: ImportsConfig, * parameters?: ParametersConfig, * services?: ServicesConfig, * test?: TestConfig, * } * } $config */
| */ | ||
| public function __construct(array $config = []) | ||
| {'.$body.' | ||
| }', ['PARAM_TYPE' => ArrayShapeGenerator::generate($node)]); |
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.
This also means less tests coveringArrayShapeGenerators.
| @@ -0,0 +1,104 @@ | |||
| <?php | |||
| namespaceSymfony\Component\DependencyInjection\Loader\Configurator; | |||
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.
No moreSymfony\Config namespace?
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.
Nope: this would create more complexity, eg to duplicate the functions in the namespace + weird autoloading rules.
bbd5451 toc9609c5CompareThanks for the reviews, I addressed your comments! |
c9609c5 to493b781CompareThere 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. Nothing important in my comments, they may be the subject of later PR.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| * parameters?: ParametersConfig, | ||
| * services?: ServicesConfig, | ||
| * ...<string, ExtensionConfig>, | ||
| * ...<string, array{ // extra keys must follow the when@%env% pattern |
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.
As this, I would merge bothunsealed shapes; since they have the same key type.
...<string, ExtensionConfig|array{ }>But this would conflict with yourstr_replace.
| return$clone; | ||
| } | ||
| } | ||
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.
That was in 7.4:#61490
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/PhpConfigReferenceDumpPass.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/PhpConfigReferenceDumpPass.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
GromNaN left a comment• edited by nicolas-grekas
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by nicolas-grekas
Uh oh!
There was an error while loading.Please reload this page.
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 tried insymfony/demo, you can see what it looks like inGromNaN/symfony-demo#4
The inline PhpDoc comments arenot fully supported by PHPStorm.
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/PhpConfigReferenceDumpPass.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/PhpConfigReferenceDumpPass.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| }); | ||
| // Expose AppReference::config() as App::config() | ||
| if (!class_exists(App::class)) { | ||
| class_alias(AppReference::class, App::class); |
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.
In fact, theconfig/reference.php file is never loaded by Symfony, even when it exists. However, if someone explicitly requires it, that works the same way, but there is no benefit to doing so. This file is there only for static analysis.
alexandre-daubois commentedOct 23, 2025 • 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.
I played with it a bit, that's nice and it works well! I stumbled across a problem however: when testing your first example: <?phpnamespaceSymfony\Component\DependencyInjection\Loader\Configurator;return App::config(['App\\' => ['resource' =>'../src/' ],]); It doesn't compile and I get this error: Do I miss something? Tested with a minimal project and default configuration. |
My example in the PR description was incorrect. |
alexandre-daubois left a comment• 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.
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.
Works well with the new example, thanks!
I think it should be mentioned inreference.php that the file is meant to be committed to the project? Such generated files are usually excluded from projects and people may not understand it shouldn't be ignored. Mentioning this would avoid ambiguity?
GromNaN commentedOct 23, 2025 • 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.
The file is only useful for static analysis and IDE completion. It's not required to run the application. It can be versioned or not, whatever. |
Alright, maybe the description saying "This means that the file should be committed" should be interpreted as "the filemay be committed". Got it 👍 |
nicolas-grekas commentedOct 23, 2025 • 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.
PR updated:
Support for These changes are in asecond commit for ppl that did review the first one. |
d4b863e to1625625Compare…riting and discovering app's configuration
1625625 to22349f5Compare2a1d924 intosymfony:7.4Uh oh!
There was an error while loading.Please reload this page.
Let's iterate :) PR welcome to fine-tune this if needed. |
First tried out today to install 7.4 and this file feels very strange to have in my project code as a commited file. My personal opinion as it is autogenerated and should be in my opinion just be part of a var/cache directory and not part of a project code, like we have the var/cache/dev/App_KernelDevDebugContainer.xml for IDE and Tools already. It's for phpstan symfony normal to configure the container directory, and PHPStorm Plugin the cache directory. Even things like Also question is this |
phpstan supports reading annotations with the |
Okay thats nice to know that psalm perfixes work also for phpstan, it would also work if its in |
nicolas-grekas commentedOct 24, 2025 • 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.
About this, I explained why I put this in |
This reverts commit5abeb29.The scanned file no longer exists sincesymfony/symfony#62129, which aims precisely atmaking the scan unnecessary (among other things).
Uh oh!
There was an error while loading.Please reload this page.
This PR reverts#61490 and#61885, and builds on#61894.
These reverts explain a big chunk of the attached patch.
This adds a compiler pass that generates a
config/reference.phpfile.This file contains two classes that define array-shapes for app's and routing configuration.
Part of these shapes are auto-generated from the list of bundles found in
config/bundles.php.The
config/reference.phpfile could be loaded by a new line in the "autoload" entry of composer.json files:"classmap": ["config/"]- but it's not strictly needed. This means that the file might be committed. This is on purpose: as the name suggests, this file is also a config reference for human readers. Having to commit the changes is also a nice way to convey config improvements to the community - at least for ppl that review their commits ;). It also solves a discovery problem that happens with phpstan/etc having a hard time to find the classes currently generated for config builders in the cache directory.With this PR,
config/services.phpcould start as such:and
config/routes.phpwould start as:The generated shapes use advanced features that are not fully supported by phpstan / phpstorm. But the gap should be thin enough to be closed soon.
PS:https://symfony.com/blog/new-in-symfony-7-4-deprecated-xml-configuration will need an update.