Property Hooks - reflectionProperty::getInitialized

Description

The following code:

<?php

class PropertyHooks
{
    public ?string $backedGetOnly {
        get => $this->backedGetOnly;
    }

    public ?string $backedSetOnly {
        set (?string $value) {
            $this->backedSetOnly = $value;
        }
    }

    public ?string $backedGetAndSet {
        set (?string $value) {
            $this->backedGetAndSet = $value;
        }

        get => $this->backedGetAndSet;
    }
}

$propertyHooks = new PropertyHooks();

$reflectionProperty = new ReflectionProperty($propertyHooks, 'backedGetOnly');

$reflectionProperty->isInitialized($propertyHooks); // returns true - I would expect false
                                      // backSetOnly reports  false
                                      // backedGetAndSet reports true - I would also expect false

if ($reflectionProperty->isInitialized($propertyHooks)) {
    $reflectionProperty->getRawValue($propertyHooks); // Uncaught exception 'Error' with message Typed property PropertyHooks::$backedGetOnly must not be accessed before initialization
} else {
    echo 'not initialized';
}

Resulted in this output:

Fatal error: Uncaught Error: Typed property PropertyHooks::$backedGetOnly must not be accessed before initialization in /in/KXI3l:33
Stack trace:
#0 [internal function]: PropertyHooks->$backedGetOnly::get()
#1 /in/KXI3l(33): ReflectionProperty->getRawValue(Object(PropertyHooks))

But I expected this output instead:

not initialized

ReflectionProperty::isInitialized() seems to be based on whether or not there's a get hook instead of the backing-value?
If that is expected behavior, should we also have a isRawValueInitialized() method?

(isInitialized() returns true for virtual properties)

PHP Version

PHP 8.4 (master - 2024-09-01)

Operating System

OSX & 3v4l

@iluuu1994

Maybe @iluuu1994 or @Crell can clarify?

This is a oversight. I agree that ReflectionProperty::isInitialized() should refer to the properties backing value. Semantically, hooked properties (with a get operation) cannot be uninitialized, as a function cannot return an uninitialized value. So there would be no point in checking for it.

@Crell

That does pose the question what isInitialized should return for virtual properties. There are only two sensible options:

Always return true, even though they have no concept of being initialized.
Throw, which makes ReflectionProperty::isInitialized() conditionally usable.

Not a fan of either of those. @Crell Thoughts?

That does pose the question what isInitialized should return for virtual properties.

in the sense that ReflectionProperty::isInitialized() serves as check of whether or not one can access the property (via both the obj and reflection)

$obj->prop, or ReflectionProperty::getValue($obj). both will work... it's "initialized"
it's already documented that ReflectionProperty::getRawValue() will throw an exception for virtual properties

returning true (apparent current behavior) makes as much sense as anything

in the sense that ReflectionProperty::isInitialized() serves as check of whether or not one can access the property

Under that definition, the current behavior is actually correct for backed properties too, since it's the responsibility of the getter to ensure not to access the uninitialized backing value. Otherwise, lazy getters would not be safe to access, according to isInitialized.

I added the qualifier: "(via both the obj and reflection)"

as getRawValue() throws the "must not be accessed before initialization" : not initialized

it's a very "loose" interpretation of what it means to be "initialized" 😊

Right. What I was trying to imply is that reflection can't know whether it's safe to call get on a property with an uninitialized backing value, as it entirely depends on its implementation. I.e. a lazy get may initialize the backing value during its execution. Hence, a "check of whether or not one can access the property" can't be implemented for backed properties. We can however check whether its backing value is set, but that is not meaningful for virtual properties. So essentially, either we have to throw, or use a different definition of what the function does for backed and unbacked properties.

Sorry if I'm being pedantic about terminology. I'm also trying to figure out what the correct behavior is.

(Thinking aloud...)

