From 12b5e79ff2d99ea6dfe8b23a77ae515c741c324a Mon Sep 17 00:00:00 2001 From: Carl Vuorinen Date: Fri, 2 Sep 2016 10:31:26 +0300 Subject: [PATCH 1/5] Create a failing test for issue #5989 Field with type=simple_array in a joined inheritance gets overridden by empty array in the hydrator --- .../Models/Issue5989/Issue5989Employee.php | 27 ++++++++++ .../Models/Issue5989/Issue5989Manager.php | 27 ++++++++++ .../Models/Issue5989/Issue5989Person.php | 29 +++++++++++ .../ORM/Functional/Ticket/Issue5989Test.php | 51 +++++++++++++++++++ .../Doctrine/Tests/OrmFunctionalTestCase.php | 11 ++++ 5 files changed, 145 insertions(+) create mode 100644 tests/Doctrine/Tests/Models/Issue5989/Issue5989Employee.php create mode 100644 tests/Doctrine/Tests/Models/Issue5989/Issue5989Manager.php create mode 100644 tests/Doctrine/Tests/Models/Issue5989/Issue5989Person.php create mode 100644 tests/Doctrine/Tests/ORM/Functional/Ticket/Issue5989Test.php diff --git a/tests/Doctrine/Tests/Models/Issue5989/Issue5989Employee.php b/tests/Doctrine/Tests/Models/Issue5989/Issue5989Employee.php new file mode 100644 index 000000000..e3eaac7d8 --- /dev/null +++ b/tests/Doctrine/Tests/Models/Issue5989/Issue5989Employee.php @@ -0,0 +1,27 @@ +tags; + } + + public function setTags(array $tags) + { + $this->tags = $tags; + } +} diff --git a/tests/Doctrine/Tests/Models/Issue5989/Issue5989Manager.php b/tests/Doctrine/Tests/Models/Issue5989/Issue5989Manager.php new file mode 100644 index 000000000..32c053984 --- /dev/null +++ b/tests/Doctrine/Tests/Models/Issue5989/Issue5989Manager.php @@ -0,0 +1,27 @@ +tags; + } + + public function setTags(array $tags) + { + $this->tags = $tags; + } +} diff --git a/tests/Doctrine/Tests/Models/Issue5989/Issue5989Person.php b/tests/Doctrine/Tests/Models/Issue5989/Issue5989Person.php new file mode 100644 index 000000000..c9e00c4c1 --- /dev/null +++ b/tests/Doctrine/Tests/Models/Issue5989/Issue5989Person.php @@ -0,0 +1,29 @@ +id; + } +} diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/Issue5989Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/Issue5989Test.php new file mode 100644 index 000000000..29a5a3fa2 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/Issue5989Test.php @@ -0,0 +1,51 @@ +useModelSet('issue5989'); + parent::setUp(); + } + + public function testSimpleArrayTypeHydratedCorrectlyInJoinedInheritance() + { + $manager = new Issue5989Manager(); + + $managerTags = array('tag1', 'tag2'); + $manager->setTags($managerTags); + $this->_em->persist($manager); + + $employee = new Issue5989Employee(); + + $employeeTags = array('tag2', 'tag3'); + $employee->setTags($employeeTags); + $this->_em->persist($employee); + + $this->_em->flush(); + + $managerId = $manager->getId(); + $employeeId = $employee->getId(); + + // clear entity manager so that $repository->find actually fetches them and uses the hydrator + // instead of just returning the existing managed entities + $this->_em->clear(); + + $repository = $this->_em->getRepository(Issue5989Person::class); + + $manager = $repository->find($managerId); + $employee = $repository->find($employeeId); + + static::assertEquals($managerTags, $manager->getTags()); + static::assertEquals($employeeTags, $employee->getTags()); + } +} diff --git a/tests/Doctrine/Tests/OrmFunctionalTestCase.php b/tests/Doctrine/Tests/OrmFunctionalTestCase.php index 907c00617..a6a3bece4 100644 --- a/tests/Doctrine/Tests/OrmFunctionalTestCase.php +++ b/tests/Doctrine/Tests/OrmFunctionalTestCase.php @@ -295,6 +295,11 @@ abstract class OrmFunctionalTestCase extends OrmTestCase 'Doctrine\Tests\Models\VersionedManyToOne\Category', 'Doctrine\Tests\Models\VersionedManyToOne\Article', ), + 'issue5989' => array( + 'Doctrine\Tests\Models\Issue5989\Issue5989Person', + 'Doctrine\Tests\Models\Issue5989\Issue5989Employee', + 'Doctrine\Tests\Models\Issue5989\Issue5989Manager', + ), ); /** @@ -563,6 +568,12 @@ abstract class OrmFunctionalTestCase extends OrmTestCase $conn->executeUpdate('DELETE FROM versioned_many_to_one_category'); } + if (isset($this->_usedModelSets['issue5989'])) { + $conn->executeUpdate('DELETE FROM issue5989_persons'); + $conn->executeUpdate('DELETE FROM issue5989_employees'); + $conn->executeUpdate('DELETE FROM issue5989_managers'); + } + $this->_em->clear(); } From 95546d68c5eec4576b0d5dcb741175ee39692333 Mon Sep 17 00:00:00 2001 From: Carl Vuorinen Date: Sat, 3 Sep 2016 22:25:11 +0300 Subject: [PATCH 2/5] Fix hydration in a joined inheritance with simple array or json array SimpleArrayType and JsonArrayType convert NULL value to an empty array, which fails the null check that is used to prevent overwrite Fixes issue #5989 --- lib/Doctrine/ORM/Internal/Hydration/SimpleObjectHydrator.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/Internal/Hydration/SimpleObjectHydrator.php b/lib/Doctrine/ORM/Internal/Hydration/SimpleObjectHydrator.php index 92c77ddd3..798bd305a 100644 --- a/lib/Doctrine/ORM/Internal/Hydration/SimpleObjectHydrator.php +++ b/lib/Doctrine/ORM/Internal/Hydration/SimpleObjectHydrator.php @@ -121,6 +121,9 @@ class SimpleObjectHydrator extends AbstractHydrator continue; } + // Check if value is null before conversion (because some types convert null to something else) + $valueIsNull = $value === null; + // Convert field to a valid PHP value if (isset($cacheKeyInfo['type'])) { $type = $cacheKeyInfo['type']; @@ -130,7 +133,7 @@ class SimpleObjectHydrator extends AbstractHydrator $fieldName = $cacheKeyInfo['fieldName']; // Prevent overwrite in case of inherit classes using same property name (See AbstractHydrator) - if ( ! isset($data[$fieldName]) || $value !== null) { + if ( ! isset($data[$fieldName]) || ! $valueIsNull) { $data[$fieldName] = $value; } } From 00bbf4f52351280791fbc59761533d296b59861b Mon Sep 17 00:00:00 2001 From: Carl Vuorinen Date: Sun, 4 Sep 2016 18:37:03 +0300 Subject: [PATCH 3/5] Use yoda condition in the null check --- lib/Doctrine/ORM/Internal/Hydration/SimpleObjectHydrator.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/Internal/Hydration/SimpleObjectHydrator.php b/lib/Doctrine/ORM/Internal/Hydration/SimpleObjectHydrator.php index 798bd305a..785a2b796 100644 --- a/lib/Doctrine/ORM/Internal/Hydration/SimpleObjectHydrator.php +++ b/lib/Doctrine/ORM/Internal/Hydration/SimpleObjectHydrator.php @@ -122,7 +122,7 @@ class SimpleObjectHydrator extends AbstractHydrator } // Check if value is null before conversion (because some types convert null to something else) - $valueIsNull = $value === null; + $valueIsNull = null === $value; // Convert field to a valid PHP value if (isset($cacheKeyInfo['type'])) { From d2cbd5e872ac637474846c79bed7df9738343ffa Mon Sep 17 00:00:00 2001 From: Carl Vuorinen Date: Thu, 8 Sep 2016 11:48:34 +0300 Subject: [PATCH 4/5] PR fixes (public properties & correct letter case in annotations) --- .../Tests/Models/Issue5989/Issue5989Employee.php | 14 ++------------ .../Tests/Models/Issue5989/Issue5989Manager.php | 14 ++------------ .../Tests/Models/Issue5989/Issue5989Person.php | 7 +------ .../ORM/Functional/Ticket/Issue5989Test.php | 16 ++++++++-------- 4 files changed, 13 insertions(+), 38 deletions(-) diff --git a/tests/Doctrine/Tests/Models/Issue5989/Issue5989Employee.php b/tests/Doctrine/Tests/Models/Issue5989/Issue5989Employee.php index e3eaac7d8..9f5e248d8 100644 --- a/tests/Doctrine/Tests/Models/Issue5989/Issue5989Employee.php +++ b/tests/Doctrine/Tests/Models/Issue5989/Issue5989Employee.php @@ -9,19 +9,9 @@ namespace Doctrine\Tests\Models\Issue5989; class Issue5989Employee extends Issue5989Person { /** - * @column(type="simple_array", nullable=true) + * @Column(type="simple_array", nullable=true) * * @var array */ - private $tags; - - public function getTags() - { - return $this->tags; - } - - public function setTags(array $tags) - { - $this->tags = $tags; - } + public $tags; } diff --git a/tests/Doctrine/Tests/Models/Issue5989/Issue5989Manager.php b/tests/Doctrine/Tests/Models/Issue5989/Issue5989Manager.php index 32c053984..e00067d9c 100644 --- a/tests/Doctrine/Tests/Models/Issue5989/Issue5989Manager.php +++ b/tests/Doctrine/Tests/Models/Issue5989/Issue5989Manager.php @@ -9,19 +9,9 @@ namespace Doctrine\Tests\Models\Issue5989; class Issue5989Manager extends Issue5989Person { /** - * @column(type="simple_array", nullable=true) + * @Column(type="simple_array", nullable=true) * * @var array */ - private $tags; - - public function getTags() - { - return $this->tags; - } - - public function setTags(array $tags) - { - $this->tags = $tags; - } + public $tags; } diff --git a/tests/Doctrine/Tests/Models/Issue5989/Issue5989Person.php b/tests/Doctrine/Tests/Models/Issue5989/Issue5989Person.php index c9e00c4c1..d31e65def 100644 --- a/tests/Doctrine/Tests/Models/Issue5989/Issue5989Person.php +++ b/tests/Doctrine/Tests/Models/Issue5989/Issue5989Person.php @@ -20,10 +20,5 @@ class Issue5989Person * @Column(type="integer") * @GeneratedValue */ - private $id; - - public function getId() - { - return $this->id; - } + public $id; } diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/Issue5989Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/Issue5989Test.php index 29a5a3fa2..cf601b99a 100644 --- a/tests/Doctrine/Tests/ORM/Functional/Ticket/Issue5989Test.php +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/Issue5989Test.php @@ -21,20 +21,20 @@ class Issue5989Test extends \Doctrine\Tests\OrmFunctionalTestCase { $manager = new Issue5989Manager(); - $managerTags = array('tag1', 'tag2'); - $manager->setTags($managerTags); + $managerTags = ['tag1', 'tag2']; + $manager->tags = $managerTags; $this->_em->persist($manager); $employee = new Issue5989Employee(); - $employeeTags = array('tag2', 'tag3'); - $employee->setTags($employeeTags); + $employeeTags =['tag2', 'tag3']; + $employee->tags = $employeeTags; $this->_em->persist($employee); $this->_em->flush(); - $managerId = $manager->getId(); - $employeeId = $employee->getId(); + $managerId = $manager->id; + $employeeId = $employee->id; // clear entity manager so that $repository->find actually fetches them and uses the hydrator // instead of just returning the existing managed entities @@ -45,7 +45,7 @@ class Issue5989Test extends \Doctrine\Tests\OrmFunctionalTestCase $manager = $repository->find($managerId); $employee = $repository->find($employeeId); - static::assertEquals($managerTags, $manager->getTags()); - static::assertEquals($employeeTags, $employee->getTags()); + static::assertEquals($managerTags, $manager->tags); + static::assertEquals($employeeTags, $employee->tags); } } From 43d22984ae56facfba103a5d8e2361ba75fac8ca Mon Sep 17 00:00:00 2001 From: Carl Vuorinen Date: Thu, 8 Sep 2016 13:50:28 +0300 Subject: [PATCH 5/5] Add unit test for SimpleObjectHydrator --- .../Hydration/SimpleObjectHydratorTest.php | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/Doctrine/Tests/ORM/Hydration/SimpleObjectHydratorTest.php b/tests/Doctrine/Tests/ORM/Hydration/SimpleObjectHydratorTest.php index eb33c23c2..d56718834 100644 --- a/tests/Doctrine/Tests/ORM/Hydration/SimpleObjectHydratorTest.php +++ b/tests/Doctrine/Tests/ORM/Hydration/SimpleObjectHydratorTest.php @@ -86,4 +86,34 @@ class SimpleObjectHydratorTest extends HydrationTestCase $hydrator = new \Doctrine\ORM\Internal\Hydration\SimpleObjectHydrator($this->_em); $hydrator->hydrateAll($stmt, $rsm); } + + /** + * @group issue-5989 + */ + public function testNullValueShouldNotOverwriteFieldWithSameNameInJoinedInheritance() + { + $rsm = new ResultSetMapping; + $rsm->addEntityResult('Doctrine\Tests\Models\Issue5989\Issue5989Person', 'p'); + $rsm->addFieldResult('p', 'p__id', 'id'); + $rsm->addFieldResult('p', 'm__tags', 'tags', 'Doctrine\Tests\Models\Issue5989\Issue5989Manager'); + $rsm->addFieldResult('p', 'e__tags', 'tags', 'Doctrine\Tests\Models\Issue5989\Issue5989Employee'); + $rsm->addMetaResult('p', 'discr', 'discr', false, 'string'); + $resultSet = array( + array( + 'p__id' => '1', + 'm__tags' => 'tag1,tag2', + 'e__tags' => null, + 'discr' => 'manager' + ), + ); + + $expectedEntity = new \Doctrine\Tests\Models\Issue5989\Issue5989Manager(); + $expectedEntity->id = 1; + $expectedEntity->tags = ['tag1', 'tag2']; + + $stmt = new HydratorMockStatement($resultSet); + $hydrator = new \Doctrine\ORM\Internal\Hydration\SimpleObjectHydrator($this->_em); + $result = $hydrator->hydrateAll($stmt, $rsm); + $this->assertEquals($result[0], $expectedEntity); + } }