[RFC] Property hooks

[iluuu1994]

https://wiki.php.net/rfc/property-hooks

[arnaud-lb]

Since hooks never receive by ref, we may be able to optimize ops to their non-ref variants like we do for known functions. E.g. for this code:

    public mixed $x {
        set => parent::$x::set($value['x']);
    }

we generate this:

0000 CV0($value) = RECV 1
0001 INIT_PARENT_PROPERTY_HOOK_CALL 1 string("x") 1
0002 CHECK_FUNC_ARG 1
0003 V1 = FETCH_DIM_FUNC_ARG CV0($value) string("x")
0004 SEND_FUNC_ARG V1 1
0005 DO_FCALL
0006 RETURN null

and it should optimize to something like this once the optimizer knows that hooks do not receive by ref:

0000 CV0($value) = RECV 1
0001 INIT_PARENT_PROPERTY_HOOK_CALL 1 string("x") 1
0003 T1 = FETCH_DIM_R CV0($value) string("x")
0004 SEND_VAL T1 1
0005 DO_FCALL
0006 RETURN null

[iluuu1994]

Right. Parent hooks are not very optimized at this moment. I think they will be rare in practice. I'll have a look nonetheless. We might be switching to a different syntax though (parent::$prop and parent::$prop = $value), if possible.

[TRowbotham]

Potential scoping issue when hooks are defined in traits. Not sure if this is the expected behavior or not.

https://3v4l.org/8NILP/rfc#vrfc.property-hooks

[iluuu1994]

@TRowbotham Thanks for letting me know. I added this to my todo list.

[TRowbotham]

Here is another one: https://3v4l.org/7Al8b/rfc#vrfc.property-hooks

When hooks are defined in a class that implements the \Iterator interface the iterator returns the values of each property's get hook instead of the values that the iterator contains.

[iluuu1994]

@TRowbotham Good catch! I'll have a look at that one as well.

[TRowbotham]

Ran into a behavior difference between __get/__set and property hooks when a property hook's setter calls a function, which reads a property hook's value from a different scope. With __get/__set it works without error, however, the property hooks version reports a fatal error. I couldn't tell from the RFC if the difference in behavior was intentional.

(property hooks) https://3v4l.org/6W26c/rfc#vrfc.property-hooks
(__get/__set) https://3v4l.org/3pUtD/rfc#vrfc.property-hooks

[iluuu1994]

@TRowbotham This is intended behavior. Inside functions called from a hook, $this->prop refers to the backing field, as it would in the hook. However, the compiler cannot infer that that a backing field must be added, since it isn't used directly in the hook. As such, the property is marked as virtual. When doSomething is called, it tries to access this backing field that doesn't exist.

An additional difference is that, for the __get/__set case, each magic method is "guarded" on it's own. Guarding in this context means that inside the magic method, it won't be called again, but the underlying property will be accessed. If you assign a variable, invoking __set, __get can still be invoked inside __set. The same is not true for hooks, once you're in the hook $this->prop always refers to the backing value. This is intentional, because otherwise read/write assignments such as ++ within set might unexpectedly call your getter. Furthermore, it allows for more flexibility in the future, where self::$prop::get() may still be used as an explicit way to invoke the other hook.

I hope that clears it up.

[TRowbotham]

@iluuu1994 Thanks for the explanation. That makes it clear.

[rodrigoslayertech]

Congratulations!!! RFC passed! PHP 8.4 will be AWESOME!!!

[ramsey]

@rodrigoslayertech The voting is open until 29 April. Let's not count our chickens before they hatch. ;-)

[MarienMupenda]

Awsome, this sounds like the dynamic properties we have in laravel models, voted.

[claudepache]

Hi,

While playing with the feature on 3v4l.org, I found the following bug. Although I’ve not tested the PR in its current state, I suspect that the bug might be still here, because I didn’t find test against it.

Test case (https://3v4l.org/fuj6p/rfc#vrfcproperty-hooks):

abstract class A {
    abstract public $x { get; }
}

class C extends A {
    private $_x;
    public $x {
        get => $this->_x;
    }
}

// expected: bool(true); actual: bool(false)
var_dump((new ReflectionProperty(C::class, 'x'))->isVirtual());

// expected: Error: Property C::$x is read-only; actual: no error
$c = new C;
$c->x = 3;

The test case works as expected when I replace the abstract class with an interface (https://3v4l.org/ed1TG/rfc#vrfc.property-hooks).

(Background: I expect to use that pattern in order to approximate asymmetric visibility.)

[iluuu1994]

@claudepache The 3v4l build is indeed outdated. In any case, I'll make sure to have a look at your example. Thanks!

[iluuu1994]

@TRowbotham Thanks! This wasn't really specified in the RFC. I fixed the issue, but also raised it to Larry to discuss over whether we even want to allow yield. Edit: Talked it over with Larry and Bob. There's no good reason not to allow this, so we will.

[iluuu1994]

The implementation now solves all known issues, and CI (including nightly) passes. So I will be merging it in the coming days.

[nielsdos]

Is it the intention that ZEND_ACC_VIRTUAL implies the existence of hooks? If we allow to specify @virtual in the stub file in a follow-up to avoid allocating backing storage, then this check can potentially dereference a NULL pointer because prop->hooks isn't necessarily filled in.

[iluuu1994]

I don't know all the checks by heart but at least here, ZEND_ACC_VIRTUAL assumes hooks are present. I presume your intention is to use @virtual for existing virtual properties implemented using the existing object property handlers? Tweaking the logic where necessary to check for the existence of hooks would be ok with me, if these cannot immediately be migrated to hooks (in part because internal classes aren't supported yet). But I'd prefer doing that after this has merged.

[nielsdos]

I presume your intention is to use @virtual for existing virtual properties implemented using the existing object property handlers?

Indeed.

Tweaking the logic where necessary to check for the existence of hooks would be ok with me, if these cannot immediately be migrated to hooks (in part because internal classes aren't supported yet)

I don't know how internal hooks would look like so can't really comment on this (yet).

But I'd prefer doing that after this has merged.

Of course

[iluuu1994]

I don't know how internal hooks would look like so can't really comment on this (yet).

My intention is to allow marking the property with { get; set; } (or possibly just annotations until we have PHP-Parser support), which will look for and link the corresponding function into zend_property_info.hooks. So, hopefully it will not be too complicated.

[nielsdos]

Ah, those would be zend_function's right? That's a bit of a pitty because it would require completely reworking the virtual properties in DOM/XMLReader with the additional penalty of having to deal with a stackframe, unless they can be frameless.

[iluuu1994]

🤔 Good question. Avoiding the stack frame was not planned for now, but I'll see what's possible once I get to it.

[Wirone]

@iluuu1994 I know it's already merged, but I have question: wouldn't it be a good idea to introduce T_PROPERTY_HOOK_BRACE_OPEN and T_PROPERTY_HOOK_BRACE_CLOSE, so tools based on tokenizer could take direct advantage of it for their logic? Without this, PHP-CS-Fixer probably needs to introduce new custom token and transformer to make it easier for all the rules to hook into new syntax.

[iluuu1994]

@Wirone Unfortunately, we are lacking the context in the lexer ourselves to do that.

[Bilge]

Don't know if anyone cares, but this comment (/* {{{ */) seems to be mismatched (no closing braces).

[iluuu1994]

Thanks. I fixed it.