Skip to content

[Serializer] Documenting the new SKIP_UNINITIALIZED_VALUES option #15823

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

Merged
merged 2 commits into from
Oct 26, 2022

Conversation

vuryss
Copy link
Contributor

@vuryss vuryss commented Sep 16, 2021

Documents changes introduced here: symfony/symfony#41615
Original issue: symfony/symfony#40578
Docs issue: #15785

fabpot added a commit to symfony/symfony that referenced this pull request Oct 16, 2021
…(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
@javiereguiluz javiereguiluz added this to the 5.4 milestone Oct 26, 2022
@javiereguiluz javiereguiluz merged commit 1fc575c into symfony:5.4 Oct 26, 2022
@javiereguiluz
Copy link
Member

Georgi, congrats on your first Symfony Docs contribution 🎉

This was an amazing first-time contribution, and I'm really sorry it took us so long to merge it.

@vuryss vuryss deleted the serializer_skip_uninitialized branch October 28, 2022 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants