From dce0aeaa15582970773849e0e739004890a9a199 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 b36a83487..611f357e8 100644 --- a/tests/Doctrine/Tests/OrmFunctionalTestCase.php +++ b/tests/Doctrine/Tests/OrmFunctionalTestCase.php @@ -284,6 +284,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', + ), ); /** @@ -544,6 +549,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 7352b97b1457a9460911c1d333692078d47f9a5a 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 1c21369e3..5eb8cc117 100644 --- a/lib/Doctrine/ORM/Internal/Hydration/SimpleObjectHydrator.php +++ b/lib/Doctrine/ORM/Internal/Hydration/SimpleObjectHydrator.php @@ -122,6 +122,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']; @@ -131,7 +134,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 c47c23a101a6acebf63156c3d70917c49fd8905a 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 5eb8cc117..5fb77d67e 100644 --- a/lib/Doctrine/ORM/Internal/Hydration/SimpleObjectHydrator.php +++ b/lib/Doctrine/ORM/Internal/Hydration/SimpleObjectHydrator.php @@ -123,7 +123,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 33e23cdddb6366d3daa2d941db58d16f72818224 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 da41161d73e5fca87b3659219eda56026d95a515 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 459ce9ba1..9878728f2 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); + } }