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

Merged
merged 1 commit into from Nov 12, 2018

Conversation

Projects
None yet
6 participants
@xabbuh
Member

xabbuh commented Oct 4, 2018

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

@xabbuh xabbuh added this to the 2.8 milestone Oct 4, 2018

@xabbuh xabbuh force-pushed the xabbuh:issue-20916 branch from 57e49ed to 0309d2c Oct 19, 2018

@xabbuh xabbuh force-pushed the xabbuh:issue-20916 branch 4 times, most recently from b7df8b9 to 6cf76d1 Oct 28, 2018

@xabbuh xabbuh changed the title from [WIP][Form] invalidate forms on transformation failures to [Form] invalidate forms on transformation failures Oct 31, 2018

@xabbuh

This comment has been minimized.

Member

xabbuh commented Oct 31, 2018

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

This comment has been minimized.

@yceruto

yceruto Nov 1, 2018

Contributor

The message is static then shouldn't the placeholder be there? ->trans('This value {{ value }} is not valid.', array('{{ value }}' => $clientDataAsString)) ?

This comment has been minimized.

@xabbuh

xabbuh Nov 5, 2018

Member

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()));

This comment has been minimized.

@yceruto

yceruto Nov 1, 2018

Contributor

Same here for the message template (2nd argument)

Show resolved Hide resolved ...ny/Component/Form/Extension/Core/Type/TransformationFailureExtension.php

@xabbuh xabbuh force-pushed the xabbuh:issue-20916 branch from 423460b to 14f35a3 Nov 5, 2018

@xabbuh

This comment has been minimized.

Member

xabbuh commented Nov 10, 2018

foreach ($form as $child) {
if (!$child->isSynchronized()) {
$childrenSynchronized = false;
break;

This comment has been minimized.

@ostrolucky

ostrolucky Nov 10, 2018

Contributor

Would be more elegant to just return here. Then you wouldn't need $childrenSynchronized variable and guard it later at all

@xabbuh xabbuh force-pushed the xabbuh:issue-20916 branch from 14f35a3 to 385d9df Nov 11, 2018

@HeahDude

So elegant :), 👍

@fabpot

fabpot approved these changes Nov 12, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Nov 12, 2018

Thank you @xabbuh.

@fabpot fabpot merged commit 385d9df into symfony:2.8 Nov 12, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Nov 12, 2018

bug #28731 [Form] invalidate forms on transformation failures (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

@xabbuh xabbuh deleted the xabbuh:issue-20916 branch Nov 12, 2018

@fabpot fabpot referenced this pull request Nov 16, 2018

Merged

Release v4.2.0-BETA2 #29237

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment