Skip to content

[Config] Fix generating PHP config for keyed list of scalars #60316

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: 6.4
Choose a base branch
from
Open

[Config] Fix generating PHP config for keyed list of scalars #60316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/Symfony/Component/Config/Builder/ConfigBuilderGenerator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,15 +284,16 @@ public function NAME(string $VAR, TYPE $VALUE): static
if ($hasNormalizationClosures) {
$comment = sprintf(" * @template TValue\n * @param TValue \$value\n%s", $comment);
$comment .= sprintf(' * @return %s|$this'."\n", $childClass->getFqcn());
$comment .= sprintf(' * @psalm-return (TValue is array ? %s : static)'."\n ", $childClass->getFqcn());
$comment .= sprintf(' * @psalm-return (TValue is array ? %s : static)'."\n", $childClass->getFqcn());
$comment .= sprintf(' * @phpstan-return ($value is array ? %s : $this)'."\n ", $childClass->getFqcn());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improvements like this don't qualify as bug fixes, so this should be a separate PR for 7.3 (or more likely 7.4, since 7.3 is now in feature freeze).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was searching for for some guide lines on what qualifies as bug, but I'm not sure, this seem to me like a bug, currently it's not possible to use some php configs with strict PHPStan, that said, I'm going to change the target to 7.3 or 7.4 (when branch is available)
Outside of target branch, do you think this is acceptable fix/improvement? Or do I need to do it in any other way?

if ('' !== $comment) {
$comment = "/**\n$comment*/\n";
}

if (null === $key = $node->getKeyAttribute()) {
$body = $hasNormalizationClosures ? '
COMMENTpublic function NAME(PARAM_TYPE $value = []): CLASS|static
COMMENTpublic function NAME(PARAM_TYPE $value = []): CLASS|self
{
$this->_usedProperties[\'PROPERTY\'] = true;
if (!\is_array($value)) {
Expand All @@ -317,7 +318,7 @@ public function NAME(string $VAR, TYPE $VALUE): static
]);
} else {
$body = $hasNormalizationClosures ? '
COMMENTpublic function NAME(string $VAR, PARAM_TYPE $VALUE = []): CLASS|static
COMMENTpublic function NAME(string $VAR, PARAM_TYPE $VALUE = []): CLASS|self
{
if (!\is_array($VALUE)) {
$this->_usedProperties[\'PROPERTY\'] = true;
Expand Down Expand Up @@ -352,7 +353,7 @@ public function NAME(string $VAR, TYPE $VALUE): static
'CLASS' => $childClass->getFqcn(),
'VAR' => '' === $key ? 'key' : $key,
'VALUE' => 'value' === $key ? 'data' : 'value',
'PARAM_TYPE' => \in_array('mixed', $prototypeParameterTypes, true) ? 'mixed' : implode('|', $prototypeParameterTypes),
'PARAM_TYPE' => \in_array('mixed', $prototypeParameterTypes, true) ? 'mixed' : implode('|', [...$prototypeParameterTypes, $childClass->getFqcn()]),
]);
}

Expand Down
7 changes: 7 additions & 0 deletions src/Symfony/Component/Config/Tests/Builder/Fixtures/ScalarNormalizedTypes.config.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
* file that was distributed with this source code.
*/

use Symfony\Config\ScalarNormalizedTypes\KeyedListScalarConfig;
use Symfony\Config\ScalarNormalizedTypesConfig;

return static function (ScalarNormalizedTypesConfig $config) {
Expand All @@ -28,4 +29,10 @@
'nested_object' => true,
'nested_list_object' => ['one', 'two'],
]);

$config->keyedListScalar('Foo\\Bar')->list(['one', 'two']);
$config->keyedListScalar('Foo\\Baz', ['list' => ['one', 'two']]);
$keyedListScalarConfig = new KeyedListScalarConfig();
$keyedListScalarConfig->list(['one', 'two']);
$config->keyedListScalar('Foo\\Foo', $keyedListScalarConfig);
Comment on lines +35 to +37
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't really how configuration classes are meant to be used. They should never be manually instantiated.

The main issue I see is that in some cases, $value can be multiple types, for example, MessengerConfig::transport:

public function transport(string $name, string|array $value = []): \Symfony\Config\Framework\Messenger\TransportConfig|static
{
    // ...
}

In other cases, it can only be an array. The proper fix would be to avoid including the if (!\is_array($value)) { check when the type is strictly an array.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok got it, maybe then __construct of generated classes should be marked as @internal right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, not sure about that one 🤔

};
11 changes: 11 additions & 0 deletions src/Symfony/Component/Config/Tests/Builder/Fixtures/ScalarNormalizedTypes.output.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,15 @@
'nested_object' => true,
'nested_list_object' => ['one', 'two'],
],
'keyed_list_scalar' => [
'Foo\\Bar' => [
'list' => ['one', 'two'],
],
'Foo\\Baz' => [
'list' => ['one', 'two'],
],
'Foo\\Foo' => [
'list' => ['one', 'two'],
],
],
];
17 changes: 17 additions & 0 deletions src/Symfony/Component/Config/Tests/Builder/Fixtures/ScalarNormalizedTypes.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,23 @@ public function getConfigTreeBuilder(): TreeBuilder
->end()
->end()
->end()
->arrayNode('keyed_list_scalar')
->normalizeKeys(false)
->useAttributeAsKey('class')
->beforeNormalization()
->always()
->then(function ($config) {
return $config;
})
->end()
->prototype('array')
->children()
->arrayNode('list')
->prototype('scalar')->end()
->end()
->end()
->end()
->end()
->end()
;

Expand Down
52 changes: 52 additions & 0 deletions ...ures/ScalarNormalizedTypes/Symfony/Config/ScalarNormalizedTypes/KeyedListScalarConfig.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<?php

namespace Symfony\Config\ScalarNormalizedTypes;

use Symfony\Component\Config\Loader\ParamConfigurator;
use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;

/**
* This class is automatically generated to help in creating a config.
*/
class KeyedListScalarConfig
{
private $list;
private $_usedProperties = [];

/**
* @param ParamConfigurator|list<ParamConfigurator|mixed> $value
*
* @return $this
*/
public function list(ParamConfigurator|array $value): static
{
$this->_usedProperties['list'] = true;
$this->list = $value;

return $this;
}

public function __construct(array $value = [])
{
if (array_key_exists('list', $value)) {
$this->_usedProperties['list'] = true;
$this->list = $value['list'];
unset($value['list']);
}

if ([] !== $value) {
throw new InvalidConfigurationException(sprintf('The following keys are not supported by "%s": ', __CLASS__).implode(', ', array_keys($value)));
}
}

public function toArray(): array
{
$output = [];
if (isset($this->_usedProperties['list'])) {
$output['list'] = $this->list;
}

return $output;
}

}
3 changes: 2 additions & 1 deletion ...lder/Fixtures/ScalarNormalizedTypes/Symfony/Config/ScalarNormalizedTypes/NestedConfig.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ public function nestedObject(mixed $value = []): \Symfony\Config\ScalarNormalize
* @param TValue $value
* @return \Symfony\Config\ScalarNormalizedTypes\Nested\NestedListObjectConfig|$this
* @psalm-return (TValue is array ? \Symfony\Config\ScalarNormalizedTypes\Nested\NestedListObjectConfig : static)
* @phpstan-return ($value is array ? \Symfony\Config\ScalarNormalizedTypes\Nested\NestedListObjectConfig : $this)
*/
public function nestedListObject(mixed $value = []): \Symfony\Config\ScalarNormalizedTypes\Nested\NestedListObjectConfig|static
public function nestedListObject(mixed $value = []): \Symfony\Config\ScalarNormalizedTypes\Nested\NestedListObjectConfig|self
{
$this->_usedProperties['nestedListObject'] = true;
if (!\is_array($value)) {
Expand Down
43 changes: 41 additions & 2 deletions ...sts/Builder/Fixtures/ScalarNormalizedTypes/Symfony/Config/ScalarNormalizedTypesConfig.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
require_once __DIR__.\DIRECTORY_SEPARATOR.'ScalarNormalizedTypes'.\DIRECTORY_SEPARATOR.'ListObjectConfig.php';
require_once __DIR__.\DIRECTORY_SEPARATOR.'ScalarNormalizedTypes'.\DIRECTORY_SEPARATOR.'KeyedListObjectConfig.php';
require_once __DIR__.\DIRECTORY_SEPARATOR.'ScalarNormalizedTypes'.\DIRECTORY_SEPARATOR.'NestedConfig.php';
require_once __DIR__.\DIRECTORY_SEPARATOR.'ScalarNormalizedTypes'.\DIRECTORY_SEPARATOR.'KeyedListScalarConfig.php';

use Symfony\Component\Config\Loader\ParamConfigurator;
use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
Expand All @@ -21,6 +22,7 @@ class ScalarNormalizedTypesConfig implements \Symfony\Component\Config\Builder\C
private $listObject;
private $keyedListObject;
private $nested;
private $keyedListScalar;
private $_usedProperties = [];

/**
Expand Down Expand Up @@ -78,8 +80,9 @@ public function object(mixed $value = []): \Symfony\Config\ScalarNormalizedTypes
* @param TValue $value
* @return \Symfony\Config\ScalarNormalizedTypes\ListObjectConfig|$this
* @psalm-return (TValue is array ? \Symfony\Config\ScalarNormalizedTypes\ListObjectConfig : static)
* @phpstan-return ($value is array ? \Symfony\Config\ScalarNormalizedTypes\ListObjectConfig : $this)
*/
public function listObject(mixed $value = []): \Symfony\Config\ScalarNormalizedTypes\ListObjectConfig|static
public function listObject(mixed $value = []): \Symfony\Config\ScalarNormalizedTypes\ListObjectConfig|self
{
$this->_usedProperties['listObject'] = true;
if (!\is_array($value)) {
Expand All @@ -96,8 +99,9 @@ public function listObject(mixed $value = []): \Symfony\Config\ScalarNormalizedT
* @param TValue $value
* @return \Symfony\Config\ScalarNormalizedTypes\KeyedListObjectConfig|$this
* @psalm-return (TValue is array ? \Symfony\Config\ScalarNormalizedTypes\KeyedListObjectConfig : static)
* @phpstan-return ($value is array ? \Symfony\Config\ScalarNormalizedTypes\KeyedListObjectConfig : $this)
*/
public function keyedListObject(string $class, mixed $value = []): \Symfony\Config\ScalarNormalizedTypes\KeyedListObjectConfig|static
public function keyedListObject(string $class, mixed $value = []): \Symfony\Config\ScalarNormalizedTypes\KeyedListObjectConfig|self
{
if (!\is_array($value)) {
$this->_usedProperties['keyedListObject'] = true;
Expand Down Expand Up @@ -128,6 +132,32 @@ public function nested(array $value = []): \Symfony\Config\ScalarNormalizedTypes
return $this->nested;
}

/**
* @template TValue
* @param TValue $value
* @return \Symfony\Config\ScalarNormalizedTypes\KeyedListScalarConfig|$this
* @psalm-return (TValue is array ? \Symfony\Config\ScalarNormalizedTypes\KeyedListScalarConfig : static)
* @phpstan-return ($value is array ? \Symfony\Config\ScalarNormalizedTypes\KeyedListScalarConfig : $this)
*/
public function keyedListScalar(string $class, array|\Symfony\Config\ScalarNormalizedTypes\KeyedListScalarConfig $value = []): \Symfony\Config\ScalarNormalizedTypes\KeyedListScalarConfig|self
{
if (!\is_array($value)) {
$this->_usedProperties['keyedListScalar'] = true;
$this->keyedListScalar[$class] = $value;

return $this;
}

if (!isset($this->keyedListScalar[$class]) || !$this->keyedListScalar[$class] instanceof \Symfony\Config\ScalarNormalizedTypes\KeyedListScalarConfig) {
$this->_usedProperties['keyedListScalar'] = true;
$this->keyedListScalar[$class] = new \Symfony\Config\ScalarNormalizedTypes\KeyedListScalarConfig($value);
} elseif (1 < \func_num_args()) {
throw new InvalidConfigurationException('The node created by "keyedListScalar()" has already been initialized. You cannot pass values the second time you call keyedListScalar().');
}

return $this->keyedListScalar[$class];
}

public function getExtensionAlias(): string
{
return 'scalar_normalized_types';
Expand Down Expand Up @@ -171,6 +201,12 @@ public function __construct(array $value = [])
unset($value['nested']);
}

if (array_key_exists('keyed_list_scalar', $value)) {
$this->_usedProperties['keyedListScalar'] = true;
$this->keyedListScalar = array_map(fn ($v) => \is_array($v) ? new \Symfony\Config\ScalarNormalizedTypes\KeyedListScalarConfig($v) : $v, $value['keyed_list_scalar']);
unset($value['keyed_list_scalar']);
}

if ([] !== $value) {
throw new InvalidConfigurationException(sprintf('The following keys are not supported by "%s": ', __CLASS__).implode(', ', array_keys($value)));
}
Expand All @@ -197,6 +233,9 @@ public function toArray(): array
if (isset($this->_usedProperties['nested'])) {
$output['nested'] = $this->nested->toArray();
}
if (isset($this->_usedProperties['keyedListScalar'])) {
$output['keyed_list_scalar'] = array_map(fn ($v) => $v instanceof \Symfony\Config\ScalarNormalizedTypes\KeyedListScalarConfig ? $v->toArray() : $v, $this->keyedListScalar);
}

return $output;
}
Expand Down
Loading