[Form] Deprecate a callable empty_data in ChoiceType #25924
Conversation
UPGRADE-5.0.md
Outdated
| ```php | ||
| 'empty_data' => function (FormInterface $form, $data) { | ||
| return SomeValueObject::getDefaultValue(); | ||
| }, |
There was a problem hiding this comment.
I think, you should rather use a global function call as an example. A static method call can still be written as ['SomeValueObject', 'getDefaultValue'] and I personally would prefer that notation over an anonymous function.
There was a problem hiding this comment.
And in vain you prefer such a call to static functions.
This leads to the fact that the connection to the real function is lost and this is guaranteed to lead to problems during refactoring.
There was a problem hiding this comment.
Well, it was not my intention to discuss my personal preferences. Shouldn't have mentioned it. Still, the static call adds unnecessary distraction to the example, imho.
UPGRADE-5.0.md
Outdated
| Form | ||
| ---- | ||
|
|
||
| * Using callable strings as `empty_data` in `ChoiceType` has been deprecated in Symfony 4.1 use a `\Closure` instead. |
There was a problem hiding this comment.
Should be:
* Support for callable strings as `empty_data` in `ChoiceType` has been removed. Use a `\Closure` instead.
UPGRADE-4.1.md
Outdated
| Form | ||
| ---- | ||
|
|
||
| * Using callable strings as `empty_data` in `ChoiceType` has been deprecated in Symfony 4.1 use a `\Closure` instead. |
There was a problem hiding this comment.
Better: … is deprecated and will be removed in Symfony 5.0, use a…
| 4.1.0 | ||
| ----- | ||
|
|
||
| * Using callable strings as `empty_data` in `ChoiceType` has been deprecated in Symfony 4.1 use a `\Closure` instead. |
There was a problem hiding this comment.
The version information is redundant. To match the message style of other entries in this file, I'd suggest:
* deprecated support for callable strings as `empty_data` in `ChoiceType`
|
Maybe we should add |
| @@ -91,7 +91,14 @@ public function buildForm(FormBuilderInterface $builder, array $options) | |||
| $emptyData = $form->getConfig()->getEmptyData(); | |||
|
|
|||
| if (false === FormUtil::isEmpty($emptyData) && array() !== $emptyData) { | |||
| $data = is_callable($emptyData) ? call_user_func($emptyData, $form, $data) : $emptyData; | |||
| if (is_callable($emptyData)) { | |||
| if (is_string($emptyData)) { | |||
There was a problem hiding this comment.
I've introduced that change as a bug fix in #17822 targeting 2.7.
But I've made a mistake, IMO we should do it like https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Form.php#L610 and merge it in 2.7 as bug fix too. No need to deprecate anything.
There was a problem hiding this comment.
#17822 is over 1.5y old now. People might have built code against that "feature" already. :-(
There was a problem hiding this comment.
Also, I would not check for instanceof \Closure as this would forbid array callables and invokable objects for no reason.
There was a problem hiding this comment.
The empty_data MUST be a closure for all forms, I'm not sure people rely on this hidden feature which is just a bug I introduced no matter when IMO.
@derrabus sorry, but i would prefer to see this in another PR. |
| After: | ||
|
|
||
| ```php | ||
| 'empty_data' => function (FormInterface $form, $data) { |
There was a problem hiding this comment.
Or just Closure::fromCallable('some_function') 😉
|
Closing in favor of #25948 |
…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
…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 symfony/symfony#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 ------- 9722c063fb [Form] Fixed empty data on expanded ChoiceType and FileType
This PR is related to bug #25896, but doesn't solve it.