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

Closed
gigib82 opened this Issue Feb 7, 2017 · 8 comments

Projects

None yet

4 participants

@gigib82
gigib82 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
Member
xabbuh commented Feb 7, 2017

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

@gigib82
gigib82 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
Member
xabbuh commented Feb 8, 2017

@gigib82 Did you enable or disable the ignoreNull option?

@gigib82
gigib82 commented Feb 8, 2017

No, never explicitly set that option.

@xabbuh
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
Member
xabbuh commented Feb 9, 2017

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

@gigib82
gigib82 commented Feb 9, 2017

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

@xabbuh
Member
xabbuh commented Feb 9, 2017

Thanks for the confirmation.

@fabpot fabpot added a commit that referenced this issue Feb 12, 2017
@fabpot fabpot bug #21562 [DoctrineBridge] make sure that null can be the invalid va…
…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
e7541d9
@fabpot fabpot closed this Feb 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment