Skip to content

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

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 1 commit into from
Jan 26, 2021
Merged

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

merged 1 commit into from
Jan 26, 2021

Conversation

BoShurik
Copy link
Contributor

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

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

@BoShurik
Copy link
Contributor Author

BoShurik commented Nov 3, 2020

I'm not sure the failed appveyor build is related. All tests are Ok.

@dunglas
Copy link
Member

dunglas commented Jan 25, 2021

It looks like we have a similar issue in API Platform: api-platform/core#3974
I wonder if this shouldn't be fixed once and for all in PropertyAccess (this doesn't prevent to merge this PR first).

@nicolas-grekas
Copy link
Member

PR welcome on PropertyAccess if someone is up to having a look there.

@nicolas-grekas
Copy link
Member

Thank you @BoShurik.

@nicolas-grekas nicolas-grekas merged commit 484a95d into symfony:4.4 Jan 26, 2021
This was referenced Jan 27, 2021
nicolas-grekas added a commit that referenced this pull request Jan 27, 2021
…getters (julienfalque)

This PR was merged into the 4.4 branch.

Discussion
----------

[Serializer] Prevent access to private properties without getters

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

When upgrading `symfony/serializer` from `v5.2.1` to `v5.2.2`, the serializer starts throwing exceptions because it cannot access some private properties that don't have a getter. This looks related to #38900.

Commits
-------

f0409b4 [Serializer] Prevent access to private properties without getters
@gkurin
Copy link

gkurin commented Jan 28, 2021

Hey guys, I'm having an issue with this PR, since it's been merged, I'm receiving the following error from the Property Accessor.

I've searched through the whole project and could not find the methods described in the error, what I can find is the domainEvents property, which is included in a Trait being used by the class "App\Domain\Shift\Rating".

Does anybody have any insights on this issue or has had any similar issues with this update? I've restricted the Serializer package to version 4.4.18 and this has prevented the problem, but I would like to find a better solution.

Thanks!!

code: 500
line: 429
message: "Neither the property "domainEvents" nor one of the methods "getDomainEvents()", "domainEvents()", "isDomainEvents()", "hasDomainEvents()", "__get()" exist and have public access in class "App\Domain\Shift\Rating"."
title: ""
trace: "#0 /app/vendor/symfony/property-access/PropertyAccessor.php(93): Symfony\Component\PropertyAccess\PropertyAccessor->readProperty(Array, 'domainEvents', false)↵#1 /app/vendor/symfony/serializer/Normalizer/ObjectNormalizer.php(148): Symfony\Component\PropertyAccess\PropertyAccessor->getValue(Object(App\Domain\Shift\Rating), 'domainEvents')↵#2 /app/vendor/symfony/serializer/Normalizer/AbstractObjectNormalizer.php(183): Symfony\Component\Serializer\Normalizer\ObjectNormalizer->getAttributeValue(Object(App\Domain\Shift\Rating), 'domainEvents', NULL, Array)↵#3 /app/vendor/symfony/serializer/Serializer.php(153): Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer->normalize(Object(App\Domain\Shift\Rating), NULL, Array)↵#4 /app/vendor/symfony/serializer/Serializer.php(167): Symfony\Component\Serializer\Serializer->normalize(Object(App\Domain\Shift\Rating), NULL, Array)↵#5 /app/src/Application/Service/Fractal/FractalDataArraySerializer.php(52): Symfony\Component\Serializer\Serializer->normalize(Array)↵#6 /app/src/Application/Service/Fractal/FractalScope.php(33): App\Application\Service\Fractal\FractalDataArraySerializer->item(NULL, Array)↵#7 /app/vendor/league/fractal/src/Scope.php(239): App\Application\Service\Fractal\FractalScope->serializeResource(Object(App\Application\Service\Fractal\FractalDataArraySerializer), Array)↵#8 /app/src/Application/Service/Fractal/FractalService.php(55): League\Fractal\Scope->toArray()↵#9 /app/src/Application/Http/Controller/ApiController.php(39): App\Application\Service\Fractal\FractalService->item(Object(App\Domain\Shift\Shift), Object(App\Application\Http\Transformer\Shift\ShiftForNurseTransformer))↵#10 /app/src/Application/Http/Controller/ApiController.php(91): App\Application\Http\Controller\ApiController->item(Object(App\Domain\Shift\Shift), Object(App\Application\Http\Transformer\Shift\ShiftForNurseTransformer), Array)↵#11 /app/src/Application/Http/Controller/HospitalShiftController.php(231): App\Application\Http\Controller\ApiController->resource(Object(App\Domain\Shift\Shift), Object(App\Application\Http\Transformer\Shift\ShiftForNurseTransformer))↵#12 /app/vendor/symfony/http-kernel/HttpKernel.php(158): App\Application\Http\Controller\HospitalShiftController->shiftView('d0a1a029-ea81-4...', Object(App\Infrastructure\Persistence\Doctrine\ShiftRepository), Object(App\Application\Http\Transformer\Shift\ShiftForNurseTransformer))↵#13 /app/vendor/symfony/http-kernel/HttpKernel.php(80): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)↵#14 /app/vendor/symfony/http-kernel/Kernel.php(201): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)↵#15 /app/public/index.php(49): Symfony\Component\HttpKernel\Kernel->handle(Object(Symfony\Component\HttpFoundation\Request))↵#16 {main}"

@nicolas-grekas
Copy link
Member

@gkurin this should have been fixed by #40004

hultberg pushed a commit to hultberg/symfony that referenced this pull request Sep 17, 2021
…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
fabpot added a commit 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
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.

5 participants