doctrine / instantiator

https://www.doctrine-project.org/projects/instantiator.html
MIT License
10.94k stars 60 forks source link

Promoted property causing Error #86

Closed garak closed 1 year ago

garak commented 3 years ago

A value object, mapped as embeddable, perfectly working:

<?php
class Foo {
    private ?string $bar = null;
    public function __construct(?string $bar) {
        $this->bar = $bar;
    }
}

But if you try to promote property:

<?php
class Foo {
    public function __construct(private ?string $bar = null) {
    }
}

It fails with Error: Typed property Foo::$bar must not be accessed before initialization as soon as you try to access property.

Property is not mapped.

Edit: tried to downgrade to Doctrine 2.8 to check if could be a problem with 2.9: no changes.

jvasseur commented 3 years ago

This is because your promoted property example is equivalent to

<?php
class Foo {
    private ?string $bar;
    public function __construct(?string $bar = null) {
        $this->bar = $bar;
    }
}

thus the property ends up without a default value and since Doctrine skip the constructor when loading objects from the database the property isn't initialized.

garak commented 3 years ago

thus the property ends up without a default value and since Doctrine skip the constructor when loading objects from the database the property isn't initialized.

Are you sure? That doesn't make sense at all (for PHP)

greg0ire commented 3 years ago

See https://www.doctrine-project.org/projects/doctrine-instantiator/en/latest/index.html Maybe a test case should be added to that project?

jvasseur commented 3 years ago

thus the property ends up without a default value and since Doctrine skip the constructor when loading objects from the database the property isn't initialized.

Are you sure? That doesn't make sense at all (for PHP)

This makes a lot of sens if you want users of the ORM to do what they want in the constructor, this could be requiring arguments to be passed to the constructor (were the ORM wouldn't know what to pass) or even triggering side effects in the constructor.

garak commented 3 years ago

I mean: it doesn't make sense that PHP disallow putting a default value in a (promoted) property when such null value is required.

jvasseur commented 3 years ago

Well, desugaring the default value as the argument default value instead of the property default value makes sense since it allows omitting it when calling the constructor.

The default value could be desugared as a default value in both the property and the constructor but the RFC explains why this could cause problems in the future.

garak commented 3 years ago

But I can't understand why they didn't consider the special case of nullable variables, where null must be a default. Anyway, if I understand correctly, the only solution here is avoiding promotion.

jvasseur commented 3 years ago

Well in most case it wouldn't be a problem since the constructor would set the property and thus eliminate the need to set a default value on it. It's only a problem in doctrine where constructor is skipped during object creation.

derrabus commented 2 years ago

Can we close this issue? I don't think it's actionable for us.