NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

[Form] Introduce validation events#47210

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

Seb33300 wants to merge4 commits intosymfony:7.4

base:7.4

Choose a base branch

fromSeb33300:form-validate-events

Open

[Form] Introduce validation events#47210

Seb33300 wants to merge4 commits intosymfony:7.4fromSeb33300:form-validate-events

+151 −7

Conversation

Copy link

Contributor

Seb33300 commentedAug 6, 2022•
edited
Loading

Q	A
Branch?	6.2
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix#47046
License	MIT
Doc PR	symfony/symfony-docs#17229

Inspired from#38479 (@alcaeus, I coud not reuse your commit because you deleted your branch)

With some changes:

I removed the BC break
form.pre_validate andform.post_validate events are triggered individually to all Form items, not only the to the root form (same as all others existing form events)

Regarding the second point, for example, when working with nested forms, if theform.post_validate event is attached to a child form:

$event->getForm()->isValid() will return the validity for the current form item only (the nested one)
If you want to check if the entire form is valid form a nested form, you will have to check the root form:$event->getForm()->getRoot()->isValid()

Usage example:

Thy are working in the same way as existing form events work.

class MyFormTypeextends AbstractType{publicfunctionbuildForm(FormBuilderInterface$builder,array$options):void    {$builder            ->addEventListener(ValidatorFormEvents::PRE_VALIDATE,function (FormEvent$event) {// ...            })            ->addEventListener(ValidatorFormEvents::POST_VALIDATE,function (FormEvent$event) {$event->getForm()->isValid();// Check if current form item is valid$event->getForm()->getRoot()->isValid();// Check if entire form is valid            })        ;    }}

Why are those events required?
Can't we just add aPOST_SUBMIT event with a lower priority than the validation event?
Yes, this solution can work, but ONLY on the root form.
Because the validation of the form is triggered only by the root form, and events are triggered individually on all form items, starting from ancestors and ending by the root form.
This means, we currently have no way to know if a nested form is valid when attaching a POST_SUBMIT event on it.

Seb33300 requested review fromxabbuh andyceruto ascode owners

August 6, 2022 10:45

carsonbot added Status: Needs Review Feature Form labels

Aug 6, 2022

carsonbot added this to the6.2 milestone

Aug 6, 2022

Seb33300 force-pushed theform-validate-events branch from64603b1 to134ff4bCompare

August 6, 2022 11:12

Copy link

ContributorAuthor

Seb33300 commentedAug 6, 2022

In case you want to review, I saw you participated to the previous PR
@d-ph @silasjoisten

Seb33300 mentioned this pull request

Aug 6, 2022

[Form] Addingvalid &invalid form events#47046

Closed

silasjoisten reviewed

Aug 7, 2022

View reviewed changes

src/Symfony/Component/Form/Extension/Validator/EventListener/ValidationListener.phpShow resolvedHide resolved

94noni reviewed

Aug 7, 2022

View reviewed changes

src/Symfony/Component/Form/Extension/Validator/ValidatorFormEvents.php OutdatedShow resolvedHide resolved

94noni reviewed

Aug 7, 2022

View reviewed changes

src/Symfony/Component/Form/Extension/Validator/EventListener/ValidationListener.phpShow resolvedHide resolved

Copy link

d-ph commentedAug 7, 2022•
edited
Loading

Hi.

Just wanted to say that I'm happy to see this feature gaining traction, and looking forward to seeing it in the stock Symfony code. Unfortunately, it's unlikely I'd review the PR in depth, however I did skim through the change, and it looks good to me.

May I ask a quick question though, just to double-check: Given the following case:

A root formfoo listening on the postValidate event.
A child formfooChild also listenting on the postValidate event.
The formfoo is being submitted.

Then:

fooChild's postValidate is going to be called first, and thenfoo's postValidate is going to be called second. I.e. The new events are dispatched recursively, deepest child form first?
IffooChild adds a new FormError on itself in the postValidate listener, thefoo's postValidate listener will be able to see that there are validation errors by calling either:$event->getForm()->isValid(), or$event->getForm()->get('fooChild')->isValid()?

Thanks.

Btw Seb33300 search for "->idValid(" on this page and consider correcting the typos, because it takes a few seconds to conclude that it's a typo. Cheers.

Copy link

ContributorAuthor

Seb33300 commentedAug 7, 2022•
edited
Loading

@d-ph typo fixed. Thanks.

POST_VALIDATE event happens AFTER the form validation has been processed ON ALL form items.

Yes, child form event will be dispatched first. The root form event will be the last one to be dispatched. (same order as others form events)
Yes, you can add a new error to the form using$event->getForm()->addError() inside this event. If you do so, the form will be considered invalid after that. When the event will be propagated to parent form, the parent form will also be considered as invalid. Because a form is valid only if all his children are valid.

Seb33300 mentioned this pull request

Aug 26, 2022

[Form] Introduce validation eventssymfony/symfony-docs#17229

Open

yceruto reviewed

Aug 27, 2022

View reviewed changes

Copy link

Member

yceruto left a comment

Choose a reason for hiding this comment

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

Thanks for this proposal. I've now a better idea about the problem to solve after reading your code and description. I left you one comment to improve the implementation based on your approach and the other two are mainly consistency-related concerns.

I'm wondering if dispatching these new events inside theFormValidator makes more sense, is more accurate, and simplifies the implementation too (as the collection of the form tree won't be needed).

Although I've to admit that I'm not entirely sure how useful this feature is or if we are opening the door to the writing of something completely detached from the Form context.

src/Symfony/Component/Form/Extension/Validator/EventListener/ValidationListener.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Form/Extension/Validator/EventListener/ValidationListener.php

		if ($form->isRoot()) {
		$this->dispatchEvents(ValidatorFormEvents::PRE_VALIDATE);

Copy link

Member

ycerutoAug 26, 2022

Choose a reason for hiding this comment

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

I like the goal of this new event but the dispatching moment is a bit confusing to me. If I create a listener in a nested form type, I expect the event to be received just before the current form is validated inFormValidator and not before the whole validation process of all forms in the tree is started.

The meaning is different in this case from other events.

Copy link

ContributorAuthor

Seb33300Aug 27, 2022

Choose a reason for hiding this comment

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

To be consistant with POST_VALIDATE event, I decided to dispatch it before the validation of the entire form start.
(see my other comment to know why POST_VALIDATE is dispatched after all form items are validated)

src/Symfony/Component/Form/Extension/Validator/EventListener/ValidationListener.php

		@@ -50,6 +64,24 @@ public function validateForm(FormEvent $event)

		$this->violationMapper->mapViolation($violation, $form, $allowNonSynchronized);
		}

		$this->dispatchEvents(ValidatorFormEvents::POST_VALIDATE);

Copy link

Member

ycerutoAug 26, 2022

Choose a reason for hiding this comment

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

I understand that child forms can check easily whether their parent is valid or not, but I have the same confusion about the right moment this event is being dispatched. They are not done just after the current form is validated but when the whole validation process is finished for all forms.

Copy link

ContributorAuthor

Seb33300Aug 27, 2022

Choose a reason for hiding this comment

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

This is the main purpose of this PR, to be able to know if the entire form is valid or not.
That's why the POST_VALIDATE event must be dispatched here.
If we dispatch the event just after the validation of the current item, we will only be able to know if the current item (and his parents) are valid.

Maybe we can introduce a 3rd event "VALIDATE" that could be dispatched just after the current form item is validated?