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]  Allow Form events to be used as expected with inherit_data option #40515

Open
wants to merge 3 commits into
base: 7.2
Choose a base branch
from

Conversation

cristoforocervino
Copy link
Contributor

@cristoforocervino cristoforocervino commented Mar 18, 2021

Q A
Branch? 7.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets #8607, #8834
License MIT
Doc PR TODO

This PR allows PRE_SET_DATA, POST_SET_DATA and SUBMIT form events to be used also with inherit_data option set to true.
On top of that, usage of PRE_SUBMIT and POST_SUBMIT was allowed with inherit_data option but dispatched events have $event->getData() as null. This PR addresses that issue as well.

Even if the documentation explains (at the very bottom of this page) that those form events are not going to work when the option inherit_data is used, I think it's a common problem you could run into with an advanced use of Form component (also because no exception is thrown when you use *_SET_DATA, SUBMIT and inherit_data together).

This is important to keep the high consistency standard Form component has.
Without this PR, the form component behaviour is ambiguous but known and documented, so I think we should consider this a new feature instead of a bug fix.

@carsonbot carsonbot changed the title [Form] Allow PRE_SET_DATA and POST_SET_DATA to be used with inherit_data option [Form]  Allow PRE_SET_DATA and POST_SET_DATA to be used with inherit_data option Mar 18, 2021
@cristoforocervino cristoforocervino force-pushed the pre-set-data-event-to-inherit-data-true-children branch 4 times, most recently from d6aff48 to c833e0f Compare March 18, 2021 22:58
@xabbuh xabbuh added this to the 5.x milestone Mar 19, 2021
@cristoforocervino cristoforocervino force-pushed the pre-set-data-event-to-inherit-data-true-children branch from b0c7af7 to 46be0ae Compare March 19, 2021 10:26
@cristoforocervino cristoforocervino changed the title [Form]  Allow PRE_SET_DATA and POST_SET_DATA to be used with inherit_data option [Form]  Allow Form events to be used as expected with inherit_data option Mar 19, 2021
@cristoforocervino cristoforocervino force-pushed the pre-set-data-event-to-inherit-data-true-children branch 3 times, most recently from adbecf2 to 2ae62b6 Compare March 19, 2021 10:40
@nicolas-grekas nicolas-grekas changed the title [Form]  Allow Form events to be used as expected with inherit_data option [Form] Allow Form events to be used as expected with inherit_data option Mar 19, 2021
@xabbuh
Copy link
Member

xabbuh commented Mar 26, 2021

I am a bit worried that merging this would break existing applications that rely on the fact that their listeners are not called on forms that have the inherit_data option enabled.

@cristoforocervino
Copy link
Contributor Author

cristoforocervino commented Mar 26, 2021

Should this behavior enabled with an option?
I don't know, this is probably a BC Symfony 6.x could have but I see no reason to wait until then.
I'm open to suggestions to change this feature in order to make it safer for existing applications.

@xabbuh
Copy link
Member

xabbuh commented Mar 26, 2021

Do you have a concrete use case in mind where removing this limitation would be useful?

@cristoforocervino
Copy link
Contributor Author

Using PRE_SET_DATA event is the only way to add a form child and change its options based on given object state.

Let's suppose we have a form child we want to render as disabled false when creating a new entity (letting the user to set its value) and render it as disable true while editing (because the value cannot be changed but has to be shown).

There is no way to achieve that without PRE_SET_DATA on forms with inherit_data option.
The above is the actual use case that made me discover this Symfony Form limitation.

@cristoforocervino cristoforocervino force-pushed the pre-set-data-event-to-inherit-data-true-children branch 2 times, most recently from bbdd335 to 3d1f8d4 Compare April 15, 2021 13:54
@fabpot fabpot removed this from the 5.4 milestone Nov 16, 2021
@carsonbot carsonbot changed the title [Form] Allow Form events to be used as expected with inherit_data option [Form]  Allow Form events to be used as expected with inherit_data option Oct 17, 2022
@cristoforocervino cristoforocervino force-pushed the pre-set-data-event-to-inherit-data-true-children branch from c0fdb09 to e1ccfe3 Compare October 21, 2022 16:23
@cristoforocervino cristoforocervino requested review from stof and removed request for dunglas, lyrixx, wouterj, sroze, xabbuh, yceruto and chalasr October 21, 2022 16:24
@nicolas-grekas nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022
@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@Amunak
Copy link
Contributor

Amunak commented Sep 26, 2023

Any chance of moving this forward? It seems extremely counter-intuitive that some FormEvents are called on child forms and others aren't.

@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

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

Any event allowing to modify the data is actually incompatible with inherit_data by design. So I'm not sure we should pursue this.

if ($dispatcher->hasListeners(FormEvents::PRE_SET_DATA)) {
$event = new PreSetDataEvent($form, $modelData);
$dispatcher->dispatch($event, FormEvents::PRE_SET_DATA);
$modelData = $event->getData();
Copy link
Member

Choose a reason for hiding this comment

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

This would not allow child forms with inherit_data to change the model data of the parent form. This is a very bad idea IMO (and was one of the reasons why this was not supported)

}
$event = new PreSubmitEvent($form, $eventSubmittedData);
$dispatcher->dispatch($event, FormEvents::PRE_SUBMIT);
$submittedData = $event->getData();
Copy link
Member

Choose a reason for hiding this comment

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

This is broken. It will replace the whole submitted data by the data of the child only.

@Amunak
Copy link
Contributor

Amunak commented Dec 8, 2023

Any event allowing to modify the data is actually incompatible with inherit_data by design. So I'm not sure we should pursue this.

Why do you consider them incompatible? If this is by design it should be documented that some events trigger every time and others don't.

@stof
Copy link
Member

stof commented Dec 8, 2023

@Amunak see my comments on those lines that describe the problems.

@cristoforocervino
Copy link
Contributor Author

@stof I understand. But if this is incompatible by design, wouldn't be better to throw an exception when you use *_SET_DATA, SUBMIT and inherit_data together?

@symfony symfony deleted a comment from carsonbot Dec 9, 2023
@xabbuh xabbuh modified the milestones: 7.1, 7.2 May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants