[Form] Fixed potential BC break on submitting data to disabled child form #38025
Conversation
There was a problem hiding this comment.
I tried all kind of validations for disabled forms:
- constraints linked to the underlying object are working, OK,
- constraints linked to the root form, OK,
- constraints linked to form fields, KO! see related failure
I will try to find a solution this week.
| { | ||
| $form = $this->formFactory->createBuilder(FormType::class, ['field1' => 0]) | ||
| ->setDisabled(true) | ||
| ->add('field1', null, ['constraints' => new GreaterThanOrEqual(1)]) |
There was a problem hiding this comment.
It seems this kind of validation (tied directly to the form field) doesn't work because form fields will require to be marked as submitted too:
and currently only the root form is being marked as "submitted".
| { | ||
| $form = $this->formFactory | ||
| ->createBuilder(FormType::class, ['field1' => 0], [ | ||
| 'constraints' => new Callback(['callback' => static function ($data, ExecutionContextInterface $context) { |
There was a problem hiding this comment.
Even though constraints violation linked to the root form are working correctly.
| } | ||
| if ($dispatcher->hasListeners(FormEvents::SUBMIT)) { | ||
| $event = new FormEvent($this, $this->getNormData()); | ||
| $dispatcher->dispatch($event, FormEvents::SUBMIT); |
There was a problem hiding this comment.
I've added another test covering this case where the underlying object has been changed. This shouldn't happen if the entire form is disabled. Also this would break BC.
https://travis-ci.org/github/symfony/symfony/jobs/723435931#L3539
| $dispatcher->dispatch(FormEvents::POST_SUBMIT, $event); | ||
| if ($dispatcher->hasListeners(FormEvents::POST_SUBMIT)) { | ||
| $event = new FormEvent($this, $this->getViewData()); | ||
| $dispatcher->dispatch($event, FormEvents::POST_SUBMIT); |
|
@yceruto Any news on this PR? I'd like to take a decision about reverting the linked PR ASAP as 5.2 freeze time is approaching fast now. |
|
I tried to solve the initial BC here, but later I spotted others that I cannot solve. So I would revert so far. |
Addressing #33381 (comment)
Added test case.
For reviewers, better diff by hidding whitespace changes:
https://github.com/symfony/symfony/pull/38025/files?diff=unified&w=1