Skip to content
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

[BC Break][Bug] Form is validating constraints of wrong validation group in 3.1.4 / 3.2 #20853

Closed
peterrehm opened this issue Dec 9, 2016 · 7 comments

Comments

@peterrehm
Copy link
Contributor

#19388 introduces a BC break which just hit me.

I am having an application where FOSUserBundle is used and I have a form manipulating some fields of the custom user entity. Since the above mentioned PR the validator insists on constraints which are not in the same validation group.

E.g. the plainPassword constraint (https://github.com/FriendsOfSymfony/FOSUserBundle/blob/master/Resources/config/validation.xml#L56) is a problem.

When looking at #19388 it seems after some debugging, that just all constraints of an entity are registered in the defaultValidationGroup which ist just wrong.

     $member->constraintsByGroup[$this->getDefaultGroup()][] = $constraint;

The constraint has the following config extracted from a dump:

NotBlank {#1492 
  +message: "fos_user.password.blank"
  +payload: null
  +"groups": array:3 [
    0 => "Registration"
    1 => "ResetPassword"
    2 => "ChangePassword"
  ]
}

As I see it it must not be registered into the default group which is User in this case.

@senaria can you have a look into that again? I havent had much time to wok around this,
but based on your PR comments I think the adjustment should be mage related to GroupSequence
only?

@yakobe
Copy link
Contributor

yakobe commented Dec 10, 2016

I'm not sure I understand your example. Could you add some more information so i can understand your issue?

@xabbuh
Copy link
Member

xabbuh commented Jan 3, 2017

@peterrehm Are you able to provide a test case that used to pass before, but broke with #19388? I would like to look into a solution that wouldn't break @yakobe's fix.

@peterrehm
Copy link
Contributor Author

@xabbuh Please have a look at #20857 I recommend to revert asap and to look for a new solution. I dont think it will be an easy solution.

@yakobe
Copy link
Contributor

yakobe commented Jan 3, 2017

I find it a little dangerous to simply revert the code without a new solution for the group sequence. That would technically create another BC break for people that rely on that functionality. Myself, for example 😁. And also @senaria (the original author of the pr)

@peterrehm
Copy link
Contributor Author

You two are from the same company, and you broke the validator. That is why imho the revert is needed asap, knowing you dislike it 😃. Then someone should look for a proper workaround. Cant you guys look into this?

@yakobe
Copy link
Contributor

yakobe commented Jan 3, 2017

We both have projects that will break if it is reverted. Perhaps many more people do now too? Once something is in a "stable" release, people base solutions on it. It is currenly not so much 'broken' but providing a solution for another use case.

I agree that the current situation is not optimal. I'm swamped right now, but I can take a look as soon as possible. However, before I start implementing a solution i would prefer to reach an agreement about how it should be solved. Otherwise we risk breaking something for someone else, or possibly spending much time developing a solution that would not be accepted. If the repository mainteiners are happy with a proposal then anyone can pick this up... you, me, @senaria, or someone else equally invested in the topic.

@xabbuh
Copy link
Member

xabbuh commented Jan 6, 2017

@peterrehm Can you check if #21183 fixes your issue?

@yakobe Can you confirm that your use case doesn't break with the changes I propose in #21183?

fabpot added a commit that referenced this issue Jan 10, 2017
This PR was merged into the 2.7 branch.

Discussion
----------

[Validator] respect groups when merging constraints

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

Commits
-------

9a60057 respect groups when merging constraints
@fabpot fabpot closed this as completed Jan 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants