[Serializer] Skip uninitialized (PHP 7.4) properties in PropertyNormalizer and ObjectNormalizer #34791

vudaltsov · 2019-12-03T18:04:22Z

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	n/a
License	MIT
Doc PR	n/a

When trying to read from an uninitialized property in PHP 7.4, a TypeError is generated, see https://wiki.php.net/rfc/typed_properties_v2#uninitialized_and_unset_properties. This PR fixes the issue.

dunglas · 2019-12-03T18:46:22Z

Hi, and thanks for contributing to Symfony!

Reading a non-initialized property having a type must trigger an error. In my opinion, it's not a bug in the Symfony Serializer but in the user's app. Adding this check will slow down a bit the serializer, and defeats the purpose of using Typed Properties.

👎 on my side.

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

vudaltsov · 2019-12-03T19:00:51Z

@dunglas , agree, an error might be the desired behavior. On the other hand allowing to ignore the error might ease the migration to PHP 7.4. What if we add a context option to allow for uninitialized properties? It will be false by default.

derrabus · 2019-12-03T19:05:06Z

What would be a valid use case of serializing an object with uninitialized properties? I fear that this change might hide a potential problem from the developer.

vudaltsov · 2019-12-03T19:20:20Z

If you never touch an uninitialized property of an object in the php 7.4 code, you will never get an error. So an object with some uninitialized properties is absolutely valid. The problem is that when you pass that absolutely valid object with uninitialized properties to the Symfony Serializer, you all of a sudden get an error.

Another solution would be to skip uninitialized properties when normalizing. So that they don't appear in the resulting array.

vudaltsov · 2019-12-03T19:31:16Z

@derrabus , any use case of serializing an object with uninitialized properties is valid, because an object with uninitialized properties is a valid object.

vudaltsov · 2019-12-03T19:37:26Z

final class SomeClass
{
    private string $property;
}

$obj = new SomeClass();

echo json_encode($obj);

