Setting the parent object when creating a new instance gives error with uninitialised property (PHP >=7.4)

[7ochem]

Environment

PHP 7.4
Symfony 4.4
Sonata Admin 3

Sonata packages

show

$ composer show --latest 'sonata-project/*'
sonata-project/admin-bundle 3.107.1 3.107.1

Symfony packages

show

$ composer show --latest 'symfony/*'
symfony/property-info   v4.4.31 v4.4.31 

PHP version

$ php -v
PHP 7.4.13 (cli) (built: Dec  1 2020 04:25:48) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
    with Zend OPcache v7.4.13, Copyright (c), by Zend Technologies

Subject

When Sonata Admin is setting a parent object on a child object when creating a new instance of the child object, Symfony Property Access gives an \Symfony\Component\PropertyAccess\Exception\AccessException because of an uninitialised property when having a typed non-nullable uninitialised property.

Steps to reproduce

I have a parent-child 1:n relation between two entities and also a parent and child admin configured. I recently updated my code to call addChild() on the parent admin with the second parameter set (to remove deprecations).

The child entity needs to have a parent entity set, so I have written my PHP code as such:

class Child {
    /**
     * @ORM\ManyToOne(targetEntity="App\Entity\ParentEntity", inversedBy="child")
     * @ORM\JoinColumn(onDelete="CASCADE", nullable=false)
     */
    private ParentEntity $parent;

    public function getParent(): ParentEntity { return $this->parent; }

    public function setParent(ParentEntity $parent): void { $this->parent = $parent; }
}

Parent could be something like:

class ParentEntity {
    /**
     * @var Collection<int, Child>
     * @ORM\OneToMany(targetEntity="App\Entity\Child", mappedBy="parent", cascade={"persist", "remove"}, orphanRemoval=true)
     */
    public Collection $children;

    /* public functions getChildren, setChildren, addChild, removeChild etc. */
}

In admin services definition for the parent entity admin:

        calls:
            - [addChild, ['@app.admin.child_admin', 'parent']]

Actual results

Now when Sonata Admin is setting the parent object when creating a new instance, Symfony Property Access gives an error because of an uninitialised property:

\Symfony\Component\PropertyAccess\Exception\AccessException
The property "App\Entity\Child::$parent" is not readable because it is typed "App\Entity\ParentEntity". You should initialize it or declare a default value instead.

Expected results

I do not want to make my properties nullable and initialise them with a null value. Neither do I want to initialise the property in the constructor with $this->parent = new ParentEntity();.

Sonata Admin should not need to get the property to set it or a least should not error out on this.

Possible solution

\Symfony\Component\PropertyAccess\Exception\AccessException is only being thrown when the property cannot be read because it has not been initialised. We could wrap the specific line that is responsible for this error with a try { } catch(AccessException $e):

-                $value = $propertyAccessor->getValue($object, $propertyPath);
+                try {
+                    $value = $propertyAccessor->getValue($object, $propertyPath);
+                } catch(\Symfony\Component\PropertyAccess\Exception\AccessException $e) {
+                    // Symfony PropertyAccess failed to get the value because the property is uninitialised. Set value to null.
+                    $value = null;
+                }

[VincentLanglet]

Actual results

Now when Sonata Admin is setting the parent object when creating a new instance, Symfony Property Access gives an error because of an uninitialised property:

\Symfony\Component\PropertyAccess\Exception\AccessException
The property "App\Entity\Child::$parent" is not readable because it is typed "App\Entity\ParentEntity". You should initialize it or declare a default value instead.

Expected results

I do not want to make my properties nullable and initialise them with a null value. Neither do I want to initialise the property in the constructor with $this->parent = new ParentEntity();.

Sonata Admin should not need to get the property to set it or a least should not error out on this.

With your code, if I do (new Child())->getParent(), I ends up with the same error.
So in some way, your code is not valid...

Does Sonata is supposed to handle non valid code ? It's debatable...
You're basically asking us to try catch and fallback to null, because you don't want your getter to return null, even if the current value is null.

Having the stack trace could be interesting to know which call is failing.
If I understand correctly, it's in the method appendParentObject ?

$value = $propertyAccessor->getValue($object, $parentAssociationMapping);
if (\is_array($value) || $value instanceof \ArrayAccess) {
    $value[] = $parentObject;
    $propertyAccessor->setValue($object, $parentAssociationMapping, $value);
} else {
    $propertyAccessor->setValue($object, $parentAssociationMapping, $parentObject);
}

Accessing the value is currently our way to know if the value is a OtO or OtM.
If in your case, we fallback in the else, it still won't solve the following code

class Child {
    private Collection $parents;

    public function getParents(): Collection { return $this->parents; }

    public function setParent(Collection $parents): void { $this->parents; }
}

Here, calling setValue($object, $parentAssociationMapping, $parentObject) would be wrong.
So if we would like to avoid any error, a try/catch should be added with a return in the catch.
You'll have to set the parent by yourself then.

[7ochem]

Yes it is in the code of the method appendParentObject(). Specifically that part that you mention:

$value = $propertyAccessor->getValue($object, $parentAssociationMapping);
if (\is_array($value) || $value instanceof \ArrayAccess) {
    $value[] = $parentObject;
    $propertyAccessor->setValue($object, $parentAssociationMapping, $value);
} else {
    $propertyAccessor->setValue($object, $parentAssociationMapping, $parentObject);
}

With your code, if I do (new Child())->getParent(), I ends up with the same error.
So in some way, your code is not valid...

Does Sonata is supposed to handle non valid code ? It's debatable...

I think it is unfair to say my code is not valid. I think it is a perfectly fine use case.

You're basically asking us to try catch and fallback to null, because you don't want your getter to return null, even if the current value is null.

Correct 😄 Except that the current value is not null but just not yet set.

If in your case, we fallback in the else, it still won't solve the following code

class Child {
    private Collection $parents;

    public function getParents(): Collection { return $this->parents; }

    public function setParent(Collection $parents): void { $this->parents; }
}

Here, calling setValue($object, $parentAssociationMapping, $parentObject) would be wrong.

True, but that is not the case I'm trying to cover. That's a different case. If you would write that as below, it also would still go wrong because $value in that piece of code will also not be an array/collection.

class Child {
    private ?Collection $parents = null;

    public function getParents(): ?Collection { return $this->parents; }

    public function setParents(Collection $parents): void { $this->parents = $parents; }
}

The goal of the method is to set the parent object and within it is needs to determine whether or not it needs to set it or append it and it does so by getting the current value. The setting of the parent can go wrong in many ways because of "non valid code". I think within the means of the method to determine something it should prevent errors and/or cover possible use cases.

In my code I use this on the other side of the relation btw. so in that case it still isn't nullable, but it is initialised with an empty collection:

class ParentEntity {
    /**
     * @var Collection<int, Child>
     * @ORM\OneToMany(targetEntity="App\Entity\Child", mappedBy="parent", cascade={"persist", "remove"}, orphanRemoval=true)
     */
    public Collection $children;

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

    /* public functions getChildren, setChildren, addChild, removeChild etc. */
}

[VincentLanglet]

True, but that is not the case I'm trying to cover.

We're suppose to support every case, not just yours. What I meant is that, I'd like to avoid adding a fallback `If we cannot know if the data is a OtO or a OtM, we consider it's a OtO.

But seems like it's already the behavior for

/** @var Collection */
private $parents;

so we it's seems reasonable to keep this way.

Feel free to open a PR to catch UninitializedPropertyException

[7ochem]

I've created pull request #7705 to fix this issue.