Numeric parent hook call fails with assertion

[yype]

Description

Hello there.

The following code:

<?php new class { function a () { PARENT :: $ { 0} :: get () ; } };

Resulted in this output:

php: Zend/zend_ast.h:347: zend_string *zend_ast_get_str(zend_ast *): Assertion `zval_get_type(&(*(zv))) == 6' failed.

To reproduce:

> php-src/sapi/cli/php <path_to_code>
php: Zend/zend_ast.h:347: zend_string *zend_ast_get_str(zend_ast *): Assertion `zval_get_type(&(*(zv))) == 6' failed.

Additional reference: https://3v4l.org/ZPSua/rfc#vgit.master

PHP Version

Nightly and 8.4.2

Operating System

Ubuntu 22.04

[ndossche]

Either we disallow this by checking the type, or we allow this by doing the following:

diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
index 456c0b8f410..70db11c4cc2 100644
--- a/Zend/zend_compile.c
+++ b/Zend/zend_compile.c
@@ -5098,7 +5098,8 @@ static bool zend_compile_parent_property_hook_call(znode *result, zend_ast *ast,
 		zend_error_noreturn(E_COMPILE_ERROR, "Cannot create Closure for parent property hook call");
 	}
 
-	zend_string *property_name = zend_ast_get_str(class_ast->child[1]);
+	zval *property_hook_name_zv = zend_ast_get_zval(class_ast->child[1]);
+	zend_string *property_name = zval_get_string(property_hook_name_zv);
 	zend_string *hook_name = zend_ast_get_str(method_ast);
 	zend_property_hook_kind hook_kind = zend_get_property_hook_kind_from_name(hook_name);
 	ZEND_ASSERT(hook_kind != (uint32_t)-1);
@@ -5122,7 +5123,6 @@ static bool zend_compile_parent_property_hook_call(znode *result, zend_ast *ast,
 	zend_op *opline = get_next_op();
 	opline->opcode = ZEND_INIT_PARENT_PROPERTY_HOOK_CALL;
 	opline->op1_type = IS_CONST;
-	zend_string_copy(property_name);
 	opline->op1.constant = zend_add_literal_string(&property_name);
 	opline->op2.num = hook_kind;
 

However, this isn't really super useful as you can't define property hooks where the property name is numeric.
The only point for this patch would be consistency then with allowing numeric property names in this expression, albeit useless
WDYT @iluuu1994 ?

[iluuu1994]

I think it makes sense to fail at compile time when the property name is not a string.

[ndossche]

Okay, I'll get to that, thanks for the input

[iluuu1994]

@nielsdos Or probably just fall back to the normal behavior in the big ugly if above this logic.

[ndossche]

@nielsdos Or probably just fall back to the normal behavior in the big ugly if above this logic.

This is simple to do, but if you do this, then it becomes kind of inconsistent in meaning.
I.e. the following would compile normally but won't be an actual property hook parent call

<?php
class ParentC {}
class Child extends ParentC {
    function a () { PARENT :: $ { 0} :: get () ; }
};

[iluuu1994]

@nielsdos Right. We're sharing this syntax, and decided (for now) to only interpret the purest "form" as a parent property hook call. I.e. the class name needs to be parent, the method name must be get and set, the property name must be a ZEND_AST_ZVAL, i.e. known at compile time. Restricting this further would be ok imo, but it's equally fine to interpret the property as 0. This will simply fail a few lines below I believe (Must not use parent::$%s::%s() in a different property ($%s)).

[ndossche]

I'm going for my original patch idea for consistency then.

This will simply fail a few lines below I believe (Must not use parent::$%s::%s() in a different property ($%s)).

In OP's code almost right: we're not in a hook context but indeed if I use a hook context it fails here. Which is nice and consistent imo.