Fix for #6393 - use field names when extracting identifier fields #6394

tobiasstadler · 2017-04-12T10:17:59Z

lcobucci

Also we need tests 😄

lcobucci · 2017-04-12T10:19:12Z

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

        $columnName   = $this->quoteStrategy->getColumnName($versionField, $versionedClass, $this->platform);
+
+		$where = array();


Please use short array syntax

lcobucci · 2017-04-12T10:19:17Z

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

        $columnName   = $this->quoteStrategy->getColumnName($versionField, $versionedClass, $this->platform);
+
+		$where = array();
+        $params = array();


Please use short array syntax

lcobucci · 2017-04-12T10:20:44Z

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

+
+		$where = array();
+        $params = array();
+        foreach ($versionedClass->identifier as $idField) {


Can you extract a method from this code?

phansys

Status: Needs work.

phansys · 2017-04-14T19:57:43Z

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

+    /**
+     * Determains the identifier columns and values for an entity
+     *
+     * @param \Doctrine\ORM\Mapping\ClassMetadata $class


This class is already imported, so please use ClassMetadata instead FQCN.

phansys · 2017-04-14T19:58:18Z

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

+     * @param array                               $id
+     * @return array
+     */
+    private function determineIdentifierColumnsAndValues($class, array $id)


Could you please declare the type for $class argument?

phansys · 2017-04-14T19:59:11Z

tests/Doctrine/Tests/ORM/Functional/Ticket/DDC6393Test.php

+        $this->_em->persist($b);        
+        $this->_em->flush();
+
+        $this->assertEquals(1, $b->getVersion());


Please, use assertSame() instead.

tobiasstadler · 2017-05-25T17:50:03Z

Any chance for getting this in?

Ocramius · 2017-06-01T12:38:53Z

I think this looks fine, but I don't have my laptop atm. Will try merging later today 👍

Ocramius

I went through the patch again and tried applying it locally. There are some major issues that need fixing:

only the first association column is considered, whereas the IdentifierFlattener considered all columns
the association column is not being quoted
overall, the logic in determineIdentifierColumnsAndValues is an identifier lookup, and it should produce a [ $values, $columns, $types] triplet
The code in determineIdentifierColumnsAndValues should be abstracted away, or an existing abstraction should be found. The IdentifierFlattener is close to it, but it was designed for the identity map internals, not for column operations.

tobiasstadler · 2017-10-04T20:58:14Z

Hi I've tried a different (and hopefully better) approach for solving #6394

lcobucci · 2017-11-26T17:02:56Z

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

+            $identifier = $this->em->getUnitOfWork()->getEntityIdentifier($entity);
+
+            $id = [];
+            foreach ($this->class->identifier as $idField) {


This seems like a workaround to me... as @Ocramius said we should rather use field names everywhere.

This doesn't seem possible, because in UnitOfWork::createEntity it passes the whole $row ($data) into the identifier flattener and the hydration works by passing the association data as columns not as fields, because it must support composite primary keys as assocations where using field name as a single name would not be able to represent two ore more columns.

ACtually the other way around, BasicEntityPersister::update used the wrong identifier representation. See #8384

lcobucci · 2017-11-26T17:03:45Z

tests/Doctrine/Tests/ORM/Functional/Ticket/DDC6393Test.php

+class DDC6393Test extends \Doctrine\Tests\OrmFunctionalTestCase
+{
+
+    protected function setUp()


: void missing

lcobucci · 2017-11-26T17:03:50Z

tests/Doctrine/Tests/ORM/Functional/Ticket/DDC6393Test.php

+     * Test the the version of an entity can be fetched, when the id field and
+     * the id column are different.
+     */
+    public function testFetchVersionValueForDifferentIdFieldAndColumn()


: void missing

lcobucci · 2017-11-26T17:04:05Z

tests/Doctrine/Tests/ORM/Functional/Ticket/DDC6393Test.php

+    /**
+     * Test the the version of an entity can be fetched, when the id field and
+     * the id column are different.
+     */


Please add @group 6393

lcobucci · 2017-11-26T17:04:26Z

tests/Doctrine/Tests/ORM/Functional/Ticket/DDC6393Test.php

+        $this->_em->persist($b);        
+        $this->_em->flush();
+
+        $this->assertSame(1, $b->getVersion());


self::assert* is more appropriated since the method is static

lcobucci · 2017-11-26T17:06:13Z

tests/Doctrine/Tests/ORM/Functional/Ticket/DDC6393Test.php

+     */
+    private $version;
+
+    public function __construct($a, $something)


public function __construct(A $a, string $something)

lcobucci · 2017-11-26T17:06:19Z

tests/Doctrine/Tests/ORM/Functional/Ticket/DDC6393Test.php

+        $this->something = $something;
+    }
+
+    public function getA()


Please this method and make the property public, so that we simplify the entity a bit

lcobucci · 2017-11-26T17:06:24Z

tests/Doctrine/Tests/ORM/Functional/Ticket/DDC6393Test.php

+        return $this->a;
+    }
+
+    public function getSomething()


Please this method and make the property public, so that we simplify the entity a bit

lcobucci · 2017-11-26T17:06:28Z

tests/Doctrine/Tests/ORM/Functional/Ticket/DDC6393Test.php

+        $this->something = $something;
+    }
+
+    public function getVersion()


Please this method and make the property public, so that we simplify the entity a bit

lcobucci · 2017-11-26T17:06:45Z

tests/Doctrine/Tests/ORM/Functional/Ticket/DDC6393Test.php

+        return $this->something;
+    }
+
+    public function setSomething($something)


Please this method and simply change the property on the test

… non-object values

…ntifiers for version assignment.

…ect values. (#8384) * [GH-6394] Bugfix: IdentifierFlattener support for association non-object values * [GH-6394] Bugfix: BasicEntityPersister::update used wrong identifiers for version assignment. * Exclude MissingNativeTypeHint phpcs rule as 7.4 is not lowest version.

Fix for #6393

6f5475f

lcobucci suggested changes Apr 12, 2017

View reviewed changes

lcobucci added the Missing Tests label Apr 12, 2017

tobiasstadler added 3 commits April 12, 2017 13:34

Fix for #6393

cfb7b9e

Incorporated review comments

6dd37d9

Added test for #6393

ed14b73

phansys suggested changes Apr 14, 2017

View reviewed changes

Incorporated review comments

2b58b73

Ocramius added Bug Requires merge/review and removed Missing Tests labels Jun 1, 2017

Ocramius added this to the 2.6.0 milestone Jun 1, 2017

Ocramius self-assigned this Jun 1, 2017

Ocramius removed their assignment Jun 21, 2017

Ocramius added Missing Tests and removed Missing Tests labels Jun 21, 2017

Ocramius changed the title ~~Fix for #6393~~ Fix for #6393 - use field names when extracting identifier fields Jun 21, 2017

Ocramius self-assigned this Jun 21, 2017

Ocramius suggested changes Jun 21, 2017

View reviewed changes

Ocramius removed their assignment Jun 21, 2017

Ocramius added Missing Tests and removed Requires merge/review labels Jun 21, 2017

Ocramius removed this from the 2.6.0 milestone Jun 21, 2017

tobiasstadler added 2 commits October 4, 2017 22:39

Anaother approach for solving #6393

64c607b

Merge remote-tracking branch 'upstream/master'

4a546f5

If schema creation fails, running the test won't work

6f4861e

lcobucci suggested changes Nov 26, 2017

View reviewed changes

Incorporated changes requested by @lcobucci

1fbca4f

Majkl578 removed the Missing Tests label Dec 26, 2017

beberlei self-assigned this Dec 8, 2020

beberlei added a commit to beberlei/doctrine2 that referenced this pull request Dec 8, 2020

[doctrineGH-6394] Bugfix: IdentifierFlattener support for association…

66a95ea

… non-object values

beberlei added a commit to beberlei/doctrine2 that referenced this pull request Dec 8, 2020

[doctrineGH-6394] Bugfix: BasicEntityPersister::update used wrong ide…

59dadf6

…ntifiers for version assignment.

beberlei mentioned this pull request Dec 8, 2020

[GH-6394] Bugfix: IdentifierFlattener support for association non-object values. #8384

Merged

beberlei added a commit to beberlei/doctrine2 that referenced this pull request Dec 8, 2020

[doctrineGH-6394] Bugfix: BasicEntityPersister::update used wrong ide…

b621e77

…ntifiers for version assignment.

greg0ire closed this Feb 23, 2021

greg0ire deleted the branch doctrine:master February 23, 2021 08:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for #6393 - use field names when extracting identifier fields #6394

Fix for #6393 - use field names when extracting identifier fields #6394

tobiasstadler commented Apr 12, 2017 •

edited by Ocramius

lcobucci left a comment

lcobucci Apr 12, 2017

lcobucci Apr 12, 2017

lcobucci Apr 12, 2017

phansys left a comment

phansys Apr 14, 2017

phansys Apr 14, 2017

phansys Apr 14, 2017

tobiasstadler commented May 25, 2017

Ocramius commented Jun 1, 2017

Ocramius left a comment •

edited

tobiasstadler commented Oct 4, 2017

lcobucci Nov 26, 2017 •

edited

beberlei Dec 8, 2020

beberlei Dec 8, 2020

lcobucci Nov 26, 2017

lcobucci Nov 26, 2017

lcobucci Nov 26, 2017

lcobucci Nov 26, 2017

lcobucci Nov 26, 2017

lcobucci Nov 26, 2017

lcobucci Nov 26, 2017

lcobucci Nov 26, 2017

lcobucci Nov 26, 2017

		$columnName = $this->quoteStrategy->getColumnName($versionField, $versionedClass, $this->platform);

		$where = array();

Fix for #6393 - use field names when extracting identifier fields #6394

Fix for #6393 - use field names when extracting identifier fields #6394

Conversation

tobiasstadler commented Apr 12, 2017 • edited by Ocramius

lcobucci 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

phansys 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

tobiasstadler commented May 25, 2017

Ocramius commented Jun 1, 2017

Ocramius left a comment • edited

Choose a reason for hiding this comment

tobiasstadler commented Oct 4, 2017

lcobucci Nov 26, 2017 • edited

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

Choose a reason for hiding this comment

tobiasstadler commented Apr 12, 2017 •

edited by Ocramius

Ocramius left a comment •

edited

lcobucci Nov 26, 2017 •

edited