Skip to content
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] Add "is empty callback" to form config #32747

Open
wants to merge 1 commit into
base: master
from

Conversation

@fancyweb
Copy link
Contributor

fancyweb commented Jul 25, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #31572 for 4.4+
License MIT
Doc PR -

This PR introduces a new feature that allow to resolve a bug.

Currently, the isEmpty() behavior of the Form class is the same whatever its configuration. That prevents us to specify a different behavior by form type.

But I think that some form types should have dedicated empty values. For example, the CheckboxType model data either resolves to true (checked) or false (unchecked). But false is not an empty value in the Form::isEmpty() method, so a CheckboxType form can never be empty. false should not be in that list because for other form types, it's perfectly fine that it's not considered as an empty value.

The problem is better seen in #31572 with a ChoiceType that is never considered as empty (when no radio button is checked).

Being able to specify the "is empty" behavior by form type would also allow users to define their own logic in their custom form types + probably define it ourselves in all our form types in order to get rid of the default common behavior.

Copy link
Member

xabbuh left a comment

We also need to trigger some deprecations if implementations of the FormConfigInterface and FormConfigBuilderInterface do not implement the new methods. And those changes must be documented in the CHANGELOG.md file of the Form component as well as in the UPGRADE-4.4.md and UPGRADE-5.0.md files.

src/Symfony/Component/Form/FormConfigBuilder.php Outdated Show resolved Hide resolved
src/Symfony/Component/Form/FormConfigBuilder.php Outdated Show resolved Hide resolved
src/Symfony/Component/Form/FormConfigBuilder.php Outdated Show resolved Hide resolved
src/Symfony/Component/Form/Form.php Outdated Show resolved Hide resolved
src/Symfony/Component/Form/FormConfigInterface.php Outdated Show resolved Hide resolved
@fancyweb

This comment has been minimized.

Copy link
Contributor Author

fancyweb commented Jul 26, 2019

We also need to trigger some deprecations if implementations of the FormConfigInterface and FormConfigBuilderInterface do not implement the new methods. And those changes must be documented in the CHANGELOG.md file of the Form component as well as in the UPGRADE-4.4.md and UPGRADE-5.0.md files.

Nice point, let's throw a deprecation in the Form::isEmpty() method if the new config method does not exist?

Is the BC break on CheckboxType considered empty now for false value ok btw?

@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Jul 26, 2019

Is the BC break on CheckboxType considered empty now for false value ok btw?

I was thinking about that as well. Right now I couldn't come up with anything that used to work before and would now break. And our test suite seems to confirm this. But we should try hard to think about potential use cases that could break with this change.

@fancyweb fancyweb force-pushed the fancyweb:is-empty-callback branch from 5c69323 to 601a517 Jul 26, 2019
@yceruto

This comment has been minimized.

Copy link
Member

yceruto commented Jul 29, 2019

I wonder if it might be useful to add a dedicated option to set it up, maybe an is_empty option (callable). The advantage would be to be able to change this behavior from the outside. Make sense for you?

@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Jul 29, 2019

I was actually rather thinking about whether we should mark the new method as internal instead as I am not really sure that this is an extension point we should expose that much. I think usage of this in userland is quite limited and it increases risk by forms not working correctly if not used correctly.

@fancyweb

This comment has been minimized.

Copy link
Contributor Author

fancyweb commented Jul 30, 2019

I wonder if it might be useful to add a dedicated option to set it up, maybe an is_empty option (callable). The advantage would be to be able to change this behavior from the outside. Make sense for you?

Using an option would mean that it could not exist (in custom form type not extending FormType or in a standalone component usage) so we would still need a default behavior in Form::isEmpty() which I'm trying to remove.

I was actually rather thinking about whether we should mark the new method as internal instead as I am not really sure that this is an extension point we should expose that much. I think usage of this in userland is quite limited and it increases risk by forms not working correctly if not used correctly.

I agree that userland usage is limited. However, I'm against making it internal because it adds a nice customisable behavior if well used.

@nicolas-grekas nicolas-grekas changed the title [Form] Is empty callback [Form] add "is empty callback" to form config Jul 31, 2019
UPGRADE-4.4.md Outdated Show resolved Hide resolved
src/Symfony/Component/Form/CHANGELOG.md Outdated Show resolved Hide resolved
src/Symfony/Component/Form/Form.php Outdated Show resolved Hide resolved
src/Symfony/Component/Form/Form.php Outdated Show resolved Hide resolved
src/Symfony/Component/Form/FormConfigBuilder.php Outdated Show resolved Hide resolved
@fancyweb fancyweb force-pushed the fancyweb:is-empty-callback branch from 601a517 to 98df4f2 Aug 1, 2019
nicolas-grekas added a commit that referenced this pull request Aug 1, 2019
… (fancyweb)

This PR was merged into the 4.3 branch.

Discussion
----------

Sync "not implementing the method" deprecations messages

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Suggested in #32747 (comment)

Useful for consistency and for future reference for similar messages.

Commits
-------

f6fae1c Sync "not implementing the method" deprecations messages
symfony-splitter pushed a commit to symfony/security that referenced this pull request Aug 1, 2019
… (fancyweb)

This PR was merged into the 4.3 branch.

Discussion
----------

Sync "not implementing the method" deprecations messages

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Suggested in symfony/symfony#32747 (comment)

Useful for consistency and for future reference for similar messages.

Commits
-------

f6fae1c361 Sync "not implementing the method" deprecations messages
@fancyweb fancyweb force-pushed the fancyweb:is-empty-callback branch from 25b989f to 7395bb3 Nov 23, 2019
@fancyweb fancyweb requested review from dunglas, lyrixx and sroze as code owners Nov 23, 2019
@fancyweb fancyweb changed the base branch from 4.4 to master Nov 23, 2019
@fancyweb fancyweb force-pushed the fancyweb:is-empty-callback branch 2 times, most recently from 19d8b9b to 57f05fb Nov 23, 2019
src/Symfony/Component/Form/Form.php Outdated Show resolved Hide resolved
UPGRADE-5.1.md Outdated Show resolved Hide resolved
src/Symfony/Component/Form/CHANGELOG.md Outdated Show resolved Hide resolved
src/Symfony/Component/Form/CHANGELOG.md Outdated Show resolved Hide resolved
src/Symfony/Component/Form/CHANGELOG.md Outdated Show resolved Hide resolved
UPGRADE-5.1.md Outdated Show resolved Hide resolved
@fancyweb fancyweb force-pushed the fancyweb:is-empty-callback branch 2 times, most recently from f05c8ef to f4d1c75 Nov 23, 2019
@fancyweb fancyweb force-pushed the fancyweb:is-empty-callback branch from f4d1c75 to 24e199d Nov 23, 2019
@fancyweb fancyweb force-pushed the fancyweb:is-empty-callback branch 2 times, most recently from 7c430bd to 899a2b9 Nov 23, 2019
@xabbuh
xabbuh approved these changes Nov 23, 2019
@fancyweb fancyweb force-pushed the fancyweb:is-empty-callback branch from 899a2b9 to 32cd54c Dec 10, 2019
Copy link
Member

nicolas-grekas left a comment

please rebase

@fancyweb fancyweb force-pushed the fancyweb:is-empty-callback branch from 32cd54c to cc73e52 Dec 26, 2019
@fancyweb fancyweb force-pushed the fancyweb:is-empty-callback branch from cc73e52 to 0226851 Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.