[Form] backport type fixes #42013
[Form] backport type fixes #42013
Conversation
src/Symfony/Component/Form/ChoiceList/Factory/CachingFactoryDecorator.php
Outdated
Show resolved
Hide resolved
6d276b1 to
d41c245
Compare
|
Hey! I think @VincentLanglet has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
| * generating the choice values | ||
| * @param callable|Cache\ChoiceFilter|null $filter The callable or static option for | ||
| * filtering the choices | ||
| * @param callable|string|Cache\ChoiceValue|null $value The callable or static option for |
There was a problem hiding this comment.
This looks wrong to me. Where do we use arbitrary strings?
There was a problem hiding this comment.
strings are for property paths IIRC
There was a problem hiding this comment.
If that's the case, we should update the description accordingly IMO.
There was a problem hiding this comment.
That's because of PropertyAccessDecorator, which also accept PropertyPath btw.
This makes me think that:
PropertyAccessDecoratorshould maybe acceptPropertyPathinstead?CachingFactoryDecoratorshould acceptmixedhere, because it shouldn't know anything about what it decorates.
WDYT?
src/Symfony/Component/Form/ChoiceList/Factory/CachingFactoryDecorator.php
Outdated
Show resolved
Hide resolved
d41c245 to
75691c6
Compare
|
PR updated according to https://github.com/symfony/symfony/pull/42013/files#r666967532 |
| * generating the choice values | ||
| * @param callable|Cache\ChoiceFilter|null $filter The callable or static option for | ||
| * filtering the choices | ||
| * @param mixed $value |
There was a problem hiding this comment.
I don't think that this is really correct as we are forwarding the input and there is no guarantee that the decorated factory handles every input type as well.
There was a problem hiding this comment.
A decorator cannot provide this guarantee, but it cannot restrict the accepted types either.
The decorated will throw as expected if an incompatible type is provided.
That's the correct behavior IMHO.
| * @param mixed $label | ||
| * @param mixed $index | ||
| * @param mixed $groupBy | ||
| * @ 8000 param mixed $attr |
There was a problem hiding this comment.
In https://github.com/symfony/symfony/pull/41998/files#diff-cb1213e76aa5d215066d613f70f20e58f403200646f57a1d82b18983d86d9c3eR171-R178 you added more concrete types. Sure this should be mixed?
There was a problem hiding this comment.
In that commit, I widened the accepted types to make tests pass, without thinking more about it.
Now, using my brain a bit more, I realized that we had to add string because that's what PropertyAccessDecorator accepts. But since when is CachingFactoryDecorator coupled in anyway with PropertyAccessDecorator???
A decorator cannot know/restrict the types it accepts here, because it doesn't know what it decorates exactly.
So yes, I'm sure :)
|
Thank you @nicolas-grekas. |
Backported from #41998