Status: Reviewed then 👍 :)

@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 :)

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 onPOST_SUBMIT, the error is mapped (no matter where is thrown theTransformationFailedException, thePOST_SUBMIT event is still dispatched, because it's out of the try/catch inForm::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?

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.

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

Sure. TheValidationMapper,ValidationListener andFormValidator are tied to the validator component though, thus your willing to extract this "internal" error handling, which has nothing to do with validation with thesymfony/validator meaning of it, indeed.

But, would indeed extracting the transformation failure mapping part to theForm 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?

I've thought about that indeed but

registering another listener with higher priority and stopping propagation?

is a BC break :)

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 addFormError objects to the form (which is already coherent, asisValid will returnfalse with the changes in this PR).

Ok, two cases:

1. The error is mapped in theForm 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.

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.

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.

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 aForm and as theForm will handle the transformation failures mapping itself, there is no other case where the logic will be missed.

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

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

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

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 aPOST_SUBMIT listener from the constructor.

diff --git a/src/Symfony/Component/Form/Form.php b/src/Symfony/Component/Form/Form.phpindex 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 overridesForm::submit() without dispatchingPOST_SUBMIT, the listener in charge of mapping the transformation failure would not be called...but neither will be theValidationListener anyway currently. Otherwise, everything should work fine, as before.

Yeah this is the case 2.

@HeahDude What is the state here?

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.

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.