schmittjoh / serializer

Library for (de-)serializing data of any complexity (supports JSON, and XML)
http://jmsyst.com/libs/serializer
MIT License
2.32k stars 589 forks source link

Deserializing with missing promoted properties results in uninitialized state #1472

Closed mpragliola closed 5 months ago

mpragliola commented 1 year ago
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no

PHP version: 8.1.15 JMS Serializer version: 3.22.0

When I try to deserialize some payload into a class with promoted properties and a default value, I get an error when I try to use missing properties, despite having default values set in the constructor.

Steps required to reproduce the problem

  1. Define a class with promoted properties and default values
class Foo
{
    public function __construct(
        public string $bar,
        public ?string $baz = null,
    ) {
    }
}
  1. Deserialize into that class a payload where the property with the default is missing:
$data = '{"bar":"hello"}';
$fooInstance = $this->serializer->deserialize($data, Foo::class, 'json');
  1. Use the missing property
print_r($fooInstance->baz);

Expected Result

null

Actual Result

Error: Typed property Foo::baz must not be accessed before initialization

Possible root cause

As far as I've seen, the target class is instantiated via reflection but without using the constructor itself (Instantiator.php):

        if ($this->isInstantiableViaReflection($reflectionClass)) {
            return [$reflectionClass, 'newInstanceWithoutConstructor'];
        }

This would leave those properties in an uninitialized state.

Workaround

I have to explicitly initialize and demote the properties:

class Foo
{
    public ?string $baz = null;

    public function __construct(public string $bar, ?string $baz) 
    {
        $this->baz = $baz;
    }
}
scyzoryck commented 1 year ago

Hi @mpragliola ! Is it the same issue as in https://github.com/schmittjoh/serializer/issues/1367 ?

mpragliola commented 1 year ago

@scyzoryck no, I don't think so. In #1367 the property that causes the error has no default value and the class has no constructor, while in my case there is clearly a null assigned as default, but as promoted property. And, if i demote the property and assign the default outside the constructor, the error disappears, which is logical given the constructor is completely skipped by JMSSerializer in this case.

gremo commented 1 year ago

Same problem here with Collection, even worst because I cannot initialize it:

    #[ORM\OneToMany(mappedBy: 'cart', targetEntity: CartItem::class)]
    #[Serializer\Accessor(setter: 'setItems')]
    private Collection $items;

    public function __construct()
    {
        $this->items = new ArrayCollection();
    }
realjjaveweb commented 8 months ago

The core issue at hand is that the default values of PHP constructor arguments are just that - yes, even if it's defined as the promoted property - only the property type and name definition is promoted to class level, not the default value, because technically it's a constructor default value. (Honestly I hope this will change in future PHP)

However PHP does provide ways how to check:

  1. if property is promoted: https://www.php.net/manual/en/reflectionproperty.ispromoted.php
  2. if parameter has default value available: https://www.php.net/manual/en/reflectionparameter.getdefaultvalue.php
  3. get the params's default value if available: https://www.php.net/manual/en/reflectionparameter.isdefaultvalueavailable.php

So it certainly is implementable and would be appreciated by many, myself included. It makes absolutely no sense to me that for serialization you can define (not)serializing null behavior, but not for deserialization. :shrug:

Option 2 is to simply provide e.g. a PHP attribute to define default values for deserialization.

iKsSs commented 6 months ago

Hi all!

I created a wrapper for JMS Serializer (a temporary solution till JMS supports promoted props) which

Serializer implements JMS\Serializer\ArrayTransformerInterface and JMS\Serializer\SerializerInterface interfaces. The code is compliant with PHPStan level 8. It was written in Symfony 6.4 and it should work in Symfony 7.x however, it should be easily adaptable to other frameworks.

Check out the implementation: https://gist.github.com/iKsSs/f048fd04f28c638a5c3d8b4390976d25

scyzoryck commented 5 months ago

Coming back to this issue.

@mpragliola Are you using DefaultValuePropertyDriver? The case that you are describing in the first post seems to be working fine for me. See BaseSerializationTestCase::testConstructorPromotionWithDefaultValues test case that is covering exactly what you described :)

markuspoerschke commented 5 months ago

We experienced the same problem when using the JSM Serializer Bundle with Symfony Framework.

Adding the following config solved the issue for us.

jms_serializer:
    default_value_property_reader_support: true
dmaicher commented 5 months ago

We experienced the same problem when using the JSM Serializer Bundle with Symfony Framework.

Adding the following config solved the issue for us.

jms_serializer:
    default_value_property_reader_support: true

@markuspoerschke this indeed fixes our issue :blush: Thanks! @mpragliola let's close here