With PHP 7.4, the hydrator breaks with required typed property

[pounard]

For example, in hydrate methods, this:

    if (isset($values['current_revision']) || $object->current_revision !== null && \array_key_exists('current_revision', $values)) {
        $object->current_revision = $values['current_revision'];
    }

will raise a "property cannot be accessed prior to initialization" if it's typed and required, because of the object->current_revision !== null statement.

This means that the generated code must vary depending on how the property was declared, for typed and required properties, default value must be determined during compilation and code must be generated otherwise.

For example, let's consider that all other use case keep the same code, but for typed required properties, if property has a default value:

    if (isset($values['current_revision']) || \array_key_exists('current_revision', $values)) {
        $object->current_revision = $values['current_revision'];
    }

Or:

    $object->current_revision = $values['current_revision'] ?? $object->current_revision;

And if it has none:

    if (isset($values['current_revision'])) {
        $object->current_revision = $values['current_revision'];
    }

Or:

    $object->current_revision = $values['current_revision'] ?? null;

Which also could be a runtime optimization (for the later) whereas it could be slower (for the former) but since now recent PHP versions have an optimised opcode for \array_key_exists() this probably wouldn't be visible.

[Ocramius]

I'd say that this needs to be started from integration test additions: properties not being initialized (yet being typed) seems to be a consumer-side bug though

[pounard]

properties not being initialized (yet being typed) seems to be a consumer-side bug though

Maybe, maybe not, you could imagine a scenario where objects are created using ReflectionClass::newInstanceWithoutConstructor(), for example while denormalizing objects from some Rest API call, or hydrating objects from a database.

[Ocramius]

for example while denormalizing objects from some Rest API call, or hydrating objects from a database.

An object should be in valid state after its instantiation: whether that instantiation is __construct or another mechanism, if the object is not in its declared state, that's a bug in its factory

[Ocramius]

Talking about partial initialization specifically, any access to un-initialized object state should case initialization before the actual read operation happens (see doctrine/orm 3.x proxies, where this is fixed)

[pounard]

An object should be in valid state after its instantiation: whether that instantiation is __construct or another mechanism

Yes, I agree, I was thinking about using this hydrator as being part of the instantiation mechanism, in my use cases. Where data is pre-validated before being sent to the hydrator.

[pounard]

Sorry, keyboard oops.

[pounard]

Talking about partial initialization specifically, any access to un-initialized object state should case initialization before the actual read operation happens (see doctrine/orm 3.x proxies, where this is fixed)

I would very much like avoiding using proxies, it would be over-engineering in most of the use cases I encounter on a daily basis. My goal is not to proceed to partial initialization. When we allow this in our API's, we documented the properties as not being required (and will type them as not being required as soon as we will use PHP 7.4).

Does it make sense to enforce this hydrator being more resilient, robust and avoid it crashing on such as easy fix ? I agree then that wrongly initialising the object by missing required properties is a domain related problem, not this hydrator's concern; nevertheless in the end, would it be valid to use this hydrator as being part of the factory that is supposed to correctly instantiate objects ?

[Ocramius]

Does it make sense to enforce this hydrator being more resilient, robust and avoid it crashing on such as easy fix ?

I think not, reason being that the process of hydration would return partial/incorrect data, while type information declares that the data should be there. I think adding typed properties only highlighted the presence of a bug there.

[pounard]

I think adding typed properties only highlighted the presence of a bug there

That's the whole goal of typed properties, I can only agree. But let's consider the following code:

class Foo {
    private int $a = 0;
    private ?int $b = 0;
    private int $c;
    private ?int $d;
}

$callback = \Closure::bind(function (Foo $object, array $values) {
    if (isset($values['a']) || $object->a !== null && \array_key_exists('a', $values)) {
        $object->a = $values['a'];
    }
    if (isset($values['b']) || $object->b !== null && \array_key_exists('b', $values)) {
        $object->b = $values['b'];
    }
    if (isset($values['c']) || $object->c !== null && \array_key_exists('c', $values)) {
        $object->c = $values['c'];
    }
    if (isset($values['d']) || $object->d !== null && \array_key_exists('d', $values)) {
        $object->d = $values['d'];
    }
}, null, Foo::class);


$foo = (new ReflectionClass(Foo::class))->newInstanceWithoutConstructor();

$callback($foo, [
    'c' => 12,
]);

This should be valid, because:

• Foo::$a is required but has a default value,
• Foo::$b is not required and has a default value,
• Foo::$c is required and has no default value, it sets it,
• Foo::$d is not required and has no default value.

In this scenario, as a user of this hydrator, I expect this code to pass, and expect it to:

• let Foo::$a with its default value (which is done by transparently with newInstanceWithoutConstructor()),
• set Foo::$b its default value (which is done by transparently with newInstanceWithoutConstructor()),
• set Foo::$c accordingly to my input,
• set Foo::$d to null.

Whereas as of today (PHP 7.4.1) I got this:

[pounard@guinevere] ~
 >  php -f pouet.php 
PHP Fatal error:  Uncaught Error: Typed property Foo::$d must not be accessed before initialization in /var/www/irpauto/aavant2.local/app/pouet.php:20
Stack trace:
#0 /var/www/irpauto/aavant2.local/app/pouet.php(29): Foo::{closure}(Object(Foo), Array)
#1 {main}
  thrown in /var/www/irpauto/aavant2.local/app/pouet.php on line 20

Line 20 is for the 'd' value, which means that PHP did set the default values and handled Foo::$c correctly, but misbehaved with the non required Foo::$d property which would be, in my opinion, transparently left to null (or explicitly set to null if no value is within the array).

Does it make sense ?

[Ocramius]

Oh, I see what you mean: yes, it makes sense, sorry for the misunderstanding!

[pounard]

Thanks, I'm sorry if wrongly expressed myself, I'm not a native english speaker (even worse, I'm french).

[Ocramius]

I was mostly focusing on extract(), but indeed totally forgot to consider hydrate(), which in 99% od cases is acting as a factory itself, not a language issue, but rather me forgetting about stuff I wrote 👍

[armpogart]

See also discussion regarding this issue on Cycle ORM repo where this library is considered and the issue is with extract() and not hydrate().