If this is a valid case (see https://3v4l.org/rKfHb), then why this is not?

echo $serializer->serialize($obj, 'json');

derrabus · 2019-12-03T19:48:08Z

Fair enough. But even php itself makes a difference between uninitialized properties and null properties when serializing: https://3v4l.org/hkHp7

So, if we wanted the serializer to behave like php, we would need to skip an uninitialized property when normalizing the object.

vudaltsov · 2019-12-03T19:51:55Z

So, if we wanted the serializer to behave like php, we would need to skip an uninitialized property when normalizing the object.

Agree here. We can do exactly like json_encode does, we can make it configurable via the $context for more flexibility. @dunglas , what do you think?

dunglas · 2019-12-03T21:09:14Z

You make a good point!
Let's copy the behavior of json_encode. I don't think that a context option is necessary as long as we don't change the existing behavior for non-typed properties.

By the way, shouldn't ObjectNormalizer be updated accordingly?

vudaltsov · 2019-12-05T12:54:35Z

@dunglas , done for both. The Travis failure is unrelated.

It appears that ReflectionProperty::isInitialized can be called only on accessible properties, see https://travis-ci.org/symfony/symfony/jobs/621094371#L4416. Therefore, this if is is required.

vudaltsov · 2019-12-05T13:02:21Z

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

@@ -83,8 +83,14 @@ protected function extractAttributes($object, $format = null, array $context = [
            }
        }

+        $checkPropertyInitialization = \PHP_VERSION_ID >= 70400;


This can be implemented as a class constant in Symfony >= 4, btw

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

…lizer and ObjectNormalizer

fabpot · 2019-12-15T13:47:10Z

Thank you @vudaltsov.

…opertyNormalizer and ObjectNormalizer (vudaltsov) This PR was squashed before being merged into the 3.4 branch (closes #34791). Discussion ---------- [Serializer] Skip uninitialized (PHP 7.4) properties in PropertyNormalizer and ObjectNormalizer | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | n/a | License | MIT | Doc PR | n/a When trying to read from an uninitialized property in PHP 7.4, a `TypeError` is generated, see https://wiki.php.net/rfc/typed_properties_v2#uninitialized_and_unset_properties. This PR fixes the issue. Commits ------- 1ed8e42 [Serializer] Skip uninitialized (PHP 7.4) properties in PropertyNormalizer and ObjectNormalizer

…ith getters (BoShurik) This PR was merged into the 4.4 branch. Discussion ---------- [Serializer] Exclude non-initialized properties accessed with getters | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no  | Deprecations? | no  | Tickets | no  | License | MIT | Doc PR | symfony/symfony-docs#...  Allow to serialize ```php final class Php74DummyPrivate { private string $uninitializedProperty; private string $initializedProperty = 'defaultValue'; public function getUninitializedProperty(): string { return $this->uninitializedProperty; } public function getInitializedProperty(): string { return $this->initializedProperty; } } ``` Similar to #34791 Commits ------- da91003 Exclude non-initialized properties accessed with getters

…s in PropertyNormalizer and ObjectNormalizer (vudaltsov) This PR was squashed before being merged into the 3.4 branch (closes symfony#34791). Discussion ---------- [Serializer] Skip uninitialized (PHP 7.4) properties in PropertyNormalizer and ObjectNormalizer | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | n/a | License | MIT | Doc PR | n/a When trying to read from an uninitialized property in PHP 7.4, a `TypeError` is generated, see https://wiki.php.net/rfc/typed_properties_v2#uninitialized_and_unset_properties. This PR fixes the issue. Commits ------- 1ed8e42 [Serializer] Skip uninitialized (PHP 7.4) properties in PropertyNormalizer and ObjectNormalizer

…essed with getters (BoShurik) This PR was merged into the 4.4 branch. Discussion ---------- [Serializer] Exclude non-initialized properties accessed with getters | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no  | Deprecations? | no  | Tickets | no  | License | MIT | Doc PR | symfony/symfony-docs#...  Allow to serialize ```php final class Php74DummyPrivate { private string $uninitializedProperty; private string $initializedProperty = 'defaultValue'; public function getUninitializedProperty(): string { return $this->uninitializedProperty; } public function getInitializedProperty(): string { return $this->initializedProperty; } } ``` Similar to symfony#34791 Commits ------- da91003 Exclude non-initialized properties accessed with getters

…(ivannemets-sravniru) This PR was squashed before being merged into the 5.4 branch. Discussion ---------- [Serializer] #36594 attributes cache breaks normalization | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #36594 | License | MIT | Doc PR | symfony/symfony-docs#15823 The bug itself is explained in the following (simplified) example: ```php class Dummy { public array $requiredData; // supposed to be set for any object public array $optionalData; // can be not initialized (and if so - ignored by serializer) } $object1 = new Dummy(); $object1->requiredData = ['username' => 'foo']; $json1 = $serializer->serialize($object1, 'json'); // {"requiredData": {"username": "foo"}} // at this point object normalizer has already cached attributes for Dummy::class and context, // now it contains array ['requiredData'] - optionalData has been ignored as it's unitialized // then, while the script is still running, we have another object of the same class with optionalData set $object2 = new Dummy(); $object2->requiredData = ['username' => 'bar']; $object2->optionalData = ['email' => 'bar@test.com']; $json2 = $serializer->serialize($object2, 'json'); // expected: {"requiredData": {"username": "bar"}, "optionalData": {"email": "bar@test.com"}} // actual: {"requiredData": {"username": "bar"}} // here normalizer has no clue about optionalData attribute since it reuses attributes cached // in \Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::$attributesCache // while normalizing the first object ``` Though this PR created for 5.4 branch, it actually fixes a bug reproducible in 5.3. The reason why I use 5.4 for this fix is that 5.4 introduces a [new feature](symfony/symfony-docs#15823) related to the same problem. If this PR gets approved and merged, I can potentially implement the same fix for 5.3, but `SKIP_UNINITIALIZED_VALUES` will cause some merge conflicts in the future.. As of v 5.3 symfony ignores uninitialized properties by default in `ObjectNormalizer::extractAttributes` and `PropertyNormalizer::extractAttributes` (implemented in #38900 and #34791) but this approach is wrong - **`extractAttributes` method MUST return the same attributes list for any instance of the same class**, otherwise cached attributes do not match actual attributes list for different instances having different set of initialized/uninitialized properties So, this PR does a few things: 1. removes ignoring attributes from all built-in implementations of `\Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::extractAttributes` so that even uninitialized attributes will be cached by normalizer. Instead, we ignore uninitialized attributes when trying to access them while calling `\Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::getAttributeValue` 2. sets `true` as the default value for `SKIP_UNINITIALIZED_VALUES` context setting as I believe this is the expected default behavior for any built-in object normalizer 3. makes `SKIP_UNINITIALIZED_VALUES` compatible with not just `ObjectNormalizer`, but with two other built-in normalizers `PropertyNormalizer` and `GetSetMethodNormalizer` as well Commits ------- 55818c3 [Serializer] #36594 attributes cache breaks normalization

vudaltsov requested a review from dunglas as a code owner December 3, 2019 18:04

carsonbot added Status: Needs Review Serializer Bug labels Dec 3, 2019

vudaltsov force-pushed the property-normalizer-php-74 branch from a7e10fc to 71edc47 Compare December 3, 2019 18:17

derrabus reviewed Dec 3, 2019

View reviewed changes

src/Symfony/Component/Serializer/Normalizer/PropertyNormalizer.php Outdated Show resolved Hide resolved

nicolas-grekas added this to the next milestone Dec 4, 2019

vudaltsov changed the title ~~[Serializer] Added support for PHP 7.4 typed properties to PropertyNormalizer~~ [Serializer] Skip uninitialized (PHP 7.4) properties in PropertyNormalizer and ObjectNormalizer Dec 5, 2019

vudaltsov commented Dec 5, 2019

View reviewed changes

nicolas-grekas modified the milestones: next, 3.4 Dec 15, 2019

nicolas-grekas reviewed Dec 15, 2019

View reviewed changes

src/Symfony/Component/Serializer/Normalizer/PropertyNormalizer.php Show resolved Hide resolved

nicolas-grekas approved these changes Dec 15, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 15, 2019

derrabus approved these changes Dec 15, 2019

View reviewed changes

fabpot approved these changes Dec 15, 2019

View reviewed changes

[Serializer] Skip uninitialized (PHP 7.4) properties in PropertyNorma…

1ed8e42

…lizer and ObjectNormalizer

fabpot force-pushed the property-normalizer-php-74 branch from 576126a to 1ed8e42 Compare December 15, 2019 13:47

fabpot merged commit 1ed8e42 into symfony:3.4 Dec 15, 2019

This was referenced Dec 19, 2019

Release v4.4.2 #35053

Merged

Release v5.0.2 #35055

Merged

vudaltsov deleted the property-normalizer-php-74 branch December 19, 2019 18:46

This was referenced Jan 21, 2020

Release v3.4.37 #35411

Merged

Release v4.3.10 #35412

Merged

This was referenced Oct 30, 2020

[Serializer] Exclude non-initialized properties accessed with getters #38899

Closed

[Serializer] Exclude non-initialized properties accessed with getters #38900

Merged

[Serializer] Add ability to skip uninitialized properties #38972

Closed

ivannemets-sravniru mentioned this pull request Oct 13, 2021

[Serializer] symfony#36594 attributes cache breaks normalization #43469

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serializer] Skip uninitialized (PHP 7.4) properties in PropertyNormalizer and ObjectNormalizer #34791

[Serializer] Skip uninitialized (PHP 7.4) properties in PropertyNormalizer and ObjectNormalizer #34791

vudaltsov commented Dec 3, 2019 •

edited

dunglas commented Dec 3, 2019

vudaltsov commented Dec 3, 2019

derrabus commented Dec 3, 2019

vudaltsov commented Dec 3, 2019 •

edited

vudaltsov commented Dec 3, 2019

vudaltsov commented Dec 3, 2019

derrabus commented Dec 3, 2019

vudaltsov commented Dec 3, 2019

dunglas commented Dec 3, 2019

vudaltsov commented Dec 5, 2019

vudaltsov Dec 5, 2019

fabpot commented Dec 15, 2019

[Serializer] Skip uninitialized (PHP 7.4) properties in PropertyNormalizer and ObjectNormalizer #34791

[Serializer] Skip uninitialized (PHP 7.4) properties in PropertyNormalizer and ObjectNormalizer #34791

Conversation

vudaltsov commented Dec 3, 2019 • edited

dunglas commented Dec 3, 2019

vudaltsov commented Dec 3, 2019

derrabus commented Dec 3, 2019

vudaltsov commented Dec 3, 2019 • edited

vudaltsov commented Dec 3, 2019

vudaltsov commented Dec 3, 2019

derrabus commented Dec 3, 2019

vudaltsov commented Dec 3, 2019

dunglas commented Dec 3, 2019

vudaltsov commented Dec 5, 2019

vudaltsov Dec 5, 2019

Choose a reason for hiding this comment

fabpot commented Dec 15, 2019

vudaltsov commented Dec 3, 2019 •

edited

vudaltsov commented Dec 3, 2019 •

edited