-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Form] Support event listeners in buttons #33639
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
Conversation
@@ -22,6 +25,11 @@ | |||
*/ | |||
class ButtonBuilder implements \IteratorAggregate, FormBuilderInterface | |||
{ | |||
private const UNSUPPORTED_FORM_EVENTS = [ | |||
FormEvents::PRE_SET_DATA => [FormEvents::POST_SET_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.
I just don't know if we can prevent userland from using our events?
I chose to not dispatch useless events. However, if userland dispatch them manually, it can still be useful to be able to listen to them. But I guess this would be a weird practice 😕 Personally, I just prefer to forbid them.
72628a0
to
cf9ab1e
Compare
142fa17
to
ad5e077
Compare
Just added some tests. |
@@ -22,6 +25,11 @@ | |||
*/ | |||
class ButtonBuilder implements \IteratorAggregate, FormBuilderInterface | |||
{ | |||
private const UNSUPPORTED_FORM_EVENTS = [ |
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.
This also define the supported form events, what about UNSUPPORTED_SUPPORTED_FORM_EVENTS
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.
For me:
FORM_EVENTS
= The 5 form events from the FormEvents
class.
UNSUPPORTED_FORM_EVENTS
= Events from the FormEvents
class that are not supported by buttons.
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.
Yes, but self::UNSUPPORTED_FORM_EVENTS[$eventName]
= Events from the FormEvents
class that are supported by buttons :)
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.
Alright, I see what you mean 😄 However, I still think the name applies to the whole const (what it represents) and not to its "values" only.
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.
My suggestion is to find a better name that makes it clearer. I could also name it SUPPORTED_FORM_EVENTS
due to its values, so we've the chicken-and-egg situation here :)
Other names could be: FORM_EVENTS_SUPPORT
, BUTTON_EVENTS
, ... though not sure they're better :)
ad5e077
to
3ff941e
Compare
3ff941e
to
66420ad
Compare
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.
Unless there is a real use case we want to support, I think that such logic should be attached to the form itself, not the button.
Modifying the form tree is not possible on post submit event, and for a button trying to modify its parent may have side effects since some other fields may have been submitted and some may not already.
Also the current implementation adds overhead on every existing buttons and submit buttons of all projects, there is no opt-in easy solution though.
If we were to accept such feature we should at least take care of removing support for standard buttons as there is no way to check the click state server side.
Currently I'm 👎 on this, and I think we should close the related issue as "won't fix".
@@ -206,11 +206,11 @@ public function getOptionsResolver() | |||
protected function newBuilder($name, $dataClass, FormFactoryInterface $factory, array $options) | |||
{ | |||
if ($this->innerType instanceof ButtonTypeInterface) { | |||
return new ButtonBuilder($name, $options); | |||
return new ButtonBuilder($name, new EventDispatcher(), $options); |
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.
There is not point in this support, we never check server side if a button is clicked, only one submit button per request.
TODO:
Add tests