UnitializedPropertyException when using nullable UploadableField as promoted property

[tjveldhuizen]

Bug Report

Q	A
BC Break	no
Bundle version	2.2.0
Symfony version	6.3.7
PHP version	8.2.12

Summary

Currently it's not possible to use a combination of promoted properties and nullable files in an entity class. The InjectListener checks if a filename is available to build the path to the file, when filename is NULL, it obviously does not resolve the path to the file, so it doesn't set the file property to null. In that situation, the property is uninitialized, which make PHP throw an UnitializedPropertyException as soon as I use ->getFile() on the entity retrieved from the database.

When the property is a 'regular' property, everything works fine.

Does not work:

use Doctrine\ORM\Mapping as ORM;
use Symfony\Component\HttpFoundation\File\File;
use Symfony\Component\HttpFoundation\File\UploadedFile;
use Vich\UploaderBundle\Mapping\Annotation as Vich;

#[ORM\Entity]
class Foo {
    #[ORM\Column(type: 'string', nullable: true)]
    private ?string $fileName = null;

    public function __construct(
        #[Vich\UploadableField('fooupload', fileNameProperty: 'fileName')]
        private ?File $file = null,
    ) {}

    public function getFile(): ?File
    {
        return $this->file;
    }
} 

Works:

use Doctrine\ORM\Mapping as ORM;
use Symfony\Component\HttpFoundation\File\File;
use Symfony\Component\HttpFoundation\File\UploadedFile;
use Vich\UploaderBundle\Mapping\Annotation as Vich;

#[ORM\Entity]
class Foo {
    #[ORM\Column(type: 'string', nullable: true)]
    private ?string $fileName = null;

    #[Vich\UploadableField('fooupload', fileNameProperty: 'fileName')]
    private ?File $file = null;

    public function __construct(?File $file = null) {
        $this->file = $file;
    }

    public function getFile(): ?File
    {
        return $this->file;
    }
} 

In this position, it might be possible to explicitly set the property to null when $path is null (and the property is nullable and not set) https://github.com/dustin10/VichUploaderBundle/blob/18c69764b767ccd71436331ca61480c9a3c1e56c/src/Injector/FileInjector.php#L24-L26

While debugging the mentioned situation, I think I also saw a problem like this in the CleanListener, but I'm not able to reproduce that at the moment.

[garak]

I don't get what's the difference between the two cases you showed, in terms of properties: in both cases, I see that the properties have the same types and the same predefined values. Moreover, if you're not able to propose a solution in the bundle codebase, I guess we should conclude that the problem is PHP itself, so impossible to solve on our end.

[tjveldhuizen]

After further investigation, I discovered that it is qualified as a PHP feature that in case of promoted properties the default value (null in this case) does not belong to the property, but to the parameter in the constructor, as described here and referenced here. The script (*) below confirms that the property is not set after instantiation via reflection.

This makes that the user has to explicitly set the property to null if the class is instantiated via the reflection method which does not execute the constructor, as this bundle does afaik. It confirms the fact that this bundle is not compatible with promoted properties for the file property.

Given the PHP RFC content, I don't think it will be changed on their end. I think the change in this bundle are small, but I haven't checked the impact on other classes. When adding something like this at the location in the original post (and fixing the type constraints of the setFile method), I think the issue would be resolved. (I can create a PR if this kind of solution is OK for you, of course)

        if (null !== $path) {
            $mapping->setFile($obj, new File($path, false));
+        } else {
+            $mapping->setFile($obj, null);
        }

(*) Used test script

<?php

class Foo
{
    public ?string $test = null;

    public function __construct(
        ?string $test = null
    ) {
        $this->test = $test;
    }
}

class Bar
{
    public function __construct(
        public ?string $test = null
    ) {}
}

echo "\n=== FOO ===============================================================\n";
$fooRefl = new ReflectionClass(Foo::class);
$fooInst = $fooRefl->newInstanceWithoutConstructor();
var_dump($fooInst->test);

echo "\n=== BAR ===============================================================\n";
$barRefl = new ReflectionClass(Bar::class);
$barInst = $barRefl->newInstanceWithoutConstructor();
var_dump($barInst->test);

tjv@5461f0b4a67f:/app$ php test.php

=== FOO ===============================================================
NULL

=== BAR ===============================================================

Fatal error: Uncaught Error: Typed property Bar::$test must not be accessed before initialization in /app/vich_uploader_bundle_1411/test.php:29
Stack trace:
#0 {main}
  thrown in /app/test.php on line 29

[garak]

I can create a PR if this kind of solution is OK for you

sure, go ahead