Skip to content

Commit

Permalink
bug #36332 [Serializer] Fix unitialized properties (from PHP 7.4.2) w…
Browse files Browse the repository at this point in the history
…hen serializing context for the cache key (alanpoulain)

This PR was squashed before being merged into the 3.4 branch.

Discussion
----------

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

| 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
<?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
<?php

declare(strict_types=1);

namespace App\Entity;

use Doctrine\ORM\Mapping as ORM;

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

Or even:

```php
<?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):
```php
$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`:
https://github.com/symfony/symfony/blob/5da141b8d0ec7d71fb0ad637245833cd7fa56164/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php#L405-L409

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:
https://github.com/symfony/symfony/blob/15edfd39d4b3da4d011a6242238c4b2874eb1ace/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php#L118
It should be:
```php
$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?).

Commits
-------

1fafff7 [Serializer] Fix unitialized properties (from PHP 7.4.2) when serializing context for the cache key
  • Loading branch information
fabpot committed Apr 4, 2020
2 parents 60a35f8 + 1fafff7 commit 6dbf9eb
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 0 deletions.
Expand Up @@ -400,6 +400,7 @@ protected function createChildContext(array $parentContext, $attribute/*, string
*/
private function getCacheKey($format, array $context)
{
unset($context[self::OBJECT_TO_POPULATE]);
unset($context['cache_key']); // avoid artificially different keys
try {
return md5($format.serialize([
Expand Down
Expand Up @@ -163,6 +163,14 @@ public function testDenormalizeStringCollectionDecodedFromXmlWithTwoChildren()
$this->assertEquals('bar', $stringCollection->children[1]);
}

public function testDenormalizeNotSerializableObjectToPopulate()
{
$normalizer = new AbstractObjectNormalizerDummy();
$normalizedData = $normalizer->denormalize(['foo' => 'foo'], Dummy::class, null, [AbstractObjectNormalizer::OBJECT_TO_POPULATE => new NotSerializable()]);

$this->assertSame('foo', $normalizedData->foo);
}

private function getDenormalizerForStringCollection()
{
$extractor = $this->getMockBuilder(PhpDocExtractor::class)->getMock();
Expand Down Expand Up @@ -379,3 +387,15 @@ public function setSerializer(SerializerInterface $serializer)
$this->serializer = $serializer;
}
}

class NotSerializable
{
public function __sleep()
{
if (class_exists(\Error::class)) {
throw new \Error('not serializable');
}

throw new \Exception('not serializable');
}
}

0 comments on commit 6dbf9eb

Please sign in to comment.