NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:3.4fromalanpoulain:serializer-fix-cache-key-unitialized-properties

Apr 4, 2020

Merged

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

fabpot merged 1 commit intosymfony:3.4fromalanpoulain:serializer-fix-cache-key-unitialized-properties

Apr 4, 2020

Conversation

Copy link

Contributor

alanpoulain commentedApr 3, 2020•
edited
Loading

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;
TheAuthor entity uses typed properties (PHP 7.4) not initialized;
TheSerializer is used with theBook in theOBJECT_TO_POPULATE key in the context.

For instance:

<?phpdeclare(strict_types=1);namespaceApp\Entity;useDoctrine\ORM\MappingasORM;/** @ORM\Entity */class Book{/**     * @ORM\ManyToOne(targetEntity="Author")     */publicAuthor$author;public ?string$isbn;}

<?phpdeclare(strict_types=1);namespaceApp\Entity;useDoctrine\ORM\MappingasORM;/** @ORM\Entity */class Author{public ?string$name;}

Or even:

<?phpdeclare(strict_types=1);namespaceApp\Entity;useDoctrine\ORM\MappingasORM;/** @ORM\Entity */class Author{privatestring$name;publicfunction__construct()    {$this->name ='Leo';    }}

If the following is done (it's the case for instance in API Platform when aPUT 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 thegetCacheKey method of theAbstractObjectNormalizer:

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

Lines 405 to 409 in5da141b

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

Since the lazy proxyfied relation has a__sleep with unitialized properties, theserialize method will throw (sincehttps://bugs.php.net/bug.php?id=79002:php/php-src@846b647).

I propose to fix this issue by unsetting theOBJECT_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 in15edfd3

$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?).

alanpoulain requested a review fromdunglas as acode owner

April 3, 2020 12:44

carsonbot added Status: Needs Review Serializer Bug labels

Apr 3, 2020

Copy link

Member

dunglas commentedApr 3, 2020•
edited
Loading

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 inobject_to_populate will be enough, no need for a Doctrine entity).

Copy link

Member

dunglas commentedApr 3, 2020

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

dunglas mentioned this pull request

Apr 3, 2020

Problem with Proxies on PHP 7.4.2 with typed propertiesdoctrine/orm#8030

Closed

Copy link

ContributorAuthor

alanpoulain commentedApr 3, 2020

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 inObjectNormalizerTest to test this behavior (testNormalizeNotSerializableContext).

Copy link

2ec0b4 commentedApr 3, 2020

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)