Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Form] Fixed empty data on expanded ChoiceType and FileType#25948
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
c136751 to7327a9fCompare7327a9f to9722c06Compare| if (false === FormUtil::isEmpty($emptyData) &&array() !==$emptyData) { | ||
| $data =is_callable($emptyData) ?call_user_func($emptyData,$form,$data) :$emptyData; | ||
| } | ||
| $data =$emptyDatainstanceof \Closure ?$emptyData($form,$data) :$emptyData; |
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.
isinstanceof \Closure legit instead ofis_callable (same below?)
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.
This is totally legit and what must be, this is the point of this bug fix. Only a\Closure should be considered here, nothing else.
The view data can be:
- an object (whose class must be the same as defined by the
data_classoption) - an array
- a scalar
The three of them can be callable:
- the object can be invokable
- the array can be a class or an object with a method name
- the string a function name (which triggered the related issue)
Deprecating a broken behavior on master does not make any sense to me,
I really think we need to merge this PR as a bug fix, because theprevious one was wrong.
nicolas-grekas commentedJan 31, 2018
HeahDude commentedJan 31, 2018
@nicolas-grekas yes thanks, I already did.#25925 is a duplicate of#25924 where I left a comment. |
| if (false === FormUtil::isEmpty($emptyData) &&array() !==$emptyData) { | ||
| $data =is_callable($emptyData) ?call_user_func($emptyData,$form,$data) :$emptyData; | ||
| } | ||
| $data =$emptyDatainstanceof \Closure ?$emptyData($form,$data) :$emptyData; |
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.
This is totally legit and what must be, this is the point of this bug fix. Only a\Closure should be considered here, nothing else.
The view data can be:
- an object (whose class must be the same as defined by the
data_classoption) - an array
- a scalar
The three of them can be callable:
- the object can be invokable
- the array can be a class or an object with a method name
- the string a function name (which triggered the related issue)
Deprecating a broken behavior on master does not make any sense to me,
I really think we need to merge this PR as a bug fix, because theprevious one was wrong.
| $emptyData =$form->getConfig()->getEmptyData(); | ||
| $data =is_callable($emptyData) ?call_user_func($emptyData,$form,$data) :$emptyData; | ||
| $data =$emptyDatainstanceof \Closure ?$emptyData($form,$data) :$emptyData; |
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.
Note that thisbug fix was wrong as well by replicating the first.
| $data =is_callable($emptyData) ?call_user_func($emptyData,$form,$data) :$emptyData; | ||
| $event->setData($data); | ||
| $event->setData(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.
And that this is a BC break introduced in#24993 which does not count as the one which adds a method to theRequestHandlerInterface introduced in that same PR, considering theintended behavior.
fabpot commentedFeb 1, 2018
Thank you@HeahDude. |
…e (HeahDude)This PR was merged into the 2.7 branch.Discussion----------[Form] Fixed empty data on expanded ChoiceType and FileType| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#25896| License | MIT| Doc PR | ~Alternative of#25924.I don't think we have to do this by adding overhead in master and getting it properly fixed in 2 years.This is bug I've introduced while fixing another bug#17822. Which then has been replicated in#20418 and#24993.I propose instead to clean up the code for all LTS, since this is a wrong behaviour that has never been documented, and most of all unreliable.The `empty_data` can by anything in the view format as long as the reverse view transformation supports it. Even an object derived from the data class could be invokable.I think we should remain consistent withhttps://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Form/Form.php#L615.Commits-------9722c06 [Form] Fixed empty data on expanded ChoiceType and FileType
peter-gribanov commentedFeb 1, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I'm in shock 😳 |
HeahDude commentedFeb 1, 2018
@peter-gribanov Sorry for the confusion, and thank you very much for reporting the issue and trying to fix it. |
Alternative of#25924.
I don't think we have to do this by adding overhead in master and getting it properly fixed in 2 years.
This is bug I've introduced while fixing another bug#17822. Which then has been replicated in#20418 and#24993.
I propose instead to clean up the code for all LTS, since this is a wrong behaviour that has never been documented, and most of all unreliable.
The
empty_datacan by anything in the view format as long as the reverse view transformation supports it. Even an object derived from the data class could be invokable.I think we should remain consistent withhttps://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Form/Form.php#L615.