Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Form] Add "is empty callback" to form config#32747

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

Merged
nicolas-grekas merged 1 commit intosymfony:masterfromfancyweb:is-empty-callback
Feb 5, 2020

Conversation

fancyweb
Copy link
Contributor

@fancywebfancyweb commentedJul 25, 2019•
edited by nicolas-grekas
Loading

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

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

Currently, theisEmpty() behavior of theForm 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, theCheckboxType model data either resolves totrue (checked) orfalse (unchecked). Butfalse is not an empty value in theForm::isEmpty() method, so aCheckboxType 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 aChoiceType 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

@xabbuhxabbuh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We also need to trigger some deprecations if implementations of theFormConfigInterface andFormConfigBuilderInterface do not implement the new methods. And those changes must be documented in theCHANGELOG.md file of the Form component as well as in theUPGRADE-4.4.md andUPGRADE-5.0.md files.

fancyweb reacted with thumbs up emoji
@fancyweb
Copy link
ContributorAuthor

fancyweb commentedJul 26, 2019•
edited
Loading

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 theForm::isEmpty() method if the new config method does not exist?

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

@xabbuh
Copy link
Member

Is the BC break onCheckboxType 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.

@yceruto
Copy link
Member

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

@xabbuh
Copy link
Member

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
Copy link
ContributorAuthor

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 extendingFormType or in a standalone component usage) so we would still need a default behavior inForm::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-grekasnicolas-grekas changed the title[Form] Is empty callback[Form] add "is empty callback" to form configJul 31, 2019
nicolas-grekas added a commit that referenced this pull requestAug 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 requestAug 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 insymfony/symfony#32747 (comment)Useful for consistency and for future reference for similar messages.Commits-------f6fae1c361 Sync "not implementing the method" deprecations messages
@fancywebfancywebforce-pushed theis-empty-callback branch 2 times, most recently from7c430bd to899a2b9CompareNovember 23, 2019 15:14
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

please rebase

@nicolas-grekas
Copy link
Member

Comments discussed with@fancyweb on Slack, we can ignore them, PR is good to go.

@nicolas-grekas
Copy link
Member

Thank you@fancyweb.

nicolas-grekas added a commit that referenced this pull requestFeb 5, 2020
This PR was merged into the 5.1-dev branch.Discussion----------[Form] Add "is empty callback" to form config| 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.Commits-------7bfc27e [Form] Add "is empty callback" to form config
@nicolas-grekasnicolas-grekas merged commit7bfc27e intosymfony:masterFeb 5, 2020
@fancywebfancyweb deleted the is-empty-callback branchFebruary 5, 2020 16:33
nicolas-grekas added a commit that referenced this pull requestMar 12, 2020
…ncyweb)This PR was merged into the 3.4 branch.Discussion----------[Form] Handle false as empty value on expanded choices| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       |#31572| License       | MIT| Doc PR        | -This is the 3.4 version of#32747. The tests are the same. The added code has to be removed from master (if accepted).Commits-------1a366bc [Form] Handle false as empty value on expanded choices
@nicolas-grekasnicolas-grekas modified the milestones:next,5.1May 4, 2020
@fabpotfabpot mentioned this pull requestMay 5, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ycerutoycerutoyceruto left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@xabbuhxabbuhxabbuh approved these changes

@HeahDudeHeahDudeHeahDude approved these changes

@dunglasdunglasAwaiting requested review from dunglas

@lyrixxlyrixxAwaiting requested review from lyrixx

@srozesrozeAwaiting requested review from sroze

Assignees
No one assigned
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

6 participants
@fancyweb@xabbuh@yceruto@nicolas-grekas@HeahDude@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp