[Form] Fixed empty data on expanded ChoiceType and FileType #25948
Conversation
HeahDude
changed the title
HeahDude
force-pushed
the
fix/choice_type-empty_data_closure
branch
from
c136751
to
7327a9f
Compare
HeahDude
force-pushed
the
fix/choice_type-empty_data_closure
branch
from
7327a9f
to
9722c06
Compare
| if (false === FormUtil::isEmpty($emptyData) && array() !== $emptyData) { | ||
| $data = is_callable($emptyData) ? call_user_func($emptyData, $form, $data) : $emptyData; | ||
| } | ||
| $data = $emptyData instanceof \Closure ? $emptyData($form, $data) : $emptyData; |
There was a problem hiding this comment.
is instanceof \Closure legit instead of is_callable (same below?)
There was a problem hiding this comment.
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 the previous one was wrong.
|
@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 = $emptyData instanceof \Closure ? $emptyData($form, $data) : $emptyData; |
There was a problem hiding this comment.
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 the previous one was wrong.
| $emptyData = $form->getConfig()->getEmptyData(); | ||
|
|
||
| $data = is_callable($emptyData) ? call_user_func($emptyData, $form, $data) : $emptyData; | ||
| $data = $emptyData instanceof \Closure ? $emptyData($form, $data) : $emptyData; |
There was a problem hiding this comment.
Note that this bug 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.
And that this is a BC break introduced in #24993 which does not count as the one which adds a method to the RequestHandlerInterface introduced in that same PR, considering the intended behavior.
|
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 with https://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
|
I'm in shock 😳 |
|
@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 with https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Form/Form.php#L615.