Skip to content

Commit 97429b7

Browse files
fabpotfabpot
committed
bug symfony#58995 [Config] Do not generate unreachable configuration paths (bobvandevijver)
This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [Config] Do not generate unreachable configuration paths | Q | A | ------------- | --- | Branch? | 6.4 <!-- see below --> | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Issues | - <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead --> | License | MIT <!-- Replace this notice by a description of your feature/bugfix. This will help reviewers and should be a good start for the documentation. Additionally (see https://symfony.com/releases): - Always add tests and ensure they pass. - Bug fixes must be submitted against the lowest maintained branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against the latest branch. - For new features, provide some code snippets to help understand usage. - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry - Never break backward compatibility (see https://symfony.com/bc). --> PHPStan was having issues correctly inferring the returned type of a configuration function. Consider the following messages as example: ``` Call to an undefined method Symfony\Config\Framework\AnnotationsConfig|Symfony\Config\FrameworkConfig::enabled() ``` This came from the following generated config class: ```php /** * `@template` TValue * `@param` TValue $value * annotation configuration * `@default` {"enabled":false,"cache":"php_array","file_cache_dir":"%kernel.cache_dir%\/annotations","debug":true} * `@return` \Symfony\Config\Framework\AnnotationsConfig|$this * `@psalm`-return (TValue is array ? \Symfony\Config\Framework\AnnotationsConfig : static) */ public function annotations(array $value = []): \Symfony\Config\Framework\AnnotationsConfig|static { if (!\is_array($value)) { $this->_usedProperties['annotations'] = true; $this->annotations = $value; return $this; } if (!$this->annotations instanceof \Symfony\Config\Framework\AnnotationsConfig) { $this->_usedProperties['annotations'] = true; $this->annotations = new \Symfony\Config\Framework\AnnotationsConfig($value); } elseif (0 < \func_num_args()) { throw new InvalidConfigurationException('The node created by "annotations()" has already been initialized. You cannot pass values the second time you call annotations().'); } return $this->annotations; } ``` When the determined parameter type is `array`, only that type can be passed meaning that the `is_array` is unnecessary. The same holds for the generated docblock: as only an array can be passed, there is no need to define a template and psalm returns. With the changes in this PR this method is generated more cleanly: ```php /** * annotation configuration * `@default` {"enabled":false,"cache":"php_array","file_cache_dir":"%kernel.cache_dir%\/annotations","debug":true} */ public function annotations(array $value = []): \Symfony\Config\Framework\AnnotationsConfig { if (null === $this->annotations) { $this->_usedProperties['annotations'] = true; $this->annotations = new \Symfony\Config\Framework\AnnotationsConfig($value); } elseif (0 < \func_num_args()) { throw new InvalidConfigurationException('The node created by "annotations()" has already been initialized. You cannot pass values the second time you call annotations().'); } return $this->annotations; } ``` A similar issue happens with functions that do accept more than an array value: ``` Call to an undefined method Symfony\Config\Doctrine\Dbal\TypeConfig|Symfony\Config\Doctrine\DbalConfig::class() ``` This is caused by the following generated method: ```php /** * `@template` TValue * `@param` TValue $value * `@return` \Symfony\Config\Doctrine\Dbal\TypeConfig|$this * `@psalm`-return (TValue is array ? \Symfony\Config\Doctrine\Dbal\TypeConfig : static) */ public function type(string $name, string|array $value = []): \Symfony\Config\Doctrine\Dbal\TypeConfig|static { if (!\is_array($value)) { $this->_usedProperties['types'] = true; $this->types[$name] = $value; return $this; } if (!isset($this->types[$name]) || !$this->types[$name] instanceof \Symfony\Config\Doctrine\Dbal\TypeConfig) { $this->_usedProperties['types'] = true; $this->types[$name] = new \Symfony\Config\Doctrine\Dbal\TypeConfig($value); } elseif (1 < \func_num_args()) { throw new InvalidConfigurationException('The node created by "type()" has already been initialized. You cannot pass values the second time you call type().'); } return $this->types[$name]; } ``` While the method seems fine, the ``@template`` definition is not correctly defined, see https://phpstan.org/r/09317897-4cc8-4f67-98ca-8b6da3671b31. With the changes in this PR the template is now strictly defined so it matches the function signature: ```php /** * `@template` TValue of string|array * `@param` TValue $value * `@return` \Symfony\Config\Doctrine\Dbal\TypeConfig|$this * `@psalm`-return (TValue is array ? \Symfony\Config\Doctrine\Dbal\TypeConfig : static) */ ``` See https://phpstan.org/r/986db325-9869-4a6f-8587-6af06c0612d4 for the results. While the second change might actually be enough to fix the errors, I prefer both fixes as it no longers generates code that can not be executed anyways. Commits ------- c79c6a2 [Config] Do not generate unreachable configuration paths
2 parents e092b46 + c79c6a2 commit 97429b7

File tree

10 files changed

+344
-18
lines changed

10 files changed

+344
-18
lines changed

‎src/Symfony/Component/Config/Builder/ConfigBuilderGenerator.php‎

Lines changed: 30 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -127,10 +127,13 @@ private function handleArrayNode(ArrayNode $node, ClassBuilder $class, string $n
127127
$class->addRequire($childClass);
128128
$this->classes[] = $childClass;
129129

130+
$nodeTypes = $this->getParameterTypes($node);
131+
$paramType = $this->getParamType($nodeTypes);
132+
130133
$hasNormalizationClosures = $this->hasNormalizationClosures($node);
131134
$comment = $this->getComment($node);
132-
if ($hasNormalizationClosures) {
133-
$comment = \sprintf(" * @template TValue\n * @param TValue \$value\n%s", $comment);
135+
if ($hasNormalizationClosures && 'array' !== $paramType) {
136+
$comment = \sprintf(" * @template TValue of %s\n * @param TValue \$value\n%s", $paramType, $comment);
134137
$comment .= \sprintf(' * @return %s|$this'."\n", $childClass->getFqcn());
135138
$comment .= \sprintf(' * @psalm-return (TValue is array ? %s : static)'."\n ", $childClass->getFqcn());
136139
}
@@ -142,8 +145,7 @@ private function handleArrayNode(ArrayNode $node, ClassBuilder $class, string $n
142145
$node->getName(),
143146
$this->getType($childClass->getFqcn(), $hasNormalizationClosures)
144147
);
145-
$nodeTypes = $this->getParameterTypes($node);
146-
$body = $hasNormalizationClosures ? '
148+
$body = $hasNormalizationClosures && 'array' !== $paramType ? '
147149
COMMENTpublic function NAME(PARAM_TYPE $value = []): CLASS|static
148150
{
149151
if (!\is_array($value)) {
@@ -178,7 +180,7 @@ private function handleArrayNode(ArrayNode $node, ClassBuilder $class, string $n
178180
'COMMENT' => $comment,
179181
'PROPERTY' => $property->getName(),
180182
'CLASS' => $childClass->getFqcn(),
181-
'PARAM_TYPE' => \in_array('mixed', $nodeTypes, true) ? 'mixed' : implode('|', $nodeTypes),
183+
'PARAM_TYPE' => $paramType,
182184
]);
183185

184186
$this->buildNode($node, $childClass, $this->getSubNamespace($childClass));
@@ -218,10 +220,11 @@ private function handlePrototypedArrayNode(PrototypedArrayNode $node, ClassBuild
218220

219221
$nodeParameterTypes = $this->getParameterTypes($node);
220222
$prototypeParameterTypes = $this->getParameterTypes($prototype);
223+
$noKey = null === $key = $node->getKeyAttribute();
221224
if (!$prototype instanceof ArrayNode || ($prototype instanceof PrototypedArrayNode && $prototype->getPrototype() instanceof ScalarNode)) {
222225
$class->addUse(ParamConfigurator::class);
223226
$property = $class->addProperty($node->getName());
224-
if (null === $key = $node->getKeyAttribute()) {
227+
if ($noKey) {
225228
// This is an array of values; don't use singular name
226229
$nodeTypesWithoutArray = array_filter($nodeParameterTypes, static fn ($type) => 'array' !== $type);
227230
$body = '
@@ -242,7 +245,7 @@ public function NAME(PARAM_TYPE $value): static
242245
'PROPERTY' => $property->getName(),
243246
'PROTOTYPE_TYPE' => implode('|', $prototypeParameterTypes),
244247
'EXTRA_TYPE' => $nodeTypesWithoutArray ? '|'.implode('|', $nodeTypesWithoutArray) : '',
245-
'PARAM_TYPE' => \in_array('mixed', $nodeParameterTypes, true) ? 'mixed' : 'ParamConfigurator|'.implode('|', $nodeParameterTypes),
248+
'PARAM_TYPE' => $this->getParamType($nodeParameterTypes, true),
246249
]);
247250
} else {
248251
$body = '
@@ -259,7 +262,7 @@ public function NAME(string $VAR, TYPE $VALUE): static
259262

260263
$class->addMethod($methodName, $body, [
261264
'PROPERTY' => $property->getName(),
262-
'TYPE' => \in_array('mixed', $prototypeParameterTypes, true) ? 'mixed' : 'ParamConfigurator|'.implode('|', $prototypeParameterTypes),
265+
'TYPE' => $this->getParamType($prototypeParameterTypes, true),
263266
'VAR' => '' === $key ? 'key' : $key,
264267
'VALUE' => 'value' === $key ? 'data' : 'value',
265268
]);
@@ -280,18 +283,27 @@ public function NAME(string $VAR, TYPE $VALUE): static
280283
$this->getType($childClass->getFqcn().'[]', $hasNormalizationClosures)
281284
);
282285

286+
$paramType = $this->getParamType($noKey ? $nodeParameterTypes : $prototypeParameterTypes);
287+
283288
$comment = $this->getComment($node);
289+
<<<<<<< HEAD
284290
if ($hasNormalizationClosures) {
285291
$comment = \sprintf(" * @template TValue\n * @param TValue \$value\n%s", $comment);
286292
$comment .= \sprintf(' * @return %s|$this'."\n", $childClass->getFqcn());
287293
$comment .= \sprintf(' * @psalm-return (TValue is array ? %s : static)'."\n ", $childClass->getFqcn());
294+
=======
295+
if ($hasNormalizationClosures && 'array' !== $paramType) {
296+
$comment = sprintf(" * @template TValue of %s\n * @param TValue \$value\n%s", $paramType, $comment);
297+
$comment .= sprintf(' * @return %s|$this'."\n", $childClass->getFqcn());
298+
$comment .= sprintf(' * @psalm-return (TValue is array ? %s : static)'."\n ", $childClass->getFqcn());
299+
>>>>>>> 100c683018d ([Config] Do not generate unreachable configuration paths)
288300
}
289301
if ('' !== $comment) {
290302
$comment = "/**\n$comment*/\n";
291303
}
292304

293-
if (null === $key = $node->getKeyAttribute()) {
294-
$body = $hasNormalizationClosures ? '
305+
if ($noKey) {
306+
$body = $hasNormalizationClosures && 'array' !== $paramType ? '
295307
COMMENTpublic function NAME(PARAM_TYPE $value = []): CLASS|static
296308
{
297309
$this->_usedProperties[\'PROPERTY\'] = true;
@@ -313,10 +325,10 @@ public function NAME(string $VAR, TYPE $VALUE): static
313325
'COMMENT' => $comment,
314326
'PROPERTY' => $property->getName(),
315327
'CLASS' => $childClass->getFqcn(),
316-
'PARAM_TYPE' => \in_array('mixed', $nodeParameterTypes, true) ? 'mixed' : implode('|', $nodeParameterTypes),
328+
'PARAM_TYPE' => $paramType,
317329
]);
318330
} else {
319-
$body = $hasNormalizationClosures ? '
331+
$body = $hasNormalizationClosures && 'array' !== $paramType ? '
320332
COMMENTpublic function NAME(string $VAR, PARAM_TYPE $VALUE = []): CLASS|static
321333
{
322334
if (!\is_array($VALUE)) {
@@ -352,7 +364,7 @@ public function NAME(string $VAR, TYPE $VALUE): static
352364
'CLASS' => $childClass->getFqcn(),
353365
'VAR' => '' === $key ? 'key' : $key,
354366
'VALUE' => 'value' === $key ? 'data' : 'value',
355-
'PARAM_TYPE' => \in_array('mixed', $prototypeParameterTypes, true) ? 'mixed' : implode('|', $prototypeParameterTypes),
367+
'PARAM_TYPE' => $paramType,
356368
]);
357369
}
358370

@@ -597,4 +609,9 @@ private function getType(string $classType, bool $hasNormalizationClosures): str
597609
{
598610
return $classType.($hasNormalizationClosures ? '|scalar' : '');
599611
}
612+
613+
private function getParamType(array $types, bool $withParamConfigurator = false): string
614+
{
615+
return \in_array('mixed', $types, true) ? 'mixed' : ($withParamConfigurator ? 'ParamConfigurator|' : '').implode('|', $types);
616+
}
600617
}

‎src/Symfony/Component/Config/Tests/Builder/Fixtures/ArrayValues.config.php‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
use Symfony\Config\ArrayValuesConfig;
13+
14+
return static function (ArrayValuesConfig $config) {
15+
$config->transports('foo')->dsn('bar');
16+
$config->transports('bar', ['dsn' => 'foobar']);
17+
18+
$config->errorPages()->withTrace(false);
19+
};

‎src/Symfony/Component/Config/Tests/Builder/Fixtures/ArrayValues.output.php‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
return [
13+
'transports' => [
14+
'foo' => [
15+
'dsn' => 'bar',
16+
],
17+
'bar' => [
18+
'dsn' => 'foobar',
19+
],
20+
],
21+
'error_pages' => [
22+
'with_trace' => false,
23+
]
24+
];

‎src/Symfony/Component/Config/Tests/Builder/Fixtures/ArrayValues.php‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
<?php
2+
3+
namespace Symfony\Component\Config\Tests\Builder\Fixtures;
4+
5+
use Symfony\Component\Config\Definition\Builder\TreeBuilder;
6+
use Symfony\Component\Config\Definition\ConfigurationInterface;
7+
8+
class ArrayValues implements ConfigurationInterface
9+
{
10+
public function getConfigTreeBuilder(): TreeBuilder
11+
{
12+
$tb = new TreeBuilder('array_values');
13+
$rootNode = $tb->getRootNode();
14+
$rootNode
15+
->children()
16+
->arrayNode('transports')
17+
->normalizeKeys(false)
18+
->useAttributeAsKey('name')
19+
->arrayPrototype()
20+
->beforeNormalization()
21+
->ifString()
22+
->then(function (string $dsn) {
23+
return ['dsn' => $dsn];
24+
})
25+
->end()
26+
->fixXmlConfig('option')
27+
->children()
28+
->scalarNode('dsn')->end()
29+
->end()
30+
->end()
31+
->end()
32+
->arrayNode('error_pages')
33+
->canBeEnabled()
34+
->children()
35+
->booleanNode('with_trace')->end()
36+
->end()
37+
->end()
38+
->end();
39+
40+
return $tb;
41+
}
42+
}

‎src/Symfony/Component/Config/Tests/Builder/Fixtures/ArrayValues/Symfony/Config/ArrayValues/ErrorPagesConfig.php‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
<?php
2+
3+
namespace Symfony\Config\ArrayValues;
4+
5+
use Symfony\Component\Config\Loader\ParamConfigurator;
6+
use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
7+
8+
/**
9+
* This class is automatically generated to help in creating a config.
10+
*/
11+
class ErrorPagesConfig
12+
{
13+
private $enabled;
14+
private $withTrace;
15+
private $_usedProperties = [];
16+
17+
/**
18+
* @default false
19+
* @param ParamConfigurator|bool $value
20+
* @return $this
21+
*/
22+
public function enabled($value): static
23+
{
24+
$this->_usedProperties['enabled'] = true;
25+
$this->enabled = $value;
26+
27+
return $this;
28+
}
29+
30+
/**
31+
* @default null
32+
* @param ParamConfigurator|bool $value
33+
* @return $this
34+
*/
35+
public function withTrace($value): static
36+
{
37+
$this->_usedProperties['withTrace'] = true;
38+
$this->withTrace = $value;
39+
40+
return $this;
41+
}
42+
43+
public function __construct(array $value = [])
44+
{
45+
if (array_key_exists('enabled', $value)) {
46+
$this->_usedProperties['enabled'] = true;
47+
$this->enabled = $value['enabled'];
48+
unset($value['enabled']);
49+
}
50+
51+
if (array_key_exists('with_trace', $value)) {
52+
$this->_usedProperties['withTrace'] = true;
53+
$this->withTrace = $value['with_trace'];
54+
unset($value['with_trace']);
55+
}
56+
57+
if ([] !== $value) {
58+
throw new InvalidConfigurationException(sprintf('The following keys are not supported by "%s": ', __CLASS__).implode(', ', array_keys($value)));
59+
}
60+
}
61+
62+
public function toArray(): array
63+
{
64+
$output = [];
65+
if (isset($this->_usedProperties['enabled'])) {
66+
$output['enabled'] = $this->enabled;
67+
}
68+
if (isset($this->_usedProperties['withTrace'])) {
69+
$output['with_trace'] = $this->withTrace;
70+
}
71+
72+
return $output;
73+
}
74+
75+
}

‎src/Symfony/Component/Config/Tests/Builder/Fixtures/ArrayValues/Symfony/Config/ArrayValues/TransportsConfig.php‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
<?php
2+
3+
namespace Symfony\Config\ArrayValues;
4+
5+
use Symfony\Component\Config\Loader\ParamConfigurator;
6+
use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
7+
8+
/**
9+
* This class is automatically generated to help in creating a config.
10+
*/
11+
class TransportsConfig
12+
{
13+
private $dsn;
14+
private $_usedProperties = [];
15+
16+
/**
17+
* @default null
18+
* @param ParamConfigurator|mixed $value
19+
* @return $this
20+
*/
21+
public function dsn($value): static
22+
{
23+
$this->_usedProperties['dsn'] = true;
24+
$this->dsn = $value;
25+
26+
return $this;
27+
}
28+
29+
public function __construct(array $value = [])
30+
{
31+
if (array_key_exists('dsn', $value)) {
32+
$this->_usedProperties['dsn'] = true;
33+
$this->dsn = $value['dsn'];
34+
unset($value['dsn']);
35+
}
36+
37+
if ([] !== $value) {
38+
throw new InvalidConfigurationException(sprintf('The following keys are not supported by "%s": ', __CLASS__).implode(', ', array_keys($value)));
39+
}
40+
}
41+
42+
public function toArray(): array
43+
{
44+
$output = [];
45+
if (isset($this->_usedProperties['dsn'])) {
46+
$output['dsn'] = $this->dsn;
47+
}
48+
49+
return $output;
50+
}
51+
52+
}

0 commit comments

Comments
 (0)