-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Form] backport type fixes #42013
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.
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
[Form] backport type fixes #42013
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,10 +84,8 @@ public function getDecoratedFactory() | |
/** | ||
* {@inheritdoc} | ||
* | ||
* @param callable|Cache\ChoiceValue|null $value The callable or static option for | ||
* generating the choice values | ||
* @param callable|Cache\ChoiceFilter|null $filter The callable or static option for | ||
* filtering the choices | ||
* @param mixed $value | ||
* @param mixed $filter | ||
*/ | ||
public function createListFromChoices(iterable $choices, $value = null/*, $filter = null*/) | ||
{ | ||
|
@@ -127,12 +125,8 @@ public function createListFromChoices(iterable $choices, $value = null/*, $filte | |
/** | ||
* {@inheritdoc} | ||
* | ||
* @param ChoiceLoaderInterface|Cache\ChoiceLoader $loader The loader or static loader to load | ||
* the choices lazily | ||
* @param callable|Cache\ChoiceValue|null $value The callable or static option for | ||
* generating the choice values | ||
* @param callable|Cache\ChoiceFilter|null $filter The callable or static option for | ||
* filtering the choices | ||
* @param mixed $value | ||
* @param mixed $filter | ||
*/ | ||
public function createListFromLoader(ChoiceLoaderInterface $loader, $value = null/*, $filter = null*/) | ||
{ | ||
|
@@ -174,11 +168,11 @@ public function createListFromLoader(ChoiceLoaderInterface $loader, $value = nul | |
/** | ||
* {@inheritdoc} | ||
* | ||
* @param array|callable|Cache\PreferredChoice|null $preferredChoices The preferred choices | ||
* @param callable|false|Cache\ChoiceLabel|null $label The option or static option generating the choice labels | ||
* @param callable|Cache\ChoiceFieldName|null $index The option or static option generating the view indices | ||
* @param callable|Cache\GroupBy|null $groupBy The option or static option generating the group names | ||
* @param array|callable|Cache\ChoiceAttr|null $attr The option or static option generating the HTML attributes | ||
* @param mixed $preferredChoices | ||
* @param mixed $label | ||
* @param mixed $index | ||
* @param mixed $groupBy | ||
* @param mixed $attr | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 A decorator cannot know/restrict the types it accepts here, because it doesn't know what it decorates exactly. So yes, I'm sure :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense |
||
*/ | ||
public function createView(ChoiceListInterface $list, $preferredChoices = null, $label = null, $index = null, $groupBy = null, $attr = null) | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.