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] Deprecate a callable empty_data in ChoiceType#25924
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
[Form] Deprecate a callable empty_data in ChoiceType#25924
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| ```php | ||
| 'empty_data' => function (FormInterface $form, $data) { | ||
| return SomeValueObject::getDefaultValue(); | ||
| }, |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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`derrabus commentedJan 25, 2018
Maybe we should add |
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 likehttps://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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
#17822 is over 1.5y old now. People might have built code against that "feature" already. :-(
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.
Also, I would not check forinstanceof \Closure as this would forbid array callables and invokable objects for no reason.
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.
Theempty_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.
peter-gribanov commentedJan 25, 2018
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Or justClosure::fromCallable('some_function') 😉
fabpot commentedFeb 1, 2018
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 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
…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 ofsymfony/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 withhttps://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
Uh oh!
There was an error while loading.Please reload this page.
This PR is related to bug#25896, but doesn't solve it.