8000 [Form] Fixed invalid state of non synchronized forms by HeahDude · Pull Request #20966 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] Fixed invalid state of non synchronized forms #20966

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Form] Fixed invalid state of non synchronized forms #20966

wants to merge 1 commit into from

Conversation

HeahDude
Copy link
Contributor
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.

@HeahDude HeahDude mentioned this pull request Dec 16, 2016
Closed
ogizanagi
ogizanagi reviewed Dec 16, 2016
View reviewed changes
src/Symfony/Component/Form/Form.php
@@ -758,7 +758,7 @@ public function isEmpty()
*/
public function isValid()
{
if (!$this->submitted) {
if (!$this->submitted || $this->transformationFailure) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance wise, this function call is useless IMO.

@ogizanagi
Copy link
Contributor

Status: Reviewed then 👍 :)

@HeahDude
Copy link
Contributor Author

@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
Copy link
Contributor
ogizanagi commented Dec 16, 2016 •
edited
Loading

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
Copy link
Contributor Author

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
Copy link
Contributor
ogizanagi commented Dec 16, 2016 •
edited
Loading

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
Copy link
Contributor Author

I've thought about that indeed but

registering another listener with higher priority and stopping propagation?

is a BC break :)

@ogizanagi
Copy link
Contributor
ogizanagi commented Dec 16, 2016 •
e 8000 dited
Loading

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
Copy link
Contributor Author
HeahDude commented Dec 16, 2016 •
edited
Loading

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
Copy link
Contributor
ogizanagi commented Dec 16, 2016 •
edited
Loading

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
Copy link
Contributor Author

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
Copy link
Contributor
ogizanagi commented Dec 16, 2016 •
edited
Loading

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
Copy link
Contributor Author

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

@ogizanagi
Copy link
Contributor
ogizanagi commented Dec 16, 2016 •
edited
Loading

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
Copy link
Contributor Author
HeahDude commented Dec 16, 2016 •
edited
Loading

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
Copy link
Contributor
ogizanagi commented Dec 16, 2016 •
edited
Loading

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
Copy link
Contributor Author

Yeah this is the case 2.

This was referenced Dec 17, 2016
Closed
Merged
@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Dec 26, 2016
@xabbuh
Copy link
Member
xabbuh commented Jan 11, 2017

@HeahDude What is the state here?

@HeahDude
Copy link
Contributor Author

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.

@ogizanagi ogizanagi mentioned this pull request Jan 12, 2017
Closed
@HeahDude
Copy link
Contributor Author
HeahDude commented Jul 6, 2017

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.

@HeahDude HeahDude closed this Jul 6, 2017
@HeahDude HeahDude deleted the fix/expanded-invalid-choice_type branch November 24, 2018 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogizanagi ogizanagi ogizanagi left review comments

Assignees
No one assigned
Labels
BC Break Bug Form Status: Needs Review
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.
5 participants
@HeahDude @ogizanagi @xabbuh @nicolas-grekas @carsonbot
0