-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
[Form] invalidate forms on transformation failures #28731
Conversation
xabbuh
commented
Oct 4, 2018
•
edited
Loading
edited
Q | A |
---|---|
Branch? | 2.8 |
Bug fix? | yes |
New feature? | no |
BC breaks? | no |
Deprecations? | no |
Tests pass? | yes |
Fixed tickets | #20916, #21242, #28584 |
License | MIT |
Doc PR |
b7df8b9
to
6cf76d1
Compare
Test added, PR is ready from my POV |
$message = 'This value is not valid.'; | ||
|
||
if (null !== $this->translator) { | ||
$message = $this->translator->trans($message, array('{{ value }}' => $clientDataAsString)); |
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.
The message is static then shouldn't the placeholder be there? ->trans('This value {{ value }} is not valid.', array('{{ value }}' => $clientDataAsString))
?
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.
fixed, thanks for catching it
$message = $this->translator->trans($message, array('{{ value }}' => $clientDataAsString)); | ||
} | ||
|
||
$form->addError(new FormError($message, 'This value is not valid.', array('{{ value }}' => $clientDataAsString), null, $form->getTransformationFailure())); |
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.
Same here for the message template (2nd argument)
src/Symfony/Component/Form/Extension/Core/Type/TransformationFailureExtension.php
Show resolved
Hide resolved
ping @symfony/deciders :) |
foreach ($form as $child) { | ||
if (!$child->isSynchronized()) { | ||
$childrenSynchronized = false; | ||
break; |
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.
Would be more elegant to just return
here. Then you wouldn't need $childrenSynchronized variable and guard it later at all
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.
So elegant :), 👍
Thank you @xabbuh. |
This PR was merged into the 2.8 branch. Discussion ---------- [Form] invalidate forms on transformation failures | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20916, #21242, #28584 | License | MIT | Doc PR | Commits ------- 385d9df invalidate forms on transformation failures