-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Serializer] Skip uninitialized (PHP 7.4) properties in PropertyNormalizer and ObjectNormalizer #34791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
a7e10fc
to
71edc47
Compare
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
Outdated
Show resolved
Hide resolved
@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 |
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. |
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. |
@derrabus , any use case of serializing an object with uninitialized properties is valid, because an object with uninitialized properties is a valid object. |
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'); |
Fair enough. But even php itself makes a difference between uninitialized properties and 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 |
You make a good point! By the way, shouldn't |
@dunglas , done for both. The Travis failure is unrelated. It appears that |
@@ -83,8 +83,14 @@ protected function extractAttributes($object, $format = null, array $context = [ | |||
} | |||
} | |||
|
|||
$checkPropertyInitialization = \PHP_VERSION_ID >= 70400; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be implemented as a class constant in Symfony >= 4, btw
…lizer and ObjectNormalizer
576126a
to
1ed8e42
Compare
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 <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | no <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> 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 <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | no <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> 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
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.