Property hooks do not call parent's magic __get and __set methods when referencing parent property

[ericnorris]

Description

I was playing around with property hooks and noticed the following (in my opinion) inconsistency when combining property hooks with inheritance and __get and __set:

• https://3v4l.org/Y27mB
• https://3v4l.org/Q2OAj

The following code (for __get, the same applies for __set in https://3v4l.org/Q2OAj):

<?php

class ParentClass {
    public function __get(string $name) {
        echo "__get($name) was called\n";
    }
}

class ChildClass extends ParentClass {
    public $foo {
        get => parent::$foo::get();
    }
}

(new ParentClass)->foo;
(new ChildClass)->foo;

Resulted in this output:

__get(foo) was called

Fatal error: Uncaught Error: Undefined property ParentClass::$foo in /in/Y27mB:13

But I expected this output instead:

__get(foo) was called
__get(foo) was called

I tried searching the RFC and existing PHP issues, but I haven't found this example - apologies if I missed it. Barring something saying that this shouldn't work, I would expect it to.

PHP Version

PHP 8.4.3

Operating System

No response

[bwoebi]

parent::$prop::get() was meant to explicitly call a hook function. There's no hook function, but a __get function, which is not quite the same. To actually call these, parent::__get($prop) would be the right thing to do.

[iluuu1994]

I agree. It's honestly not something that we explicitly considered, but I agree that parent::__get($prop); is a reasonable workaround. /cc @Crell If you agree, we can close this issue.

[ericnorris]

Thanks for the quick response! The manual currently says (emphasis mine):

Accessing parent hooks
A hook in a child class may access the parent class's property using the parent::$prop keyword, followed by the desired hook.
...
If there is no hook on the parent property, its default get/set behavior will be used.

It also gives an example of referencing the parent's property (not hook):

<?php
class Strings
{
    public string $val;
}

class CaseFoldingStrings extends Strings
{
    public bool $uppercase = true;

    public string $val {
        get => $this->uppercase
            ? strtoupper(parent::$val::get())
            : strtolower(parent::$val::get());
    }
}
?>

So I don't see how this is parent-hook-specific, since you can access plain properties. And if you can access a plain property, you should be able to access a private or non-existent one via __get.

That said, if the workaround approach is preferred, I think at the least the error messaging could be changed. My mental model is that you won't see a Undefined property error with __get, which is part of what made this surprising. If parent::$prop::get is invalid here, maybe we could throw an error specifically about that?

[iluuu1994]

If there is no hook on the parent property, its default get/set behavior will be used.

Right. This sentence implies that the parent declares such a property, but without a hook for the given operation, in which case we'll resort to accessing the properties backing store. The idea of this fallback is to allow adding hooks to child properties in a backwards compatible way, i.e. by avoiding direct access of the backing value that would break if the parent were to add a hook itself (or more precisely, it would circumvent the parents hook). __get/__set are outside of the scope of what we intended for this fallback.

I don't think your expectation for this to work is unreasonable, but it wasn't documented, and thus is technically a language change. I personally don't think we need this fallback, and it might not be quite straight forward to implement. But if you feel strongly about this topic, feel free to ask for opinions on the internals mailing list.

If parent::$prop::get is invalid here, maybe we could throw an error specifically about that?

Can you make a concrete suggestion?

[ericnorris]

This sentence implies that the parent declares such a property, but without a hook for the given operation, in which case we'll resort to accessing the properties backing store.

I can see where you're coming from, but to me it just implies "it will try to fetch the property normally", where normally means "it will follow the existing property access patterns", and hence why I'd expect __get to work.

I don't think your expectation for this to work is unreasonable, but it wasn't documented [...] if you feel strongly about this topic, feel free to ask for opinions on the internals mailing list.

I don't think I feel too strongly, but yeah I might raise it there; I think it'd be nice to either be consistent, or more strongly indicate that this will only work for existing properties or hooks. On that note...

If parent::$prop::get is invalid here, maybe we could throw an error specifically about that?

Can you make a concrete suggestion?

This is tough, because in a sense there is an "Undefined property", it's just that if you were accessing this undefined property outside of the class, it would work. It's only here that it doesn't, so maybe something like:

Error: ParentClass::$foo::get() must refer to a defined property in /in/Y27mB:13

..."defined" here could also be "named", "explicit", or anything that makes it clear that this property must actually exist in some form.

I think I'd be satisfied with this, because we're calling out that this language construct (ParentClass::$foo::get()) has a requirement to refer to real properties, so this isn't an "undefined property error", it's a ParentClass::$foo::get() error. Does that make sense?

[Crell]

Mm, yeah, we never really thought about this case. I would not be opposed to falling back to ParentClass::__get() if it's easy enough to do. If it would be hard, I don't think it's a large issue in practice to just leave it. Ideally, in a hook-filled world __get gets exponentially less common anyway.

[iluuu1994]

I can see where you're coming from, but to me it just implies "it will try to fetch the property normally", where normally means "it will follow the existing property access patterns", and hence why I'd expect __get to work.

Just to clarify:

If there is no hook on the parent property, its default get/set behavior will be used.

This section explicitly refers to a declared parent property. So, it's not really a matter of interpretation. However, the case where the parent property is not declared is unspecified. It should have been explicitly declared whether this will error, or calling magic methods.

That said, I believe calling magic methods would also not be consistent, as accessing properties from the parent class will actually use the child property if declared there, rather than calling magic methods. E.g. https://3v4l.org/06HfX

class P {
    public function setProp() {
        $this->prop = 'foo';
    }
    
    public function __set($name, $value) {
        var_dump('__set', $name, $value);
    }
}

class C extends P {
    public $prop;
}

$c = new C();
$c->setProp();
var_dump($c);

Furthermore, if the parent property does not contain magic get/set, does that mean we're resorting to dynamic properties? That's would inevitably clash with non-virtual child properties. Similarly, what happens with __get/__set when the same property is accessed again within __get/__set? This also normally resorts to dynamic properties.

It's not obvious to me what the correct, consistent behavior is here.

[ericnorris]

This section explicitly refers to a declared parent property. So, it's not really a matter of interpretation.

I appreciate that the intent was to refer to declared parent properties, and I agree that the intent is not up for interpretation either! However, when I read that section, it didn't say declared, just "parent property", and mistakenly or not I walked away from that paragraph thinking that I could do something equivalent to ->foo on the parent, just like I could on any given object. I think that might be the gist of it - I was thinking in terms of "property access" and not "defined properties", which I tend to do because of the dynamic nature of PHP.

Mainly I want to stress that I think that the nomenclature we use isn't necessarily straightforward, so maybe there's room for improvement here. I don't think I'm off the mark with this observation since property hooks themselves introduce virtual properties, adding to the mix. That said, I also want to say that I appreciate the time and effort that went into property hooks and their documentation, and this is not meant as criticism.

That said, I believe calling magic methods would also not be consistent, as accessing properties from the parent class will actually use the child property if declared there, rather than calling magic methods. E.g. https://3v4l.org/06HfX

I actually think your example is consistent, at least with my mental model. $this refers to the current instance of the class, not necessarily the class $this appears in, and so $this->prop refers to whatever $prop means on $this, not in P.

I would say that is different than the code I gave in my example, because parent:: explicitly refers to the parent class and not $this.

Furthermore, if the parent property does not contain magic get/set, does that mean we're resorting to dynamic properties?That's would inevitably clash with non-virtual child properties.

We could, and maybe that clash would be okay?

Similarly, what happens with __get/__set when the same property is accessed again within __get/__set? This also normally resorts to magic methods.

I didn't quite follow this one - what happens with __get in the parent when the same property is accessed again within __get? You said it normally resorts to magic methods, but I thought that it actually triggers a warning.

That said, I appreciate all the edge cases here, so:

It's not obvious to me what the correct, consistent behavior is here.

I'm fine with continuing to require it to be a declared / defined property, especially since that could always be relaxed in the future. Did you have an opinion about my suggestion for an error message like ParentClass::$foo::get() must refer to a defined property in /in/Y27mB:13?

[iluuu1994]

This also normally resorts to magic methods.

Oops, this should have said "dynamic properties".

[iluuu1994]

Sorry for the delay.

However, when I read that section, it didn't say declared, just "parent property"

Correct. Technically, magic methods don't involve properties, rather they are a fallback for properties. Anyway, terminology aside:

Furthermore, if the parent property does not contain magic get/set, does that mean we're resorting to dynamic properties? That's would inevitably clash with non-virtual child properties.

We could, and maybe that clash would be okay?

This is problematic because the engine cannot differentiate between the declared and dynamic property anymore. We would need more complex mechanisms to disambiguate, and I don't think this is a good area to invest energy into.

Did you have an opinion about my suggestion for an error message like ParentClass::$foo::get() must refer to a defined property in /in/Y27mB:13?

Isn't this essentially just a more properly worded sentence for "Undefined property ParentClass::$foo"? To really explain, we would have to add something like "rather than dynamic or magic method property", but that's quite a mouthful. Error message hints would be nice, like compilers have. 🙂