Fix identity through foreign entities

[vhenzl]

Failing tests for #6531 (and also #6043).

Tests are based on examples from Composite and Foreign Keys as Primary Key tutorial. None of all three examples of Identity through foreign Entities works at the moment. All of them fail with a similar error:

[Doctrine\ORM\ORMInvalidArgumentException] The given entity of type 'GH6531Address' (GH6531Address@0000000013e28b9b000000003fade3e6) has no identity/no id values set. It cannot be added to the identity map.

[Ocramius]

You should probably use the ::class syntax here

[Ocramius]

Catch only tools exceptions, please

[vhenzl]

I've copy-pasted the try/catch from some older test. Is it actually good for something? It's never used in more recent tests as I can see now. Shouldn't I rather throw it away?

[Ocramius]

Use a public property instead

[Ocramius]

Remove any fields and methods that aren't strictly required for the test scenario

[Ocramius]

If the title is not required, drop this

[Ocramius]

Same as above: drop any non-strictly-required fields

[Ocramius]

Same: drop anything that isn't strictly needed

[Ocramius]

Use ::class also when referencing classes in annotations

[Ocramius]

Drop passing tests fron the testcase

[Ocramius]

Ah, never mind, not this one 🤔

[Ocramius]

@vhenzl does this set of tests fail also on 2.5.x?

[Ocramius]

Most likely, but if you throw it away you also need a tearDown to handle dropping tables at the end of a test cycle On 13 Sep 2017 08:26, "Vašek Henzl" <notifications@github.com> wrote: *@vhenzl* commented on this pull request. ------------------------------ In tests/Doctrine/Tests/ORM/Functional/Ticket/GH6531Test.php <#6701 (comment)>:

+ protected function setUp(): void

