Skip to content

UniqueEntityValidator with multiple constraints fails if the first constraint is null #21554

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
ghost opened this issue Feb 7, 2017 · 8 comments
Closed

UniqueEntityValidator with multiple constraints fails if the first constraint is null #21554

ghost opened this issue Feb 7, 2017 · 8 comments
Labels
Bug DoctrineBridge Status: Needs Review Validator
Milestone
2.7

Comments

@ghost
Copy link

ghost commented Feb 7, 2017

Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 2.8.17

Scenario: I'm validating an entity like this

$validator->validate($device, [
    new UniqueEntity([
        'fields' => ['accountId', 'tenantUuid'],
        'em' => $provisioningEm
    ])
]);

If accountId happens to be null, UniqueEntityValidator will now exclude it from the constraint list (since commit 1e3421d ), failing later on because $criteria[$fields[0]] is an invalid array access: fields[0] is the string "accountId", which is not in $criteria.
Reverting lines 88-94 to the version before the commit linked above fixes the issue.
@xabbuh xabbuh added this to the 2.7 milestone Feb 7, 2017
@xabbuh xabbuh mentioned this issue Feb 7, 2017
Merged
@xabbuh
Copy link
Member

xabbuh commented Feb 7, 2017

@gigib82 Sorry for breaking this. Can you confirm that #21562 fixes the issue for you?

@ghost
Copy link
Author

ghost commented Feb 8, 2017

Tested #21562: there is now a behavioural difference with respect to the previous released version.
Using my own example, a null accountId and a non-null tenantUuid used to always pass validation, even if in the DB there was already a record with null accountId and the same tenantUuid. The same scenario now fails validation.
I'm not sure which is the most correct behaviour, but merging this will cause a small BC break.

@xabbuh
Copy link
Member

xabbuh commented Feb 8, 2017

@gigib82 Did you enable or disable the ignoreNull option?

@ghost
Copy link
Author

ghost commented Feb 8, 2017

No, never explicitly set that option.

@xabbuh
Copy link
Member

xabbuh commented Feb 8, 2017

@gigib82 You are right. It's not related to the changes from #21562, but was introduced in #21431. You were just not able to see the wrong behaviour due to the initial issue here. I'll work on fixing it completely.

@xabbuh
Copy link
Member

xabbuh commented Feb 9, 2017

@gigib82 I updated #21562. Would you mind checking it again?

@ghost
Copy link
Author

ghost commented Feb 9, 2017

The same test suite that spotted the errors is now ok, so I would say #21562 fixes the error

@xabbuh
Copy link
Member

xabbuh commented Feb 9, 2017

Thanks for the confirmation.

@trsteel88 trsteel88 mentioned this issue Feb 9, 2017
Closed
fabpot added a commit that referenced this issue Feb 12, 2017
@fabpot
bug #21562 [DoctrineBridge] make sure that null can be the invalid va…
e7541d9 
…lue (xabbuh)

This PR was merged into the 2.7 branch.

Discussion
----------

[DoctrineBridge] make sure that null can be the invalid value

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

Commits
-------

c3702c2 make sure that null can be the invalid value
@fabpot fabpot closed this as completed Feb 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug DoctrineBridge Status: Needs Review Validator
Projects
None yet
Milestone
2.7
Development

No branches or pull requests
3 participants
@fabpot @xabbuh @carsonbot