-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[FrameworkBundle] Auto-generate config/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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FrameworkBundle] Auto-generate config/reference.php to assist in writing and discovering app's configuration
#62129
Conversation
5db4679 to
6171809
Compare
| 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 for PhpConfigReferenceDumpPass
| * | ||
| * @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 support psalm-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 covering ArrayShapeGenerators.
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 the config/reference.php is not generated yet the first time the app is compiled. Since this App::config() method is an identity one, we don't care if the shape is here or not. So we can just alias class App to its base AppReference 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, the config/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, new ContainerConfigurator($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])) { | ||
| throw new InvalidArgumentException(\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 to
c1467e7
Compare
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/PhpConfigReferenceDumpPass.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/PhpConfigReferenceDumpPass.php
Show resolved
Hide resolved
| } | ||
| } | ||
| 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 valid https://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 use array_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.php
Show resolved
Hide resolved
| $this->loadRoutes($collection, $result, $path, $file); | ||
| $loader = new YamlFileLoader($this->locator, $this->env); | ||
| $loader->setResolver($this->resolver ?? new LoaderResolver([$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 in 742d742
| \Closure::bind(function () use ($collection, $routes, $path, $file) { | |
| foreach ($routes as $name => $config) { | |
| if (str_starts_with($when = $name, 'when@')) { | |
| if (!$this->env || 'when@'.$this->env !== $name) { | |
| continue; | |
| } | |
| $when .= '" when "@'.$this->env; | |
| } elseif (!$config instanceof RoutesConfig) { | |
| $config = [$name => $config]; | |
| } elseif (!\is_int($name)) { | |
| throw new InvalidArgumentException(\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 ($config instanceof RoutesConfig) { | |
| $config = $config->routes; | |
| } elseif (!\is_array($config)) { | |
| throw new InvalidArgumentException(\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 :)
GromNaN
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.
Awesome 🎊
| * | ||
| * @return array<string, array|bool|string|int|float|\UnitEnum|null> | ||
| */ | ||
| protected function getKernelParameters(): 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 to MicroKernelTrait::getKernelParameters() required would be a breaking change. Which is not the case now.
If people override getKernelParameters, they have to update their App\Kernel class like this to enable the new feature:
namespace App;
use Symfony\Bundle\FrameworkBundle\Kernel\MicroKernelTrait;
use Symfony\Component\HttpKernel\Kernel as BaseKernel;
class Kernel extends BaseKernel
{
use MicroKernelTrait {
getKernelParameters as private getMicroKernelParameters;
}
protected function getKernelParameters(): 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 the reference.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 covering ArrayShapeGenerators.
| @@ -0,0 +1,104 @@ | |||
| <?php | |||
|
|
|||
| namespace Symfony\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 more Symfony\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 to
c9609c5
Compare
|
Thanks for the reviews, I addressed your comments! |
c9609c5 to
493b781
Compare
GromNaN
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. Nothing important in my comments, they may be the subject of later PR.
src/Symfony/Bundle/FrameworkBundle/Tests/Kernel/MicroKernelTraitTest.php
Show resolved
Hide resolved
| * 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 both unsealed shapes; since they have the same key type.
...<string, ExtensionConfig|array{ }>
But this would conflict with your str_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
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/PhpConfigReferenceDumpPass.php
Outdated
Show resolved
Hide resolved
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 in symfony/demo, you can see what it looks like in GromNaN/symfony-demo#4
The inline PhpDoc comments are not fully supported by PHPStorm.
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/PhpConfigReferenceDumpPass.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/PhpConfigReferenceDumpPass.php
Outdated
Show resolved
Hide resolved
| }); | ||
| // 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, the config/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.
|
I played with it a bit, that's nice and it works well! I stumbled across a problem however: when testing your first example: <?php
namespace Symfony\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. |
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 in reference.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?
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 file may be committed". Got it 👍 |
|
PR updated:
Support for These changes are in a second commit for ppl that did review the first one. |
d4b863e to
1625625
Compare
…riting and discovering app's configuration
1625625 to
22349f5
Compare
|
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 |
About this, I explained why I put this in |
This reverts commit 5abeb29. The scanned file no longer exists since symfony/symfony#62129, which aims precisely at making the scan unnecessary (among other things).
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.