+ { + parent::setup(); + + try { + $this->_schemaTool->createSchema(array( + $this->_em->getClassMetadata(__NAMESPACE__ . '\GH6531User'), + $this->_em->getClassMetadata(__NAMESPACE__ . '\GH6531Address'), + $this->_em->getClassMetadata(__NAMESPACE__ . '\GH6531Article'), + $this->_em->getClassMetadata(__NAMESPACE__ . '\GH6531ArticleAttribute'), + $this->_em->getClassMetadata(__NAMESPACE__ . '\GH6531Customer'), + $this->_em->getClassMetadata(__NAMESPACE__ . '\GH6531Order'), + $this->_em->getClassMetadata(__NAMESPACE__ . '\GH6531OrderItem'), + $this->_em->getClassMetadata(__NAMESPACE__ . '\GH6531Product'), + )); + } catch (\Exception $e) { I've copy-pasted the try/catch from some older test. Is it actually good for something? It's never used in more recent tests as I can see now. Shouldn't I rather throw it away? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#6701 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakHO09g1xV4yY3G5uAcX_hCPYz1RBks5sh3V3gaJpZM4PVkuP> .

[vhenzl]

does this set of tests fail also on 2.5.x?

@Ocramius On 2.5 branch fails only first test (testSimpleDerivedIdentity) where entities have OneToOne relation, other two tests with ManyToOne pass.

[vhenzl]

@Ocramius, I did some digging in the code and commit history regarding this issue and I have a question: Was this use case actually ever supported? Ie. identity through foreign entities with id strategy set to "AUTO" (ie. to "IDENTITY" or "SEQUENCE")?

Current error

Doctrine\ORM\ORMInvalidArgumentException: The given entity of type 'GH6531Address' (GH6531Address@0000000013e28b9b000000003fade3e6) has no identity/no id values set. It cannot be added to the identity map.

appears since b889e18. Before that commit, error message would be

Doctrine\ORM\ORMException: Entity of type GH6531Address has identity through a foreign entity GH6531User, however this entity has no identity itself. You have to call EntityManager#persist() on the related entity and make sure that an identifier was generated before trying to persist 'GH6531Address'. In case of Post Insert ID Generation (such as MySQL Auto-Increment) this means you have to call EntityManager#flush() between both persist operations.

There was a discussion (see here) in PR #1176 if the check throwing this exception should be removed or not. It was removed eventually, but apparently shouldn't be.
Also note, that ORMException::entityMissingForeignAssignedId($entity, $relatedEntity) throwing this exception isn't call anywhere since then.

Anyway, according to this error message, I assume that identity through foreign entities with database generated id never was supported and this PR doesn't make sense then.

If this is true, then the documentation is wrong (from the beginning) and needs to be updated to use @GeneratedValue(strategy="NONE") instead of @GeneratedValue.

With @GeneratedValue(strategy="NONE") and manually assigned IDs everything works as expected.

---

Side note on different behaviour for 2.5 and 2.6 when in master branch fail all three tests but on 2.5 only first one.
This happens because of behaviour before #5924 (commit 1cb8d79) when entity identifier could contain null. And because both GH6531ArticleAttribute and GH6531OrderItem have a composite key, $idHash wasn't an empty string and therefore the exception wasn't thrown and it worked somehow.

[lcobucci]

Was this use case actually ever supported?

@vhenzl since they're part of the documentation I'd assume that the feature worked fine in the past and we broke that due to lack of testing...

[lcobucci]

@vhenzl @Ocramius I've rebased the branch to sync with master and updated the tests to add the proper assertions but honestly I don't see an easy solution for this, since it's related to how we read/write from/to the identity map. Maybe @guilhermeblanco could give us some ideas on how to fix this, but regardless of that I think that this should be fixed on v2.6.1 or even v3.0.

(edit)

After thinking a bit more, I believe we should fix this on v2.6.0 since it these are examples from our documentation and IMO they MUST work.

[guilhermeblanco]

@vhenzl you are indeed correct.
*ToOne Identifiers pointing to a postInsert generated values were never supported since 2.0.0 Beta 2, when we supported association identifiers.

We're now discussing internally the procedure to take (if we'll solve this in 2.X or 3.0 only). In the meantime, switch the User to assigned and it should work.

PS: And the documentation should also be addressed... =(

[soyuka]

Just adding some use case for this particular issue. Assuming you have an association (can be seen as a Many To Many relation) that need additional information, you're defining a new Entity.

For example, here is a relational entity that has a "name":

/**
 * @ORM\Entity
 */
class RelatedToDummyFriend
{
    /**
     * @ORM\Column
     */
    public $name;

    /**
     * @ORM\Id
     * @ORM\ManyToOne(targetEntity="DummyFriend")
     * @ORM\JoinColumn(name="dummyfriend_id", referencedColumnName="id", nullable=false)
     */
    public $dummyFriend;

    /**
     * @ORM\Id
     * @ORM\ManyToOne(targetEntity="RelatedDummy", inversedBy="relatedToDummyFriend")
     * @ORM\JoinColumn(name="relateddummy_id", referencedColumnName="id", nullable=false, onDelete="CASCADE")
     */
    public $relatedDummy;
}

This is a common use case to me. Using @ORM\Id just helps identifying that the columns are our identifiers. Though, we don't need to add a generated value or any other identifier, the association ones are sufficient.

This used to work pre 2.6 and IMO should still work. My error is now:

identity/no id values set. It cannot be added to the identity map

Note that this is how it's persisted:

$relatedDummy = new RelatedDummy();
$relatedDummy->setName('RelatedDummy with friends');
$this->manager->persist($relatedDummy);

$friend = new DummyFriend();
$friend->setName('Friend-'.$i);
$this->manager->persist($friend);

$relation = new RelatedToDummyFriend();
$relation->setName('Relation');
$relation->setDummyFriend($friend);
$relation->setRelatedDummy($relatedDummy);

$this->manager->persist($relation);

Both RelatedDummy and DummyFriend have generated identifiers. Though, the RelatedToDummyFriend doesn't.

[lcobucci]

Just adding some use case for this particular issue. Assuming you have an association (can be seen as a Many To Many relation) that need additional information, you're defining a new Entity.

@soyuka why not using what the lib offers for many-to-many associations? I mean, creating an entity in the middle is not the same thing.

[soyuka]

Because you can (could?) not add custom columns to the ManyToMany entity (the association table). I think that I discussed that with @Ocramius somewhere but can't find the issue anymore. Anyway the conclusion was that if you need to add fields to a ManyToMany relation, you need to create a new entity.

Obviously, in the case above you could easily add a column with an @Id. Though IMHO it's cleaner to have RelationA-RelationB as index (same constraint as you would have in a ManyToMany relation).

[lcobucci]

Because you can (could?) not add custom columns to the ManyToMany entity (the association table). I think that I discussed that with @Ocramius somewhere but can't find the issue anymore. Anyway the conclusion was that if you need to add fields to a ManyToMany relation, you need to create a new entity.

Obviously, in the case above you could easily add a column with an @id. Though IMHO it's cleaner to have RelationA-RelationB as index (same constraint as you would have in a ManyToMany relation).

@soyuka I do agree that if (and when) needed you can have an entity to connect stuff but let's be clear that these are not many-to-many associations. Regardless of that, you still can have this kind of mapping, the point is that it cannot be done using a post generated ID, only that 😄

[soyuka]

let's be clear that these are not many-to-many

indeed those are not many-to-many, as said above can be seen as

It does work if my the two associations have been flushed before we save that entity yes :).

[lcobucci]

It does work if my the two associations have been flushed before we save that entity yes :).

Yeap, because then you do have identifiers for your relations 😉

[guilhermeblanco]

@soyuka the easiest solution is to add a reference identifier to this table and remove the @id from associations.

[soyuka]

What do you mean by "reference identifier"? I like having associations as identifier. Anyway, this works just fine as long as you save the relation before saving the relation entity.

[lcobucci]

@Ocramius I've moved the fix from #7003 (with some minor fixes) to this PR and things are working fine. We did have some impact on insertion performance (SimpleInsertPerformanceBench):

- benchHydration                I0 P0 	[μ Mo]/r: 480,363.000 480,363.000 (μs) 	[μSD μRSD]/r: 0.000μs 0.00%
+ benchHydration                I0 P0 	[μ Mo]/r: 482,352.000 482,352.000 (μs) 	[μSD μRSD]/r: 0.000μs 0.00%

Could you please check if this is acceptable or if we can optimise things a bit more?

[Ocramius]

@lcobucci that impact is OK 👍

[lcobucci]

@Ocramius alright then, I'll merge this in and make it part of v2.6.1.

[lcobucci]

@Ocramius I'm still working on porting this to master (which is being a little tricky), will continue on this tomorrow 👍

[tonivdv]

@lcobucci @Ocramius I'm currently having this issue in doctrine 2.5 ... But even after manually applying this PR's patch I still have the issue. So I wanted to know 1) if this was, according to you, a 2.6 issue only, or 2) it's an issue in 2.5 too and fixing it requires more than the current patch ?

[lcobucci]

@tonivdv since the release of v2.6.0 we no longer maintain v2.5.x and recommend every one to migrate to the newer version. We just backport security fixes to v2.5.x, which didn't happen so far 👍

[tonivdv]

@lcobucci Yeah I know and trust me I'm crying (:p) here but one of the projects can't be migrated to 2.6 because it can't be migrated easily at this stage to php 7 . Anyway, I changed the entity to prevent this issue, so as soon as we migrate the php 7 we'll update doctrine accordingly.

But according to you the issue is still present in 2.5 right?

Thanks for the quick response.

Cheers

[lcobucci]

But according to you the issue is still present in 2.5 right?

It's highly possible that it still there.