[Serializer] Fix unitialized properties (from PHP 7.4.2) when serializing context for the cache key #36332

alanpoulain · 2020-04-03T12:44:31Z

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #35574 doctrine/orm#8030
License	MIT
Doc PR	N/A

This bug only happens on the following conditions:

A Doctrine entity (Book) having a relation with another entity (Author) is used;
The Author entity uses typed properties (PHP 7.4) not initialized;
The Serializer is used with the Book in the OBJECT_TO_POPULATE key in the context.

For instance:

<?php

declare(strict_types=1);

namespace App\Entity;

use Doctrine\ORM\Mapping as ORM;

/** @ORM\Entity */
class Book
{
    /**
     * @ORM\ManyToOne(targetEntity="Author")
     */
	public Author $author;

	public ?string $isbn;
}

<?php

declare(strict_types=1);

namespace App\Entity;

use Doctrine\ORM\Mapping as ORM;

/** @ORM\Entity */
class Author
{
    public ?string $name;
}

Or even:

<?php

declare(strict_types=1);

namespace App\Entity;

use Doctrine\ORM\Mapping as ORM;

/** @ORM\Entity */
class Author
{
    private string $name;

    public function __construct()
    {
        $this->name = 'Leo';
    }
}

If the following is done (it's the case for instance in API Platform when a PUT is made):

$serializer->deserialize('{"isbn":"2038717141"}', Book::class, 'json', ['object_to_populate' => $book]);

Then there will be the following error:

Fatal error: Typed property Proxies_CG_\App\Entity\Author::$ must not be accessed before initialization (in __sleep)

It's because of these lines in the getCacheKey method of the AbstractObjectNormalizer:

symfony/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

Lines 405 to 409 in 5da141b

    
           return md5($format.serialize([ 
        
               'context' => $context, 
        
               'ignored' => $this->ignoredAttributes, 
        
               'camelized' => $this->camelizedAttributes, 
        
           ]));

Since the lazy proxyfied relation has a __sleep with unitialized properties, the serialize method will throw (since https://bugs.php.net/bug.php?id=79002: php/php-src@846b647).

I propose to fix this issue by unsetting the OBJECT_TO_POPULATE key in the context because I don't think it's useful for determining the attributes of the object.

For the next versions of Symfony, the fix should probably be elsewhere, in the default context.
For instance in Symfony 4.4, instead of:

symfony/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

Line 118 in 15edfd3

    
           $this->defaultContext[self::EXCLUDE_FROM_CACHE_KEY] = [self::CIRCULAR_REFERENCE_LIMIT_COUNTERS];

It should be:

$this->defaultContext[self::EXCLUDE_FROM_CACHE_KEY] = [self::CIRCULAR_REFERENCE_LIMIT_COUNTERS, self::OBJECT_TO_POPULATE];

But I'm not sure how it should be merged (another PR maybe?).

dunglas · 2020-04-03T13:06:50Z

To be honest, I'm not sure if what you describe in the description is a bug or not, because when using typed properties, you must always initialize the property, either in its definition of in the constructor, that's the spirit of this feature. However, your patch is legit, because according to the Doctrine documentation:

If you intend to serialize (and unserialize) entity instances that still hold references to proxy objects you may run into problems with private properties because of technical limitations. Proxy objects implement __sleep and it is not possible for __sleep to return names of private properties in parent classes.

object_to_populate likely contains a Doctrine entity, so we must not serialize it (+ it's useless to serialize it).

Could you add a test to prevent regressions? (Just testing than putting something to serializable such as a closure in object_to_populate will be enough, no need for a Doctrine entity).

dunglas · 2020-04-03T13:11:02Z

Also, there is probably something to fix in Doctrine too. Our call to serialize() is already in a try/catch block. Doctrine shouldn't generate uncatchable errors like that.

alanpoulain · 2020-04-03T13:45:23Z

It cannot really tested since serializing a closure will throw an exception, not an error (and the exception is catched).
And there is already a test in ObjectNormalizerTest to test this behavior (testNormalizeNotSerializableContext).

2ec0b4 · 2020-04-03T13:55:06Z

Pay attention to the destination branch: symfony:3.4 here, but the initial issue #35574 is about symfony:4.4. This should be applied on several versions?

I reproduce this behavior on SF 4.4 here: doctrine/common#886 (comment)

dunglas · 2020-04-03T14:02:06Z

@alanpoulain for the test you can do the like it that: https://github.com/symfony/symfony/pull/36336/files#diff-e6bd6c603753788029968840196a888cR395

@2ec0b4 patches are merged to upper branches, it will be fixed in 3.4 and all superior versions.

…zing context for the cache key

fabpot · 2020-04-04T07:17:08Z

Thank you @alanpoulain.

alanpoulain requested a review from dunglas as a code owner April 3, 2020 12:44

carsonbot added Status: Needs Review Serializer Bug labels Apr 3, 2020

dunglas mentioned this pull request Apr 3, 2020

Problem with Proxies on PHP 7.4.2 with typed properties doctrine/orm#8030

Closed

dunglas mentioned this pull request Apr 3, 2020

[Serializer] Catch \Throwable in getCacheKey() #36336

Closed

dunglas approved these changes Apr 3, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 3, 2020

chalasr approved these changes Apr 3, 2020

View reviewed changes

nicolas-grekas added this to the 3.4 milestone Apr 3, 2020

fabpot approved these changes Apr 4, 2020

View reviewed changes

[Serializer] Fix unitialized properties (from PHP 7.4.2) when seriali…

1fafff7

…zing context for the cache key

fabpot force-pushed the serializer-fix-cache-key-unitialized-properties branch from 03bd90a to 1fafff7 Compare April 4, 2020 07:17

fabpot merged commit 6dbf9eb into symfony:3.4 Apr 4, 2020

alanpoulain mentioned this pull request Apr 6, 2020

[Serializer] Use default context EXCLUDE_FROM_CACHE_KEY for unsetting OBJECT_TO_POPULATE #36363

Closed

This was referenced Apr 28, 2020

Release v3.4.40 #36612

Merged

Release v4.4.8 #36614

Merged

Release v5.0.8 #36615

Merged

allan-simon mentioned this pull request Sep 23, 2020

[Serializer] Typed property Proxies\__CG__\App\Entity\Website::$ must not be accessed before initialization (in __sleep) #38274

Closed

jrushlow mentioned this pull request Feb 22, 2022

Remove question mark when the property is not nullable on getter symfony/maker-bundle#1074

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serializer] Fix unitialized properties (from PHP 7.4.2) when serializing context for the cache key #36332

[Serializer] Fix unitialized properties (from PHP 7.4.2) when serializing context for the cache key #36332

alanpoulain commented Apr 3, 2020 •

edited

Loading

dunglas commented Apr 3, 2020 •

edited

Loading

dunglas commented Apr 3, 2020

alanpoulain commented Apr 3, 2020

2ec0b4 commented Apr 3, 2020

dunglas commented Apr 3, 2020

fabpot commented Apr 4, 2020

	return md5($format.serialize([
	'context' => $context,
	'ignored' => $this->ignoredAttributes,
	'camelized' => $this->camelizedAttributes,
	]));

[Serializer] Fix unitialized properties (from PHP 7.4.2) when serializing context for the cache key #36332

[Serializer] Fix unitialized properties (from PHP 7.4.2) when serializing context for the cache key #36332

Conversation

alanpoulain commented Apr 3, 2020 • edited Loading

dunglas commented Apr 3, 2020 • edited Loading

dunglas commented Apr 3, 2020

alanpoulain commented Apr 3, 2020

2ec0b4 commented Apr 3, 2020

dunglas commented Apr 3, 2020

fabpot commented Apr 4, 2020

alanpoulain commented Apr 3, 2020 •

edited

Loading

dunglas commented Apr 3, 2020 •

edited

Loading