[Config] Fix generating PHP config for keyed list of scalars #60316
Conversation
| $keyedListScalarConfig = new KeyedListScalarConfig(); | ||
| $keyedListScalarConfig->list(['one', 'two']); | ||
| $config->keyedListScalar('Foo\\Foo', $keyedListScalarConfig); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok got it, maybe then __construct of generated classes should be marked as @internal right?
| @@ -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()); | |||
| } | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
Fixes generated PHP config for keyed objects with array of scalars. Also enhances conditional returns for PHPStan compatibility.