Check if the discriminator value is `null` before converting it and setting into `rowData` during hydration #6304

fullbl · 2017-02-24T18:30:47Z

Ocramius · 2017-02-24T20:03:47Z

Needs tests

On 24 Feb 2017 7:30 p.m., "fullbl" ***@***.***> wrote: fixes #6303 <#6303> ------------------------------ You can view, comment on, or merge this pull request online at: #6304 Commit Summary - check if value is null before converting it and setting into rowData File Changes - *M* lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php <https://github.com/doctrine/doctrine2/pull/6304/files#diff-0> (8) Patch Links: - https://github.com/doctrine/doctrine2/pull/6304.patch - https://github.com/doctrine/doctrine2/pull/6304.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#6304>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakEit5cOVeTTHSLfvACICMkc5yCJBks5rfyHYgaJpZM4MLh1s> .

fullbl · 2017-02-25T19:06:32Z

Tests added, but seems that my edit broke a lot of other tests...

Checking for non empty values instead of isset seems to fix things, but this solution is dipendent on the default value of SimpleArray.
I really believe that value assignation should be made in 2 steps, one for checking null values and another one for assigning the default in case of null!

Ocramius · 2017-02-25T21:36:03Z

tests/Doctrine/Tests/Models/Issue5989/Issue5989Person.php

