[Doctrine Bridge] fix UniqueEntityValidator for composite object primary keys #21288

Merged
merged 2 commits into from Jan 16, 2017

Projects

None yet

5 participants

@dmaicher
Contributor
dmaicher commented Jan 14, 2017 edited
Q A
Branch? master / 3.1
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes (fail on php 7.1 unrelated?)
Fixed tickets #21274
License MIT
Doc PR -

This PR fixes an issue with the UniqueEntityValidator in case the entity being validated uses a composite primary key via relations to other entities whose classes do not have a __toString method.

@dmaicher dmaicher [Doctrine Bridge] fix UniqueEntityValidator for composite object prim…
…ary keys
5aadce3
+ $identifiers = array_map(function ($identifier) use ($em) {
+ // identifiers can be objects (without any __toString method) if its a composite PK
+ if (is_object($identifier) && !method_exists($identifier, '__toString')) {
+ return sprintf('(%s)', $this->buildInvalidValueString($em, $em->getClassMetadata(get_class($identifier)), $identifier));
@dmaicher
dmaicher Jan 14, 2017 edited Contributor

is it safe to use the same object manager here for the related class? Not sure if Doctrine supports relations between classes of different object managers?

+
+ $this->validator->validate($newEntity, $constraint);
+
+ $expectedValue = 'Object of class "Symfony\Bridge\Doctrine\Tests\Fixtures\CompositeObjectNoToStringIdEntity" identified by "(Object of class "Symfony\Bridge\Doctrine\Tests\Fixtures\SingleIntIdNoToStringEntity" identified by "1"), (Object of class "Symfony\Bridge\Doctrine\Tests\Fixtures\SingleIntIdNoToStringEntity" identified by "2")"';
@dmaicher
dmaicher Jan 14, 2017 Contributor

should we find a shorter way to build this message? 😝

@dmaicher
Contributor

@HeahDude as you added this code (40c0c52) what do you think about this?

@nicolas-grekas nicolas-grekas added this to the 3.1 milestone Jan 16, 2017
@HeahDude
Contributor

I've discussed this PR with @dmaicher in dmaicher#1, which is now merged.

This looks good to me.

ping @symfony/deciders

@nicolas-grekas nicolas-grekas changed the title from [Doctrine Bridge] fix UniqueEntityValidator for composite object prim… to [Doctrine Bridge] fix UniqueEntityValidator for composite object primary keys Jan 16, 2017
@nicolas-grekas
Member

Failure looks related to me (not the messages about getMock, but the one about CompositeObjectNoToStringIdEntity) - maybe a composer.json update needed somewhere?

@HeahDude
Contributor

@nicolas-grekas I've opened dmaicher#2 which seems to have fixed the tests, waiting for @dmaicher to merge it.

@nicolas-grekas
Member
nicolas-grekas commented Jan 16, 2017 edited

@dmaicher can you please squash the history? We can't merge (nor don't want to :) ) merge PRs with merges inside.

@nicolas-grekas
Member

(maybe keep just two commits, one for you and one for @HeahDude with rebase -i ?)

@dmaicher
Contributor

@nicolas-grekas sure 😉 Let's wait for the build and hope its fixed

@HeahDude
Contributor

Green 🎉

@HeahDude @dmaicher HeahDude [DoctrineBridge] Fixed invalid unique value as composite key
Fixed UniqueEntityValidator tests
b3ced86
@dmaicher
Contributor

Done 😉

@fabpot
Member
fabpot commented Jan 16, 2017

I've not checked, but don't we have the same issue in 2.7 and 2.8?

@HeahDude
Contributor

@fabpot This fix is related to a feature introduced in 3.1 #15279, and not enough fixed by #19227 (missing composite identifiers handling, provided here).

@fabpot
Member
fabpot commented Jan 16, 2017

Thank you @dmaicher.

@fabpot fabpot merged commit b3ced86 into symfony:3.1 Jan 16, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
@fabpot fabpot added a commit that referenced this pull request Jan 16, 2017
@fabpot fabpot bug #21288 [Doctrine Bridge] fix UniqueEntityValidator for composite …
…object primary keys (dmaicher, HeahDude)

This PR was merged into the 3.1 branch.

Discussion
----------

[Doctrine Bridge] fix UniqueEntityValidator for composite object primary keys

| Q             | A
| ------------- | ---
| Branch?       | master / 3.1
| Bug fix?      | yes
| New feature?  |no
| BC breaks?    | no
| Deprecations? |no
| Tests pass?   | yes (fail on php 7.1 unrelated?)
| Fixed tickets | #21274
| License       | MIT
| Doc PR        | -

This PR fixes an issue with the UniqueEntityValidator in case the entity being validated uses a composite primary key via relations to other entities whose classes do not have a `__toString` method.

Commits
-------

b3ced86 [DoctrineBridge] Fixed invalid unique value as composite key
5aadce3 [Doctrine Bridge] fix UniqueEntityValidator for composite object primary keys
d228d34
This was referenced Jan 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment