added ability to handle parent classes for PropertyNormalizer #24321

ivoba · 2017-09-25T16:20:20Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#24152
License	MIT
Doc PR	symfony/symfony-docs#...

This adds the ability for PropertyNormalizer to normalize/denormalize properties from parent classes.

dunglas · 2017-09-26T21:40:35Z

src/Symfony/Component/Serializer/Normalizer/PropertyNormalizer.php

@@ -84,6 +84,21 @@ protected function isAllowedAttribute($classOrObject, $attribute, $format = null
                return false;
            }
        } catch (\ReflectionException $reflectionException) {
+            $className = is_string($classOrObject) ? $classOrObject : get_class($classOrObject);


The first check (line 82) can also be in the while right?

dunglas · 2017-09-26T21:41:44Z

src/Symfony/Component/Serializer/Normalizer/PropertyNormalizer.php

+
+            while ($parent = $class->getParentClass()) {
+                try {
+                    $reflectionProperty = new \ReflectionProperty($parent->getName(), $attribute);


Static methods must be excluded.

dunglas · 2017-09-26T21:42:37Z

src/Symfony/Component/Serializer/Normalizer/PropertyNormalizer.php

+
+            while ($parent = $class->getParentClass()) {
+                try {
+                    $reflectionProperty = new \ReflectionProperty($parent->getName(), $attribute);


As you have an instance of ReflectionClass available, maybe can you just call getProperty?

dunglas · 2017-09-26T21:42:55Z

src/Symfony/Component/Serializer/Normalizer/PropertyNormalizer.php

+                    if ($reflectionProperty) {
+                        return true;
+                    }
+                } catch (\Exception $e) {


Only ReflectionException should be catched.

dunglas · 2017-09-26T21:44:38Z

src/Symfony/Component/Serializer/Normalizer/PropertyNormalizer.php

-            return;
+            $class = get_parent_class($object);
+            do {
+                $reflectionProperty = new \ReflectionProperty(get_parent_class($object), $attribute);


Exceptions aren't caught here.

dunglas · 2017-09-26T21:44:50Z

src/Symfony/Component/Serializer/Normalizer/PropertyNormalizer.php

@@ -117,7 +134,13 @@ protected function getAttributeValue($object, $attribute, $format = null, array
        try {
            $reflectionProperty = new \ReflectionProperty(get_class($object), $attribute);


Same here, should be in the do/while

dunglas · 2017-09-26T21:45:14Z

src/Symfony/Component/Serializer/Normalizer/PropertyNormalizer.php

+            $class = get_parent_class($object);
+            do {
+                $reflectionProperty = new \ReflectionProperty(get_parent_class($object), $attribute);
+                if (null === $reflectionProperty) {


How can it be null?

dunglas · 2017-09-26T21:45:29Z

src/Symfony/Component/Serializer/Normalizer/PropertyNormalizer.php

@@ -136,7 +159,13 @@ protected function setAttributeValue($object, $attribute, $value, $format = null
        try {
            $reflectionProperty = new \ReflectionProperty(get_class($object), $attribute);


kbond · 2017-09-27T12:12:15Z

@ivoba I suggest adding a private method getReflectionProperty to reduce the duplication, ie:

private function getReflectionProperty($classOrObject, $attribute)
{
    $class = is_string($classOrObject) ? $classOrObject : get_class($classOrObject);

    try {
        return new \ReflectionProperty($class, $attribute);
    } catch (\ReflectionException $e) {
        $class = new \ReflectionClass($class);

        while ($parent = $class->getParentClass()) {
            try {
                return new \ReflectionProperty($parent->getName(), $attribute);
            } catch (\ReflectionException $e) {
                $class = $parent;
            }
        }

        throw $e;
    }
}

…erty retrieval

ivoba · 2017-09-28T14:14:16Z

i updated the PR and took @kbond suggestion into account.
@dunglas pls review if the changes fit your comments.

i have no idea why the test in PHP7.1 fail, looks not really related??

dunglas · 2017-09-28T15:16:25Z

src/Symfony/Component/Serializer/Normalizer/PropertyNormalizer.php

+     *
+     * @throws \ReflectionException
+     */
+    private function getReflectionProperty($classOrObject, $attribute)


What about:

private function getReflectionProperty($classOrObject, $attribute) { $class = is_string($classOrObject) ? $classOrObject : get_class($classOrObject); while (true) { try { return isset($reflectionClass) ? $reflectionClass->getProperty($attribute) : new \ReflectionProperty($class, $attribute); } catch (\ReflectionException $e) { $reflectionClass = new \ReflectionClass($class); if (!$reflectionClass = $reflectionClass->getParentClass()) { throw $e; } } } }

This runs into an endless loop.

Would work with this:

... $reflectionClass = new \ReflectionClass($class); $reflectionClass = $reflectionClass->getParentClass(); if ($reflectionClass === false) { throw $e; } ...

I've updated my snippet, it should work now

i updated the PR with the snippet included

xabbuh · 2017-10-06T20:57:22Z

src/Symfony/Component/Serializer/Normalizer/PropertyNormalizer.php

+            try {
+                return isset($reflectionClass) ? $reflectionClass->getProperty($attribute) : new \ReflectionProperty($class, $attribute);
+            } catch (\ReflectionException $e) {
+                $reflectionClass = new \ReflectionClass($class);


Wouldn't it be better to initialize this outside the loop?

fabpot · 2017-10-08T23:56:15Z

src/Symfony/Component/Serializer/Normalizer/PropertyNormalizer.php

+            try {
+                return isset($reflectionClass) ? $reflectionClass->getProperty($attribute) : new \ReflectionProperty($class, $attribute);
+            } catch (\ReflectionException $e) {
+                $reflectionClass = new \ReflectionClass($class);


Looks wrong to me as you are always overriding $reflectionClass with $class. This statement should only be executed if $reflectionClass is not set. I would add a test with a more complex hierarchy: class < parent < parent. Current code would loop forever.

ivoba · 2017-10-10T07:02:12Z

I added a new Dummy class with 3 level inheritance and applied suggested fixes.

fabpot · 2017-10-10T17:50:38Z

src/Symfony/Component/Serializer/Normalizer/PropertyNormalizer.php

+     */
+    private function getReflectionProperty($classOrObject, $attribute)
+    {
+        $class = is_string($classOrObject) ? $classOrObject : get_class($classOrObject);


This is not needed as the ReflectionClass constructor takes either a class or an object as an argument.

ivoba · 2017-10-11T06:33:31Z

removed mentioned line.
fabbot fails for License Headers but diff is empty. probably a fabbot bug?

xabbuh · 2017-10-11T07:30:23Z

your GroupDummyChild.php fixtures file is missing the license header

ivoba · 2017-10-11T08:19:56Z

ok thx, licence header is added

fabpot · 2017-10-11T15:26:33Z

Thank you @ivoba.

…malizer (ivoba) This PR was squashed before being merged into the 3.4 branch (closes #24321). Discussion ---------- added ability to handle parent classes for PropertyNormalizer | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes  | BC breaks? | no | Deprecations? | no  | Tests pass? | yes | Fixed tickets | #24152  | License | MIT | Doc PR | symfony/symfony-docs#...   This adds the ability for PropertyNormalizer to normalize/denormalize properties from parent classes. Commits ------- 5ecafc5 added ability to handle parent classes for PropertyNormalizer

carsonbot added Status: Needs Review Feature labels Sep 25, 2017

added ability to handle parent classes for PropertyNormalizer

9ef0d25

ivoba force-pushed the ticket_24152 branch from e8c6e34 to 9ef0d25 Compare September 25, 2017 16:31

nicolas-grekas added this to the 3.4 milestone Sep 26, 2017

nicolas-grekas requested a review from dunglas September 26, 2017 09:56

dunglas added the Serializer label Sep 26, 2017

dunglas requested changes Sep 26, 2017

View reviewed changes

refactored to use new getReflectionProperty method for reflectionProp…

9786dd4

…erty retrieval

ivoba force-pushed the ticket_24152 branch from 3f3eab5 to 9786dd4 Compare September 28, 2017 13:44

dunglas reviewed Sep 28, 2017

View reviewed changes

ivoba force-pushed the ticket_24152 branch 2 times, most recently from 624d7f5 to 880bebe Compare September 28, 2017 16:06

carsonbot added Status: Reviewed and removed Status: Needs Review labels Sep 28, 2017

dunglas approved these changes Sep 28, 2017

View reviewed changes

Simperfit approved these changes Oct 1, 2017

View reviewed changes

xabbuh reviewed Oct 6, 2017

View reviewed changes

nicolas-grekas approved these changes Oct 8, 2017

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Oct 8, 2017

fabpot requested changes Oct 8, 2017

View reviewed changes

ivoba force-pushed the ticket_24152 branch 2 times, most recently from 8306c91 to 4697028 Compare October 10, 2017 06:45

fabpot requested changes Oct 10, 2017

View reviewed changes

ivoba force-pushed the ticket_24152 branch from 4697028 to ae7cb05 Compare October 11, 2017 06:20

getReflectionProperty refactor

1c955d3

ivoba force-pushed the ticket_24152 branch from ae7cb05 to 1c955d3 Compare October 11, 2017 07:45

carsonbot added Status: Reviewed and removed Status: Needs Work labels Oct 11, 2017

xabbuh approved these changes Oct 11, 2017

View reviewed changes

fabpot approved these changes Oct 11, 2017

View reviewed changes

fabpot closed this Oct 11, 2017

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

javiereguiluz mentioned this pull request Oct 29, 2017

[Serializer] Added ability to handle parent classes for PropertyNormalizer symfony/symfony-docs#8564

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added ability to handle parent classes for PropertyNormalizer #24321

added ability to handle parent classes for PropertyNormalizer #24321

ivoba commented Sep 25, 2017

dunglas Sep 26, 2017

dunglas Sep 26, 2017

dunglas Sep 26, 2017

dunglas Sep 26, 2017

dunglas Sep 26, 2017

dunglas Sep 26, 2017

dunglas Sep 26, 2017

dunglas Sep 26, 2017

kbond commented Sep 27, 2017

ivoba commented Sep 28, 2017

dunglas Sep 28, 2017 •

edited

Loading

ivoba Sep 28, 2017

ivoba Sep 28, 2017

dunglas Sep 28, 2017

ivoba Sep 28, 2017

xabbuh Oct 6, 2017

fabpot Oct 8, 2017

ivoba commented Oct 10, 2017

fabpot Oct 10, 2017

ivoba commented Oct 11, 2017

xabbuh commented Oct 11, 2017

ivoba commented Oct 11, 2017

fabpot commented Oct 11, 2017

		@@ -117,7 +134,13 @@ protected function getAttributeValue($object, $attribute, $format = null, array
		try {
		$reflectionProperty = new \ReflectionProperty(get_class($object), $attribute);

		@@ -136,7 +159,13 @@ protected function setAttributeValue($object, $attribute, $value, $format = null
		try {
		$reflectionProperty = new \ReflectionProperty(get_class($object), $attribute);

added ability to handle parent classes for PropertyNormalizer #24321

added ability to handle parent classes for PropertyNormalizer #24321

Conversation

ivoba commented Sep 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbond commented Sep 27, 2017

ivoba commented Sep 28, 2017

dunglas Sep 28, 2017 • edited Loading

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

ivoba commented Oct 10, 2017

Choose a reason for hiding this comment

ivoba commented Oct 11, 2017

xabbuh commented Oct 11, 2017

ivoba commented Oct 11, 2017

fabpot commented Oct 11, 2017

dunglas Sep 28, 2017 •

edited

Loading