The user-expected behavior, I would think, is that "if it's initialized I can use it, if it's not initialized I can't." (We don't need to worry about readonly at the moment and its implications.) So we should try to get as close to that as possible. However, Ilija is very correct that there are ample cases where we cannot tell that directly. Any lazy-cached property would have that issue:

class Person {
  public string $fullName { get => $this->lastName ??= "$this->first $this->last"; }
}

That has an uninitialized backing value, but reading it is perfectly safe. OTOH, even a virtual property could be written such that the expectation doesn't hold.

class Person {
  public string $fullName { get => "$this->first $this->last"; }
}

There's no backing value, but it will still fail if $first or $last are not initialized.

So no matter what we do, there's going to be some situations where isInitialized() will be unreliable. The question is how do we minimize that.

If we think of hooks as methods, then we get an expectation of "calling the method is fine, but whether the method works or not is anyone's guess." Which would suggest always returning true, and it's the property's responsibility to not break. That would be somewhat consistent with the behavior if explicit getFoo() methods are defined. (There is no initialized for a method, so you have to just trust the method's return value is type safe and it doesn't access uninitialized values internally.)

On the flip side, what's the use case for basing it on a backing value? The backing value should be opaque, by design. In what situations would you want to determine the initialization status of a backed property's internal store? That feels like trying to access the status if a private property. Is there a use case for doing so? The only one I can think of is some kind of external lazy initialization, but... I cannot think of where you'd do that, and the lazy-cache pattern above isn't superior in every way.

So I think I'm leaning towards the status quo (the presence of a get hook always returns initialized true), and saying if that doesn't hold, the get hook is buggy and should feel bad.

(A valid use case for external determination would possibly change my view there, if one can be found.)

the presence of a get hook always returns initialized true

That's not quite the current behavior. We currently actually invoke get, which isn't really necessary since it can't return false anyway.

But I tend to agree.

In what situations would you want to determine the initialization status of a backed property's internal store? That feels like trying to access the status if a private property. Is there a use case for doing so?

There is in the kind of workload where you use reflection, which is why reflection currently does allow you to access private properties with ReflectionProperty::getValue/setValue

I can already tell if it's initialized by casting the object to an array, and var_dump also shows the backing info: https://3v4l.org/G21gs/rfc#vgit.master

It's just annoying that I can no longer just assume initialized properties will show up in the array cast and means I'm going to need more logic for properties with get hooks, so if we're counting votes from random people on the internet please put mine in the "Have it refer to the backing value" column.

That does pose the question what isInitialized should return for virtual properties. There are only two sensible options:

Always return true, even though they have no concept of being initialized.

Throw, which makes ReflectionProperty::isInitialized() conditionally usable.

2 is already the case with some reflection methods, for example: https://3v4l.org/vS9Xu/rfc#vgit.master so I'd say that's the more consistent way to go

Edit: Or you could change the return type to ?bool but that might break stuff if they check === false so it'd have to be put off until another major version, but you'd want to return false in the meantime not to break simple ifs in the future

heads up: working on a minimal reproducable example, but I'm consistently getting a

zend_mm_heap corrupted
Abort trap: 6

no idea if related to this open issue or not

seems to involve having a class with properties having hooks and properties with asymmetric visibility and reflecting on the properties...

edit: it's an issue with xdebug... will open issue with them!

@bkdotcom

@bkdotcom Dynamic properties? https://3v4l.org/L6cMa/rfc#vrfc.property-hooks There was a bugfix just after beta 4 released #15721 If that's not it ignore me

@jnvsor

@jnvsor I jumped the gun mentioning it here.... looks to be an xdebug issue.
ignore me!

So, the change I'm going to make here, if everybody is ok with it:

getInitialized() will no longer invoke the get hook.
If the property is virtual, getInitialized() will always return `true.

Does that make sense?

@iluuu1994

@iluuu1994 I presume we're talking only about virtual properties yes? In that case I'd be fine with it. Just so long as we have a way to check the backing value that works.

The first bullet-point refers to all properties with hooks. isInitialized() currently invokes get when present. Instead, backed properties will inspect the backing value, while virtual properties will always return true.

Sounds good to me 👍

The fix is here: #15822 I misunderstood the original report. isInitialized() doesn't call get, but it would just skip the actual "is the value uninitialized" check when any hooks are present. So the fix is much closer to the current behavior than I thought, which is good.

Fix ReflectionProperty::isInitialized() for hooked props

Navigation Menu

Repository navigation

Property Hooks - reflectionProperty::getInitialized #15694

Description

PHP Version

Operating System

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Property Hooks - reflectionProperty::getInitialized #15694

Description

Description

PHP Version

Operating System

Activity

cmb69 commented on Sep 1, 2024

iluuu1994 commented on Sep 1, 2024

iluuu1994 commented on Sep 1, 2024

bkdotcom commented on Sep 1, 2024

iluuu1994 commented on Sep 1, 2024

bkdotcom commented on Sep 1, 2024

iluuu1994 commented on Sep 1, 2024

Crell commented on Sep 2, 2024

iluuu1994 commented on Sep 2, 2024

jnvsor commented on Sep 5, 2024

bkdotcom commented on Sep 5, 2024

jnvsor commented on Sep 5, 2024

bkdotcom commented on Sep 5, 2024

iluuu1994 commented on Sep 8, 2024

jnvsor commented on Sep 8, 2024

iluuu1994 commented on Sep 8, 2024

jnvsor commented on Sep 8, 2024

iluuu1994 commented on Sep 10, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions