8000 [Form] Support event listeners in buttons by fancyweb · Pull Request #33639 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[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

Closed
wants to merge 1 commit into from

Conversation

fancyweb
Copy link
Contributor
@fancyweb fancyweb commented Sep 19, 2019
Q A
Branch? 4.4
Bug fix? no
New feature? yes
Deprecations? yes
Tickets #8735
License MIT
Doc PR TODO

TODO:

  • Add tests

@@ -22,6 +25,11 @@
*/
class ButtonBuilder implements \IteratorAggregate, FormBuilderInterface
{
private const UNSUPPORTED_FORM_EVENTS = [
FormEvents::PRE_SET_DATA => [FormEvents::POST_SET_DATA],
Copy link
Contributor Author

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.

@fancyweb fancyweb force-pushed the form-button-event-listeners branch from 72628a0 to cf9ab1e Compare September 19, 2019 13:44
@yceruto yceruto added this to the next milestone Sep 19, 2019
@fancyweb fancyweb force-pushed the form-button-event-listeners branch 2 times, most recently from 142fa17 to ad5e077 Compare September 30, 2019 15:31
@fancyweb
Copy link
Contributor Author

Just added some tests.

@@ -22,6 +25,11 @@
*/
class ButtonBuilder implements \IteratorAggregate, FormBuilderInterface
{
private const UNSUPPORTED_FORM_EVENTS = [
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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 :)

Copy link
Contributor Author

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.

Copy link
Member
@yceruto yceruto Oct 1, 2019

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 :)

@fancyweb fancyweb force-pushed the form-button-event-listeners branch from ad5e077 to 3ff941e Compare October 1, 2019 11:45
@fancyweb fancyweb force-pushed the form-button-event-listeners branch from 3ff941e to 66420ad Compare October 28, 2019 08:47
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.

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);
Copy link
Contributor

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.

@xabbuh
Copy link
Member
xabbuh commented Nov 23, 2019

I agree with @HeahDude. So I am closing here. We can still reopen if a real use case pops up that isn't possible right now. Thank you for your work on this anyway @fancyweb!

@xabbuh xabbuh closed this Nov 23, 2019
@fancyweb fancyweb deleted the form-button-event-listeners branch November 23, 2019 12:32
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
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
0