[Form] Support event listeners in buttons #33639
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.
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.
This also define the supported form events, what about UNSUPPORTED_SUPPORTED_FORM_EVENTS instead?
There was a problem hiding this comment.
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.
Yes, but self::UNSUPPORTED_FORM_EVENTS[$eventName] = Events from the FormEvents class that are supported by buttons :)
There was a problem hiding this comment.
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.
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.
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.
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