Skip to content

[Form] dispatch submit events for disabled forms too #33381

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 1, 2020
Merged

[Form] dispatch submit events for disabled forms too #33381

merged 1 commit into from
Sep 1, 2020

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Aug 29, 2019 •
edited
Loading

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #27217
License MIT
Doc PR

TODO:

  • add a test case covering the validation use case

@xabbuh xabbuh added this to the 3.4 milestone Aug 29, 2019
@xabbuh xabbuh mentioned this pull request Aug 29, 2019
Open
@fancyweb
Copy link
Contributor

Firstly, isn't it actually a new feature? Dispatching those events here is a new behavior. Some listeners will be executed while they were not until now.

Secondly, the events are chained dispatched but they don't use / encapsulate the (current) expected data. For example, the view data encapsulated in the FormEvents::POST_SUBMIT form event object is not the view data resynchronized from the real (potentially modified by the FormEvents::SUBMIT event) norm data. I understand why it can't be done but I definitely think that this new behavior can cause trouble since listeners are going to be called with unexpected data. IMHO, it doesn't worth the risk.

xabbuh
xabbuh commented Aug 30, 2019
View reviewed changes
HeahDude
HeahDude reviewed Nov 23, 2019
View reviewed changes
Copy link
Contributor

@HeahDude HeahDude left a comment

Choose a reason for hiding this comment

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

Left a small comment, otherwise LGTM

@nicolas-grekas
Copy link
Member

Up.
for master don't you think?

@xabbuh xabbuh changed the base branch from 3.4 to master March 28, 2020 08:31
@xabbuh xabbuh modified the milestones: 3.4, next Mar 28, 2020
@xabbuh xabbuh modified the milestones: next, 5.1 May 5, 2020
@xabbuh xabbuh force-pushed the issue-27217 branch 2 times, most recently from 69520cb to f781150 Compare May 5, 2020 07:49
@xabbuh xabbuh modified the milestones: 5.1, next May 5, 2020
@fabpot
Copy link
Member

fabpot commented Sep 1, 2020

Thank you @xabbuh.

@fabpot fabpot mentioned this pull request Sep 1, 2020
Closed
@fabpot fabpot merged commit dc63d71 into symfony:master Sep 1, 2020
@xabbuh xabbuh deleted the issue-27217 branch September 1, 2020 16:30
yceruto
yceruto reviewed Sep 1, 2020
View reviewed changes
src/Symfony/Component/Form/Form.php
// Obviously, a disabled form should not change its data upon submission.
if ($this->isDisabled()) {
if ($this->isDisabled() && $this->isRoot()) {
Copy link
Member

@yceruto yceruto Sep 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for late feedback, but, if I'm not missing something, I think this change will break the current behavior.

Assuming you have a child field disabled, getting its value this way $form->get('field')->getData() will return the submitted data rather than its initial value as expected.

It is likely that the new condition $this->isRoot() should group the new code (the events being dispatched) in place, thus keeping the previous behavior of ignoring disabled child forms.

@yceruto yceruto mentioned this pull request Sep 1, 2020
Closed
@yceruto
Copy link
Member

yceruto commented Sep 2, 2020

Some BC breaks here found in #38025, I'm not sure if should we revert this or fix them?

@fabpot
Copy link
Member

fabpot commented Sep 2, 2020

@xabbuh I let you see if you prefer to revert for now or if we are confident that we will be able to fix the BC breaks before the release.

@fabpot
Copy link
Member

fabpot commented Sep 24, 2020

Reverting

fabpot added a commit that referenced this pull request Sep 24, 2020
@fabpot
Revert "feature #33381 [Form] dispatch submit events for disabled for…
2e819f3 
…ms too (xabbuh)"

This reverts commit dc63d71, reversing
changes made to 59ae592.
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yceruto yceruto yceruto left review comments

@fancyweb fancyweb fancyweb left review comments

@HeahDude HeahDude HeahDude left review comments

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
Bug Form Status: Reviewed
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
7 participants
@xabbuh @fancyweb @nicolas-grekas @fabpot @yceruto @HeahDude @carsonbot