Skip to content

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Oct 22, 2025

Q A
Branch? 7.4
Bug fix? no
New feature? yes
Deprecations? no
Doc PR symfony/symfony-docs#21511
License MIT

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.php file.
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.php file 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.php could start as such:

<?php

namespace Symfony\Component\DependencyInjection\Loader\Configurator;

return App::config([
    'services' => [
        'App\\' => [
            'resource' => '../src/'
        ],
    ],
]);

and config/routes.php would start as:

<?php

namespace Symfony\Component\Routing\Loader\Configurator;

return Routes::config([
    'controllers' => [
        'resource' => 'attributes',
        'type' => 'tagged_services',
    ]
]);

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.

unset($knownEnvs['all']);
$parameters['.kernel.config_dir'] = $this->getConfigDir();
$parameters['.kernel.known_envs'] = $knownEnvs;
$parameters['.kernel.all_bundles'] = $allBundles;
Copy link
Member Author

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
Copy link
Member Author

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{
Copy link
Member Author

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.

Copy link
Member

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
     */

Copy link
Member Author

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>
Copy link
Member Author

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)]);
Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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);
Copy link
Member Author

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.

Copy link
Member

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);
Copy link
Member Author

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);
Copy link
Member Author

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));
Copy link
Member Author

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 {
Copy link
Member Author

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

@nicolas-grekas nicolas-grekas force-pushed the config-static-shape-method branch from 6171809 to c1467e7 Compare October 22, 2025 14:50
}
}
namespace Symfony\Component\Routing\Loader\Configurator;
Copy link
Member

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 ?

Copy link
Member

@Kocal Kocal Oct 22, 2025

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;
Copy link
Member

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.

Copy link
Member Author

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)

$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);
Copy link
Member

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

Copy link
Member

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.

\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)();

Copy link
Member

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?

Copy link
Member Author

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 :)

Copy link
Member

@GromNaN GromNaN left a 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
Copy link
Member

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;
    }
}

Copy link
Member Author

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{
Copy link
Member

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)]);
Copy link
Member

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;
Copy link
Member

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?

Copy link
Member Author

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.

@nicolas-grekas
Copy link
Member Author

Thanks for the reviews, I addressed your comments!

@nicolas-grekas nicolas-grekas force-pushed the config-static-shape-method branch from c9609c5 to 493b781 Compare October 22, 2025 19:58
Copy link
Member

@GromNaN GromNaN left a 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.

* parameters?: ParametersConfig,
* services?: ServicesConfig,
* ...<string, ExtensionConfig>,
* ...<string, array{ // extra keys must follow the when@%env% pattern
Copy link
Member

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;
}
}

Copy link
Member

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

GromNaN added a commit to GromNaN/symfony-demo that referenced this pull request Oct 22, 2025
GromNaN added a commit to GromNaN/symfony-demo that referenced this pull request Oct 22, 2025
GromNaN added a commit to GromNaN/symfony-demo that referenced this pull request Oct 22, 2025
Copy link
Member

@GromNaN GromNaN left a 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.

Image

});
// Expose AppReference::config() as App::config()
if (!class_exists(App::class)) {
class_alias(AppReference::class, App::class);
Copy link
Member

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.

@alexandre-daubois
Copy link
Member

alexandre-daubois commented Oct 23, 2025

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:

In FileLoader.php line 177:
                                                                                                                                                                                    
  Container extension "App\" is not registered in /Users/alex/PhpstormProjects/config/config/services.php (which is being imported from "/Users/alex/PhpstormProjects/config/src/K  
  ernel.php").                                                                                                                                                                      
                                                                                                                                                                                    

In ContainerBuilder.php line 224:
                                                 
  Container extension "App\" is not registered.                                           

Do I miss something? Tested with a minimal project and default configuration.

@nicolas-grekas
Copy link
Member Author

My example in the PR description was incorrect.

Copy link
Member

@alexandre-daubois alexandre-daubois left a 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?

@GromNaN
Copy link
Member

GromNaN commented Oct 23, 2025

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.

@alexandre-daubois
Copy link
Member

Alright, maybe the description saying "This means that the file should be committed" should be interpreted as "the file may be committed". Got it 👍

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Oct 23, 2025

PR updated:

  • I removed generation of int<min,max> for now
  • specific when@env are listed as suggested by @GromNaN - this makes part of autocompletion work without waiting for a future phpStorm release
  • some improvements to the shape generation
  • psalm-type are imported by duplicating them, further reducing the immediate need for a future phpStorm version

Support for ...<> in shapes would still be highly desired as we have to use them for routes and for services.
Note also that phpStorm autocompletion could be improved when nesting into an array (with branch-level elimination of impossible suggestions)

These changes are in a second commit for ppl that did review the first one.

@nicolas-grekas nicolas-grekas force-pushed the config-static-shape-method branch from d4b863e to 1625625 Compare October 23, 2025 14:16
…riting and discovering app's configuration
@nicolas-grekas nicolas-grekas force-pushed the config-static-shape-method branch from 1625625 to 22349f5 Compare October 23, 2025 14:33
@nicolas-grekas nicolas-grekas merged commit 2a1d924 into symfony:7.4 Oct 23, 2025
4 of 6 checks passed
@nicolas-grekas nicolas-grekas deleted the config-static-shape-method branch October 23, 2025 14:34
@nicolas-grekas
Copy link
Member Author

Let's iterate :) PR welcome to fine-tune this if needed.

@alexander-schranz
Copy link
Contributor

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 FrameworkConfig could be provided by the FrameworkBundle directly, but understand to have a single generated file, like the container .

Also question is this Psalm only or does PHPStan support this? What about people not using that tools. Can the generation be disabled? Can it moved to a none commited directory?

@stof
Copy link
Member

stof commented Oct 24, 2025

phpstan supports reading annotations with the @psalm prefix as long as it understands them (the opposite is also true). This is done so that each maintainer can use the prefix of the tool they use in their code, while projects installing them as dependency can choose a different tool.
PHPStorm also reads those types, providing better completion. And maybe also the PHP language server of VS Code.

@alexander-schranz
Copy link
Contributor

Okay thats nice to know that psalm perfixes work also for phpstan, it would also work if its in var or var/cache directory.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Oct 24, 2025

be part of a var/cache directory

About this, I explained why I put this in config/reference.php in the PR description. I stick to this rationale.

This was referenced Oct 27, 2025
greg0ire added a commit to greg0ire/DoctrineBundle that referenced this pull request Nov 1, 2025
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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants