ManyToManyPersister fails to remove join table entry if there is multiple join columns #6997

NicolaF · 2018-01-19T17:10:52Z

This is due to a typo in \Doctrine\ORM\Persisters\Collection\ManyToManyPersister::getDeleteSQLParameters:

        foreach ($mapping['relationToSourceKeyColumns'] as $columnName => $refColumnName) {
            $params[] = isset($sourceClass->fieldNames[$refColumnName])
                ? $identifier[$sourceClass->fieldNames[$refColumnName]]
                : $identifier[$sourceClass->getFieldForColumn($columnName)];
        }

It should be $sourceClass->getFieldForColumn($refColumnName), as $columnName is the join table column, not the entity table column.

…rsister

Ocramius

Overall good patch. Comments are really just nits, but we may need to check whether removal of the EntityManager#flush() call is possible. This is done elsewhere via UnitOfWork#registerManaged() (which may not even be needed here)

Ocramius · 2018-01-22T06:48:04Z

tests/Doctrine/Tests/Models/ManyToManyPersister/ChildClass.php

@@ -0,0 +1,103 @@
+<?php
+/**


To be dropped

Ocramius · 2018-01-22T06:48:53Z

tests/Doctrine/Tests/Models/ManyToManyPersister/ChildClass.php

+    /**
+     * @return int
+     */
+    public function getId1(): int {


Remove all public API that isn't strictly needed. Use public properties instead

Ocramius · 2018-01-22T06:50:16Z

tests/Doctrine/Tests/Models/ManyToManyPersister/ParentClass.php

+    /**
+     * @return int
+     */
+    public function getId(): int {


Remove all public API that isn't strictly needed

Ocramius · 2018-01-22T06:51:00Z

tests/Doctrine/Tests/Models/ManyToManyPersister/ChildClass.php

+use Doctrine\ORM\Mapping\JoinColumn;
+use Doctrine\ORM\Mapping\JoinTable;
+/**
+ * Class ChildClass


Ocramius · 2018-01-22T06:51:09Z

tests/Doctrine/Tests/Models/ManyToManyPersister/ChildClass.php

+use Doctrine\ORM\Mapping\JoinTable;
+/**
+ * Class ChildClass
+ * @package Doctrine\Tests\Models\ManyToManyPersister


Ocramius · 2018-01-22T06:51:22Z

tests/Doctrine/Tests/Models/ManyToManyPersister/ParentClass.php

@@ -0,0 +1,72 @@
+<?php
+/**


To be removed

Ocramius · 2018-01-22T06:51:36Z

tests/Doctrine/Tests/ORM/Persisters/ManyToManyPersisterTest.php

@@ -0,0 +1,65 @@
+<?php
+/**


To be removed

Ocramius · 2018-01-22T06:54:05Z

tests/Doctrine/Tests/ORM/Persisters/ManyToManyPersisterTest.php

+        $lastUpdate = array_pop($updates);
+
+        $this->assertEquals('DELETE FROM parent_child WHERE child_id1 = ? AND child_id2 = ?', $lastUpdate['query']);
+        $this->assertEquals([1,2], $lastUpdate['params']);


Consider using fixed assigned identifiers instead of AUTO_INCREMENT to prevent issues with subsequent test runs.

Errata: I see you are already doing that 👍

Ocramius · 2018-01-22T06:55:14Z

tests/Doctrine/Tests/ORM/Persisters/ManyToManyPersisterTest.php

+        /** @var ConnectionMock $conn */
+        $conn = $this->em->getConnection();
+
+        $updates = $conn->getExecuteUpdates();


Wondering if a full mock connection would be better here, assuming that we completely get rid of that EntityManager#flush() call

NicolaF · 2018-01-22T09:41:10Z

Hi @Ocramius,

I cleaned the test case as you requested. Howerver, I'm not sure to understand what you want to do with the EntityManager#flush() call. ManyToManyPersister#delete requires a PersistentCollection (because it needs the mapping to construct the delete statement), so UnitOfWork#registerManaged() won't do the job IMO.

Best Regards,
NicolaF

lcobucci

I've just executed your tests without the fix and it passed, this doesn't look like an issue IMO...

NicolaF · 2018-01-23T09:45:01Z

@lcobucci Yes it looks like the test case is not correct. The original code in ManyToManyPersister#getDeleteSQLParameters is :

            $params[] = isset($sourceClass->fieldNames[$refColumnName])
                ? $identifier[$sourceClass->fieldNames[$refColumnName]]
                : $identifier[$sourceClass->getFieldForColumn($columnName)];

When I stumble upon this issue, isset($sourceClass->fieldNames[$refColumnName]) evaluated to false which caused the subsequent call to $sourceClass->getFieldForColumn($columnName)

which cannot work as $columnName is the column in the join table, not in the entity table.

I will investigate further and update the unit test to make it useful.

NicolaF · 2018-01-23T10:10:56Z

Test case updated. To actually reproduce the issue, we need :

multiple join columns
at least one join column where:
- the join column and the referenced column have different names
- the referenced column is a foreign key.

In this case:

the $referencedColumn is not in $sourceClass->fieldNames
ManyToManyPersister#getDeleteSQLParameters uses getFieldForColumn which was originally called with $columnName instead of $refColumnName
which throws if $columnName !== $refColumnName

lcobucci · 2018-01-23T10:40:30Z

@NicolaF I will check it in a bit, can you please execute phpcbf on the files you created to ensure they follow our coding standard?

NicolaF · 2018-01-23T16:20:08Z

@lcobucci done

Ocramius · 2018-01-26T10:25:49Z

@NicolaF I think this is good to go. @lcobucci can you give it another check? This would need merging to 2.6.1, 2.7 and master

…has a composite PK

lcobucci · 2018-01-30T00:19:00Z

@NicolaF I think this is good to go. @lcobucci can you give it another check? This would need merging to 2.6.1, 2.7 and master

@Ocramius I've made some minor changes to simplify the SUT and organise the annotations. It LGTM now. The build is failing for nightly PHP and mysql/pgsql but I think this is fine.

lcobucci · 2018-01-30T00:22:11Z

@NicolaF 🚢 thanks!

lcobucci · 2018-01-30T00:58:49Z

@NicolaF the good thing is that your test already passes in the current state of master, but I've added to the suite to ensure we won't have any regression (via 7928231).

NicolaF · 2018-01-30T09:45:14Z

You're welcome!

Fix doctrine#6991: correctly resolve identifer values in ManyToManyPe…

333b9c0

…rsister

NicolaF mentioned this pull request Jan 19, 2018

ManyToManyPersister fails to remove join table entry if there is multiple join columns #6991

Closed

lcobucci added this to the 2.6.1 milestone Jan 20, 2018

lcobucci self-requested a review January 20, 2018 10:33

lcobucci added the Bug label Jan 20, 2018

Ocramius suggested changes Jan 22, 2018

View reviewed changes

lcobucci suggested changes Jan 23, 2018

View reviewed changes

lcobucci self-requested a review January 23, 2018 11:00

NicolaF mentioned this pull request Jan 25, 2018

Fix #6991: correctly resolve identifer values in ManyToManyPersister #6992

Closed

Ocramius assigned lcobucci Jan 26, 2018

Add test case for many-to-many collection deletion, when owning side …

40f2a3e

…has a composite PK

lcobucci force-pushed the fix/fix-6991-2.6 branch from cdd3830 to 40f2a3e Compare January 30, 2018 00:04

lcobucci approved these changes Jan 30, 2018

View reviewed changes

lcobucci merged commit c2f698e into doctrine:2.6 Jan 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ManyToManyPersister fails to remove join table entry if there is multiple join columns #6997

ManyToManyPersister fails to remove join table entry if there is multiple join columns #6997

NicolaF commented Jan 19, 2018 •

edited

Loading

Ocramius left a comment

Ocramius Jan 22, 2018

Ocramius Jan 22, 2018

Ocramius Jan 22, 2018

Ocramius Jan 22, 2018

Ocramius Jan 22, 2018

Ocramius Jan 22, 2018

Ocramius Jan 22, 2018

Ocramius Jan 22, 2018

Ocramius Jan 22, 2018

Ocramius Jan 22, 2018

NicolaF commented Jan 22, 2018

lcobucci left a comment

NicolaF commented Jan 23, 2018 •

edited

Loading

NicolaF commented Jan 23, 2018 •

edited

Loading

lcobucci commented Jan 23, 2018

NicolaF commented Jan 23, 2018

Ocramius commented Jan 26, 2018

lcobucci commented Jan 30, 2018

lcobucci commented Jan 30, 2018

lcobucci commented Jan 30, 2018

NicolaF commented Jan 30, 2018

ManyToManyPersister fails to remove join table entry if there is multiple join columns #6997

ManyToManyPersister fails to remove join table entry if there is multiple join columns #6997

Conversation

NicolaF commented Jan 19, 2018 • edited Loading

Ocramius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolaF commented Jan 22, 2018

lcobucci left a comment

Choose a reason for hiding this comment

NicolaF commented Jan 23, 2018 • edited Loading

NicolaF commented Jan 23, 2018 • edited Loading

lcobucci commented Jan 23, 2018

NicolaF commented Jan 23, 2018

Ocramius commented Jan 26, 2018

lcobucci commented Jan 30, 2018

lcobucci commented Jan 30, 2018

lcobucci commented Jan 30, 2018

NicolaF commented Jan 30, 2018

NicolaF commented Jan 19, 2018 •

edited

Loading

NicolaF commented Jan 23, 2018 •

edited

Loading

NicolaF commented Jan 23, 2018 •

edited

Loading