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

[alanpoulain]

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]

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]

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]

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]

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]

@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.

[fabpot]

Thank you @alanpoulain.