@@ -10,6 +10,7 @@
 * @DiscriminatorMap({
 *      "person"    = "Issue5989Person",
 *      "manager"   = "Issue5989Manager",
+ *      "secretary"  = "Issue5989Secretary",


Please use a different entity and a new inheritance tree to test your specific scenario

Ocramius · 2017-02-25T21:52:10Z

lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php

@@ -290,10 +290,10 @@ protected function gatherRowData(array $data, array &$id, array &$nonemptyCompon
                    // in an inheritance hierarchy the same field could be defined several times.
                    // We overwrite this value so long we don't have a non-null value, that value we keep.
                    // Per definition it cannot be that a field is defined several times and has several values.
-                    if (isset($rowData['data'][$dqlAlias][$fieldName])) {
+                    if (!empty($rowData['data'][$dqlAlias][$fieldName])) {


Comment above explicitly references "non-null" values

Ocramius · 2017-02-25T22:11:28Z

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

+        $secretary = new Issue5989Secretary();
+
+        $secretaryTags = 'single_tag';
+        $secretary->tags = $secretaryTags;


The fact that you use a different type for a property in the inheritance tree is quite confusing: is this intentional?

Ocramius · 2017-02-25T22:12:40Z

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

+
+        static::assertEquals($tags[$people[0]->id], $people[0]->tags);
+        static::assertEquals($tags[$people[1]->id], $people[1]->tags);
+        static::assertEquals($tags[$people[2]->id], $people[2]->tags);


Which of these assertions would fail without your patch in place?

Ocramius · 2017-02-25T22:14:38Z

lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php

@@ -290,10 +290,10 @@ protected function gatherRowData(array $data, array &$id, array &$nonemptyCompon
                    // in an inheritance hierarchy the same field could be defined several times.
                    // We overwrite this value so long we don't have a non-null value, that value we keep.
                    // Per definition it cannot be that a field is defined several times and has several values.


This particular definition is what is being challenged by your inheritance design. The field definition should be fetched per entity name, and $fieldName should be specific to the currently hydrated entity type instead of just the name of a field. Did you try step-debugging here? What is $type?

fullbl · 2017-02-28T19:05:59Z

Hi, I will create new entities as soon as possible, I thought it would be good to reuse the old ones!

$type is StringType for string and SimpleArrayType for simple array, so is different based on the field.

The problem is exactly as you stated in

The fact that you use a different type for a property in the inheritance tree is quite confusing: is this intentional?

This is what is breaking stuff.

The failing assertion is the one for the string field. It's hydrated by the simple_array as an empty array and missing the real value.

To be honest, I'm not satisfied with my solution, I will think about it in next days!

fullbl · 2017-03-04T14:15:48Z

Hi again, I fixed the tests to use their own inheritance, but I removed my fix, because I would like to add more tests for checking that setting some property to false, 0 or empty array should return correct values!

fullbl · 2017-03-04T16:56:15Z

Here we are, I added those tests and decided to redo the fix in another way, keeping only values from the correct table!

lcobucci

Can you also fix the coding style to match with rest of the project, please?

lcobucci · 2017-03-05T08:25:19Z

tests/Doctrine/Tests/Models/DDC6303/DDC6303Contract.php

+ *      "contract_a"  = "DDC6303ContractA"
+ * })
+ */
+class DDC6303Contract


Please move this class together with the test class (same file)

lcobucci · 2017-03-05T08:25:23Z

tests/Doctrine/Tests/Models/DDC6303/DDC6303ContractA.php

+ * @Entity
+ * @Table(name="ddc6303_contracts_a")
+ */
+class DDC6303ContractA extends DDC6303Contract


Please move this class together with the test class (same file)

lcobucci · 2017-03-05T08:25:27Z

tests/Doctrine/Tests/Models/DDC6303/DDC6303ContractB.php

+ * @Entity
+ * @Table(name="ddc6303_contracts_b")
+ */
+class DDC6303ContractB extends DDC6303Contract


Please move this class together with the test class (same file)

lcobucci · 2017-03-05T08:25:59Z

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

+{
+    public function setUp()
+    {
+        $this->useModelSet('ddc6303');


Initialise the schema here instead of using a model set

lcobucci · 2017-03-05T08:26:43Z

tests/Doctrine/Tests/OrmFunctionalTestCase.php

@@ -302,6 +302,12 @@
            Models\Issue5989\Issue5989Employee::class,
            Models\Issue5989\Issue5989Manager::class,
        ],
+        'ddc6303' => [


Not needed when models are in the test file

lcobucci · 2017-03-05T08:28:11Z

lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php

                    // in an inheritance hierarchy the same field could be defined several times.
                    // We overwrite this value so long we don't have a non-null value, that value we keep.
                    // Per definition it cannot be that a field is defined several times and has several values.
                    if (isset($rowData['data'][$dqlAlias][$fieldName])) {
                        break;
                    }
-
+                    


Please remove the trailing spaces

lcobucci · 2017-03-06T21:24:06Z

lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php

                ];
+                if( !empty($classMetadata->parentClasses && isset($this->_rsm->discriminatorColumns[$ownerMap]))){


Is this really the expression we're looking for? It looks like it should be:

if ( ! empty($classMetadata->parentClasses) && isset($this->_rsm->discriminatorColumns[$ownerMap])) { }

fullbl · 2017-03-11T19:57:42Z

Done, I made it first into different files because I copypasted from another issue, maybe it was old!

Ocramius

Patch looks good to me! The rest are nitpicks and improvements that can be done on merge.

Ocramius · 2017-03-12T03:42:00Z

lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php

+                    if(
+                        isset($cacheKeyInfo['discriminatorColumn']) && 
+                        isset($data[$cacheKeyInfo['discriminatorColumn']]) &&
+                        $data[$cacheKeyInfo['discriminatorColumn']] != $cacheKeyInfo['discriminatorValue']


This should be !== to avoid type juggling hell. Yes, that means that object discriminators will fail, that's an edge case that the ORM doesn't handle anyway.

it seems to me that this change is breaking another test (/code/tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheJoinTableInheritanceTest.php:179). Is it ok?

@fullbl it shouldn't break it... let's keep the strict comparison and try to find the root cause.

Ocramius · 2017-03-12T03:42:59Z

lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php

                ];
+                if( !empty($classMetadata->parentClasses) && isset($this->_rsm->discriminatorColumns[$ownerMap])){
+                    $returnArray += [


Consider making two return operations instead of using +=

Ocramius · 2017-03-12T03:44:06Z

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

+        $contractA->originalData = $contractAData;
+
+        $contractB = new DDC6303ContractB();
+        $contractBData = ['accepted', 'authorized'];


Could you add comments for the reader, specifying that these two entities map originalData differently?

…to skip-hydrate-on-null

Ocramius · 2017-06-21T04:53:13Z

@fullbl I rebased this branch on top of master to start merging it, but there's a failure in the test suite:

1) Doctrine\Tests\ORM\Functional\SecondLevelCacheJoinTableInheritanceTest::testOneToManyRelationJoinTable
Failed asserting that null matches expected '0000-0000'.

/home/ocramius/Documents/doctrine/doctrine2/tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheJoinTableInheritanceTest.php:179

FAILURES!
Tests: 3185, Assertions: 11878, Failures: 1, Skipped: 46, Incomplete: 2.

Ocramius · 2017-08-18T08:33:21Z

I checked the issue locally, and it's because the discriminator is not converted to a PHP value.

That said, trying to fix the issue leads to a massive can of worms that is being opened: let's keep the loose comparison there for now.

Things changed

…gnment)

… (which is bullshit), so we use `array` here

… important for this test

…/JTI require additional information and checks in the hydration process

Ocramius · 2017-08-19T16:44:57Z

Manually merged via a30d8d8

Thanks @fullbl! It took me a while to figure out, but basically, the bug depends on the discriminator map sorting.

A lot of weird factors involved, and indeed an extremely tricky bug to fix. Good work!

JanTvrdik · 2017-08-29T09:30:50Z

Maybe I'm doing sth wrong but this change seems to broke the following multi-level STI schema when selecting with QueryBuilder FROM C because $cacheKeyInfo['discriminatorValue'] is NULL and therefore $data[$cacheKeyInfo['discriminatorColumn']] != $cacheKeyInfo['discriminatorValue'] returns in TRUE.

- `em` protected property - annotation namespace change - `QueryBuilder` instantiation

…ated' Close #6988 Close #6304

…es in `3.x` - `em` protected property - annotation namespace change - `QueryBuilder` instantiation

Ocramius suggested changes Feb 25, 2017

View reviewed changes

fullbl closed this Mar 4, 2017

tests with custom inheritance tree

6a482cb

fullbl reopened this Mar 4, 2017

fullbl added 3 commits March 4, 2017 15:27

added tests on empty values

aae03be

use dataMap also in first test function

f8c0995

check if array is set before assigning

6a8d962

lcobucci previously requested changes Mar 6, 2017

View reviewed changes

fullbl added 3 commits March 11, 2017 20:24

fix wrong pharenthesis

a4be73c

moved all test models into test class

6c3289d

removed trailing spaces

aa3783c

Ocramius approved these changes Mar 12, 2017

View reviewed changes

Ocramius added Bug Requires merge/review labels Mar 12, 2017

Ocramius added this to the 2.5.7 milestone Mar 12, 2017

fullbl and others added 3 commits March 19, 2017 15:05

clarified what's the problem in a comment

d9003fc

comparison on discriminator value with !==

19a7e49

Merge branch 'skip-hydrate-on-null' of github.com:fullbl/doctrine2 in…

1122614

…to skip-hydrate-on-null

Ocramius self-assigned this Jun 21, 2017

Ocramius removed the Requires merge/review label Jun 21, 2017

Ocramius changed the title ~~check if value is null before converting it and setting into rowData~~ Check if the discriminator value is null before converting it and setting into rowData during hydration Jun 21, 2017

Ocramius added the Missing Tests label Jun 21, 2017

Ocramius removed the Missing Tests label Aug 18, 2017

Ocramius assigned Ocramius and unassigned fullbl Aug 18, 2017

Ocramius added a commit that referenced this pull request Aug 19, 2017

#6303 #6304 documenting why the loose comparison is required

7e7921e

Ocramius added a commit that referenced this pull request Aug 19, 2017

#6303 #6304 removing += operator usage on arrays (scary\!), CS (ali…

c503b81

…gnment)

Ocramius added a commit that referenced this pull request Aug 19, 2017

#6303 #6304 reverting changes to the OrmFunctionalTestCase

aaad25a

Ocramius added a commit that referenced this pull request Aug 19, 2017

#6303 #6304 minor CS fixes in the test code - alignment/formatting

f6ce69f

Ocramius added a commit that referenced this pull request Aug 19, 2017

#6303 #6304 simplified tests, removing references to contract naming

328467c

Ocramius added a commit that referenced this pull request Aug 19, 2017

#6303 #6304 simplified tests, removing references to contract naming

0882b10

Ocramius added a commit that referenced this pull request Aug 19, 2017

#6303 #6304 adding void hints where applicable

2ab363a

Ocramius added a commit that referenced this pull request Aug 19, 2017

#6303 #6304 removing duplicate test details

e18fb66

Ocramius added a commit that referenced this pull request Aug 19, 2017

#6303 #6304 using strict types in the tests

468496b

Ocramius added a commit that referenced this pull request Aug 19, 2017

#6303 #6304 making unnecessary visible fields private

764ab59

Ocramius added a commit that referenced this pull request Aug 19, 2017

#6303 #6304 correcting type mapping. For simple_array, [] == null…

8af6861

… (which is bullshit), so we use `array` here

Ocramius added a commit that referenced this pull request Aug 19, 2017

#6303 #6304 re-enabling disabled test code data

42d9162

Ocramius added a commit that referenced this pull request Aug 19, 2017

#6303 #6304 removing useless union type usage

b66643d

Ocramius added a commit that referenced this pull request Aug 19, 2017

#6303 #6304 documenting that the discriminator map order is extremely…

4cbcdb7

… important for this test

Ocramius added a commit that referenced this pull request Aug 19, 2017

#6303 #6304 documenting why collisions in field name hydration in STI…

44f2e22

…/JTI require additional information and checks in the hydration process

Ocramius closed this in a30d8d8 Aug 19, 2017

Ocramius mentioned this pull request Aug 19, 2017

ObjectHydrator and ArrayHydrator incorrectly hydrate fields of child classes in a JTI when a field name collides #6303

Closed

fullbl deleted the skip-hydrate-on-null branch August 28, 2017 12:01

kbond mentioned this pull request Jan 15, 2018

Inheritance middle-layer doesn't get hydrated #6988

Merged

Ocramius added a commit that referenced this pull request Feb 19, 2018

#6988 #6304 adapted test case to ORM test suite changes in 3.x

e7e4247

- `em` protected property - annotation namespace change - `QueryBuilder` instantiation

Ocramius added a commit that referenced this pull request Feb 19, 2018

Merge branch 'fix/#6988-#6304-ensure-inheritance-middle-layer-is-hydr…

0822f8a

…ated' Close #6988 Close #6304

maglnet pushed a commit to maglnet/doctrine2 that referenced this pull request Oct 10, 2018

doctrine#6988 doctrine#6304 adapted test case to ORM test suite chang…

e885a21

…es in `3.x` - `em` protected property - annotation namespace change - `QueryBuilder` instantiation

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check if the discriminator value is `null` before converting it and setting into `rowData` during hydration #6304

Check if the discriminator value is `null` before converting it and setting into `rowData` during hydration #6304

fullbl commented Feb 24, 2017

Ocramius commented Feb 24, 2017 via email

fullbl commented Feb 25, 2017

Ocramius Feb 25, 2017

Ocramius Feb 25, 2017

Ocramius Feb 25, 2017

Ocramius Feb 25, 2017

Ocramius Feb 25, 2017

fullbl commented Feb 28, 2017

fullbl commented Mar 4, 2017

fullbl commented Mar 4, 2017

lcobucci left a comment

lcobucci Mar 5, 2017

lcobucci Mar 5, 2017

lcobucci Mar 5, 2017

lcobucci Mar 5, 2017

lcobucci Mar 5, 2017

lcobucci Mar 5, 2017

lcobucci Mar 6, 2017

fullbl commented Mar 11, 2017

Ocramius left a comment

Ocramius Mar 12, 2017

fullbl Apr 18, 2017

lcobucci Jun 26, 2017

Ocramius Mar 12, 2017

Ocramius Mar 12, 2017

Ocramius commented Jun 21, 2017

Ocramius commented Aug 18, 2017

Ocramius commented Aug 19, 2017

JanTvrdik commented Aug 29, 2017 •

edited

Loading

		];
		if( !empty($classMetadata->parentClasses && isset($this->_rsm->discriminatorColumns[$ownerMap]))){

Check if the discriminator value is null before converting it and setting into rowData during hydration #6304

Check if the discriminator value is null before converting it and setting into rowData during hydration #6304

Conversation

fullbl commented Feb 24, 2017

Ocramius commented Feb 24, 2017 via email

fullbl commented Feb 25, 2017

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

fullbl commented Feb 28, 2017

fullbl commented Mar 4, 2017

fullbl commented Mar 4, 2017

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

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

fullbl commented Mar 11, 2017

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

Ocramius commented Jun 21, 2017

Ocramius commented Aug 18, 2017

Ocramius commented Aug 19, 2017

JanTvrdik commented Aug 29, 2017 • edited Loading

Check if the discriminator value is `null` before converting it and setting into `rowData` during hydration #6304

Check if the discriminator value is `null` before converting it and setting into `rowData` during hydration #6304

JanTvrdik commented Aug 29, 2017 •

edited

Loading