Typed property X must not be accessed before initialization

[tmarsteel]

Q	A
Bug report?	yes
Feature request?	no
BC Break report?	no
RFC?	no

This topic has come up in #1336 before, but in the context of "this is an issue with 3.14.0-rc1", not "please make this work".
So here goes: please make this work. I would really like to have the strong safety of typed properties.

Steps required to reproduce the problem

Afaict, this affects all JMS versions in PHP 7.4+.

class Foo {
  public $pre74property;
  public string $initialized74property = 'present';
  public ?string $uninitialized74property;
}

$serializer = (new JMS\Serializer\SerializerBuilder())->build();
$context = new JMS\Serializer\SerializationContext();
$context->setSerializeNull(true);

echo $serializer->serialize(new Foo(), 'json', $context);

Expected Result

{
  "pre74property": null,
  "initialized74property": "present",
  "uninitialized74property": null
}

or, with setSerializeNull(false):

{
  "initialized74property": "present"
}

Actual Result

PHP Fatal error:  Uncaught Error: Typed property Foo::$uninitialized74property must not be accessed before initialization in /home/tmarstaller/Desktop/jmstest/vendor/jms/serializer/src/Accessor/DefaultAccessorStrategy.php:89
Stack trace:
#0 /home/tmarstaller/Desktop/jmstest/vendor/jms/serializer/src/Accessor/DefaultAccessorStrategy.php(94): Foo::JMS\Serializer\Accessor\{closure}()
#1 /home/tmarstaller/Desktop/jmstest/vendor/jms/serializer/src/GraphNavigator/SerializationGraphNavigator.php(258): JMS\Serializer\Accessor\DefaultAccessorStrategy->getValue()
#2 /home/tmarstaller/Desktop/jmstest/vendor/jms/serializer/src/Serializer.php(252): JMS\Serializer\GraphNavigator\SerializationGraphNavigator->accept()
#3 /home/tmarstaller/Desktop/jmstest/vendor/jms/serializer/src/Serializer.php(163): JMS\Serializer\Serializer->visit()
#4 /home/tmarstaller/Desktop/jmstest/test.php(11): JMS\Serializer\Serializer->serialize()
#5 {main}
  thrown in /home/tmarstaller/Desktop/jmstest/vendor/jms/serializer/src/Accessor/DefaultAccessorStrategy.php on line 89

---

There is ReflectionProperty#isInitialized, which does exactly whats needed here:

class Foo {
  public string $property;
}
$reflectionProperty = (new ReflectionClass(Foo::class)->getProperty('property'));
var_dump($reflectionProperty->isInitialized(new Foo));

bool(false)

---

Also, there is the workaround of giving them a default value of null. But i'd prefer not to do this.

[Tobion]

I agree that unitialized properties should not be serialized by default.
But I also think that this has nothing to do with $context->setSerializeNull(true);. Unititialized properties are not null.
There could be a new option like $context->setSerializeUninitialized(false);. But again, I don't think there is any use-case trying to serialize unititialized properties. I would always fail. So there is not point in having this option and instead it should behave like this by default.

[tmarsteel]

Good point on that nulls and unitialized properties are different things. But i think a configuration option is still handy; maybe it would be $context->setFailOnUninitialized(false);. It boils down to whether you want an error for not initializing your objects properly or whether you want to consider "some properties not initialized" as proper.

[Tobion]

Sure but it's not a serializers job to do that validation. So I don't think such option is really necessary.

[re2bit]

I think adding this Option would add Value to this Serializer. Since this Serializer is low level Library and could be used for internal purposes, not fully initialised Objects are possible.
In a perfect world a not initialised Object should not be present, but the PHP World is not perfect.
I added this draft Pull Request and would love to receive your Feedback.

[goetas]

I personally think that both

{
  "pre74property": null,
  "initialized74property": "present",
  "uninitialized74property": null
}

and

{
  "initialized74property": "present"
}

are wrong.

if public ?string $uninitialized74property; was supposed to be null, then it should be declared NULL in the property definition.
For the same reason PHP triggers the error if you try to access that property.

[goetas]

The only "acceptable" thing that the serializer could do here is to skip always the serialization of uninitialized props (as suggested by @Tobion), but that would force us to use always the reflection accessor with obvious performance penalty compared to the closure accessor.

Another option is to have a global config parameter that skips those props, in this way the user explicitly opts-in for the feature (and the related performance penalty)

[douglasjam]

This error also affects constructor-promoted attributes, even though it has a default initial value when nullable it will crash.
By that, we benefit 0 of this when using PHP>8 versions when deserializing DTOs whose attributes are not mentioned.

I see the performance penalty, but some users do like the benefit of writing less code, and this possibility to enable it would be nice.

https://wiki.php.net/rfc/constructor_promotion

class MyClass {
    public function __construct(private ?string $prop = null) {}

   public function getProp(): ?string {
      return $this->prop;
   }
}

[Tobion]

The problem @douglasjam described is different from whether uninitialized properties should be serialized.
The observed behavior is because the serializer does not call the constructor on deserialization. So the default value (null in this case) is not applied. With constructor promotion this will indeed become more of a problem.
I don't see an easy solution for this. Either the serializer needs to be able to call the constructor or it could automatically initialize optional properties with the default values based on the constructor.

[Tobion]

The problem @douglasjam described is different from whether uninitialized properties should be serialized.
The observed behavior is because the serializer does not call the constructor on deserialization. So the default value (null in this case) is not applied. With constructor promotion is will indeed become more of a problem.

[pag1903]

that would force us to use always the reflection accessor with obvious performance penalty compared to the closure accessor.

May be that the solution implemented in Sf PropertyAccessor could be an interesting option without performance issue.

[joaojacome]

This one here would cover the constructor promotion when there's a default value
#1417

[gremo]

Don't know how to solve this bug with nested collections. Constructor is not invoked and property isn't promoted. Default value not assignable (I can't do new ArrayCollection() if not inside the constructor):

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

    public function __construct()
    {
        $this->items = new ArrayCollection();
    }

[eerison]

I'm facing the same issue :/

[afraca]

Just for the record, I know I'm late to the party, but we solved this issue with the PreSerialization attribute, and initialize the properties there. (@gremo )