[Form] Fixed invalid state of non synchronized forms

[HeahDude]

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
Fixed tickets	#20916
License	MIT
Doc PR	~

The approach of this patch fixes the valid state of any form (including ChoiceType, see the related issue) for a minimal performance impact, but also introduces a minor BC break that we should allow in this case IMHO, because some data could be persisted with the pre set data ignoring an error occurring with a transformation failure and causing the field to not be synchronized, which looks really wrong to me.

[ogizanagi]

- if (!$this->submitted || $this->transformationFailure) {
+ if (!$this->submitted || !$this->isSynchronized()) {

?

[HeahDude]

Performance wise, this function call is useless IMO.

[ogizanagi]

Status: Reviewed then 👍 :)

[HeahDude]

@ogizanagi There is still a problem I think, we have now a potential invalid form without any error because the mapping is delegated to the validation extension of the form component...

If other symfony deciders agree with this one we should find a way to attach the error closer in the core, but this would be a bigger BC break, and I need to think more about how to work around that.

Suggestions are welcome though :)

[ogizanagi]

There is still a problem I think, we have now a potential invalid form without any error because the mapping is delegated to the validation extension of the form component...

Maybe I miss the point, but your changes won't change anything to the previous behavior, right? As soon as the validation extension is triggered on POST_SUBMIT, the error is mapped (no matter where is thrown the TransformationFailedException, the POST_SUBMIT event is still dispatched, because it's out of the try/catch in Form::submit()).

If other symfony deciders agree with this one we should find a way to attach the error closer in the core

Maybe we should, in order to make the form component working better without the validator component at least for transformation failures, but it's not the PR subject (and could be seen as an enhancement/feature).

Or am I missing something?

[HeahDude]

Maybe I miss the point, but your changes won't change anything to the previous behavior, right? As soon as the validation extension is triggered on POST_SUBMIT

This is true in the full stack as you said:

Maybe we should, in order to make the form component working better without the validator component

But this is more about the internal validation form extension, we're not even talking of the Validation component here.

I agree it should be very rare to use the Form component without all its core extensions, but we still need to support such use cases.

[ogizanagi]

But this is more about the internal validation form extension, we're not even talking of the Validation component here.

Sure. The ValidationMapper, ValidationListener and FormValidator are tied to the validator component though, thus your willing to extract this "internal" error handling, which has nothing to do with validation with the symfony/validator meaning of it, indeed.

But, would indeed extracting the transformation failure mapping part to the Form class or in a default listener be a BC break? It'll only adds errors on the form, and can still be disabled by registering another listener in userland with higher priority and stopping propagation, as before?

[HeahDude]

I've thought about that indeed but

registering another listener with higher priority and stopping propagation?

is a BC break :)

[ogizanagi]

No, I meant skipping validation (transformation failures or validator related one) would still work as before.
I didn't mean someone has to register it in order to workaround the supposed BC break.

I actually miss the situation where extracting this logic from the validation extension to be enabled by default in the Form would be a(nother) BC break. ? In the worst case, it'll only add FormError objects to the form (which is already coherent, as isValid will return false with the changes in this PR).

[HeahDude]

Ok, two cases:

1. The error is mapped in the Form class (my first preferred choice).

   Problem 1: the validation listener will map the error again (the best problem, should be manageable).

   Problem 2: if we change the logic in the listener we might break BC for someone using it for any edge case we cannot think of :/

2. The error is mapped in a core extension listener instead of the validation one.

   Problem 1: the error is not mapped for someone using its own core extension, at least if we find a solution for the first problem of the first case it should work here too but this problem here is a no go.

   Problem 2: the higher priority to stop the propagation will break any listener in user land code, this is a no go.

So case 2 is dead, I'll try to explore the "first choice" and problem 1 later, or any other good suggestion that comes.

[ogizanagi]

The error is mapped in the Form class (my first preferred choice).

Problem 1: the validation listener will map the error again (the best problem, should be manageable).

Why? If the logic is moved (not duplicated, moved from the validator extension to the Form), the error will not be mapped twice? It'll not be a BC break neither to me. The listener and validator are meant to be used with a form, so there is no other case where the logic about transformation failures will be missed.

Problem 2: if we change the logic in the listener we might break BC for someone using it for any edge case we cannot think of :/

Maybe. I can't argue against that ¯\(ツ)/¯, but right now I fail to see such cases.

[HeahDude]

Why? If the logic is moved (not duplicated, moved from the validator extension to the Form), the error will not be mapped twice?

If it's just moved, someone expecting this error mapping won't have it anymore.

[ogizanagi]

f it's just moved, someone expecting this error mapping won't have it anymore.

Sorry, I've edited my comment after and, I think, answered that:

It'll not be a BC break neither to me. The listener and validator are meant to be used with a Form and as the Form will handle the transformation failures mapping itself, there is no other case where the logic will be missed.

[HeahDude]

This can happen when extending the Form class and still expecting the validation listener to handle it.

[ogizanagi]

Just asking: Is not calling the parent constructor something fully covered by the BC promise? A POST_SUBMIT listener doing this job might be registered from here. Thus no problem if someone extends the Form class as soon as he calls the parent constructor (which anyway looks mandatory to me in the case of the Form class, as it initializes private properties that cannot be set otherwise).

[HeahDude]

Just asking: Is not calling the parent constructor something fully covered by the BC promise? A POST_SUBMIT listener doing this job might be registered from here. Thus no problem if someone extends the Form class as soon as he calls the parent constructor (which anyway looks mandatory to me in the case of the Form class, as it initialize private properties that cannot be set otherwise).

If I were about to move the code from the validation listener I would probably move it to ::submit(), but a class extending submit to handle its own logic but still dispatching the listeners would expect the validation listener to do its job => BC break.

This is very unlikely, but this is possible.

It would be better and not easier IMO, to try to avoid the duplication in the validation listener, and try to deprecate it in master to keep a migration path.

I'll give a try to the solution 1, opened to other solutions :)

[ogizanagi]

If I were about to move the code from the validation listener I would probably move it to ::submit(), but a class extending submit to handle its own logic but still dispatching the listeners would expect the validation listener to do its job => BC break.

Hence the solution I suggested about registering a POST_SUBMIT listener from the constructor.

diff --git a/src/Symfony/Component/Form/Form.php b/src/Symfony/Component/Form/Form.php
index c8acaf8..78d5bde 100644
--- a/src/Symfony/Component/Form/Form.php
+++ b/src/Symfony/Component/Form/Form.php
@@ -182,6 +182,8 @@ class Form implements \IteratorAggregate, FormInterface
 
         $this->config = $config;
         $this->children = new OrderedHashMap();
+
+        $this->config->getEventDispatcher()->addListener(FormEvents::POST_SUBMIT, new TransformationFailedListener());
     }
 
     public function __clone()

If someone overrides Form::submit() without dispatching POST_SUBMIT, the listener in charge of mapping the transformation failure would not be called...but neither will be the ValidationListener anyway currently. Otherwise, everything should work fine, as before.

[HeahDude]

Yeah this is the case 2.

[xabbuh]

@HeahDude What is the state here?

[HeahDude]

Will be rebased on master since the is a major behaviorial change IMO which will come with a deprecation, but I'm working on fixing #20935 first.

[HeahDude]

Will target 3.4 with bigger changes. 2.7 is not targetable globally, we need to try to fix the ChoiceType only.

Closing here for now.