[Config] Generated php configs incorrectly using list param types #61533
Conversation
|
Being a list or a map is not indicated by the The confusion is coming from the fact that when no merging is needed (because a single configuration provides this setting), the configured array is returned as is without processing it (and so keys are preserved even if they are not meant to be. So -1 for this PR. |
I understand that the While looking at it normalizeKeys atleast felt like a somewhat more clear indication it should most likely not be a list but a map instead. (Allthough i do admit this might be a flawed assumption) Would an option be to possibly implement a new option for the ArrayNode along the lines of a "preserveKeys" you can set. Where if true, we default to the array instead of list? |
|
IIRC there was an idea in the past to introduce new |
|
@KevinVanSonsbeek regarding
|
Ooh, it seems i completely misunderstood the implementation in that case. I'll have a look later when i have some more time, and turn this PR into a draft for the meanwhile 👍🏼 |
18e89bc to
ad7af98
Compare
| @@ -506,6 +506,7 @@ private function addWorkflowSection(ArrayNodeDefinition $rootNode): void | |||
| ->end() | |||
| ->arrayNode('metadata') | |||
| ->normalizeKeys(false) | |||
| ->useAttributeAsKey('key') | |||
There was a problem hiding this comment.
@stof Adding the useAttributeAsKey would allow us to set specific metadata by key when using the config builder.
But we'd lose out on the ability to set all metadata in 1 call if we already created an instance of the parent node builder.
This is due to the generated config changing from
/**
* @param ParamConfigurator|list<ParamConfigurator|mixed> $value
*
* @return $this
*/
public function metadata(ParamConfigurator|array $value): static
{
$this->_usedProperties['metadata'] = true;
$this->metadata = $value;
return $this;
}To
/**
* @return $this
*/
public function metadata(string $key, mixed $value): static
{
$this->_usedProperties['metadata'] = true;
$this->metadata[$key] = $value;
return $this;
}Would a breaking change like this be a problem for a bugfix? (cc @nicolas-grekas )
As a sidenote, a update to the workflow docs would also be required in the case we would want to perform these changes.
As it shows some examples for the php configs setting the metadata as an array. And we would lose this option.
$pullRequest->place()
->name('review')
->metadata(['description' => 'Human review']);
$pullRequest->place()->name('merged');
$pullRequest->place()
->name('closed')
->metadata(['bg_color' => 'DeepSkyBlue',]);The XML config in general seems to be broken when parsing though. The example showed on the doc page shows nodes, that should actually be attributes.
But having a combination of place nodes, with and without metadata seems to actually break when using XML config, due to the callbacks used in the beforeNormalization possibly not accounting properly for the differences in data structure of the passed $places var, depending on the type of config used :(
There was a problem hiding this comment.
Small example of the data contained in the $places argument of the normalization callback, when using the example configs from the docs:
PHP
^ array:6 [
0 => array:1 [
"name" => "start"
]
1 => array:1 [
"name" => "coding"
]
2 => array:1 [
"name" => "test"
]
3 => array:2 [
"name" => "review"
"metadata" => array:1 [
"description" => "Human review"
]
]
4 => array:1 [
"name" => "merged"
]
5 => array:2 [
"name" => "closed"
"metadata" => array:1 [
"bg_color" => "DeepSkyBlue"
]
]
]
^ array:6 [
YAML
^ array:6 [
"start" => null
"coding" => null
"test" => null
"review" => array:1 [
"metadata" => array:1 [
"description" => "Human review"
]
]
"merged" => null
"closed" => array:1 [
"metadata" => array:1 [
"bg_color" => "DeepSkyBlue"
]
]
]
# XML
^ array:6 [
0 => "start"
1 => "coding"
2 => "test"
3 => array:2 [
"name" => "review"
"metadata" => array:2 [
"description" => "Human review"
"other_value" => "Some other value"
]
]
4 => "merged"
5 => array:2 [
"name" => "closed"
"metadata" => array:1 [
"bg_color" => "DeepSkyBlue"
]
]
]
As mentioned in #61136 the auto generated php configs assumes ArrayNode's are by default lists.
The option to purposefully not normalize keys however indicates this will not always be the case.
The workflow component docs for example show defining metadata with a key value array
But actually using said implementation will result in phpstan reporting the following error:
(This is present on a few more configs than just the Workflow components configs)