Zend: refactor zend_parse_arg_impl() to return zend_expected_type

[Girgias]

This effectively mimics part of what Fast ZPP does and allows us to re-use the fast ZPP error APIs

Further refactorings: #23104 and #23105.

[kamil-tekiela]

Do I understand correct that this was just so that the user is informed about NUL bytes in their path string? Because the NUL bytes are already handled in zend_str_has_nul_byte but the error message would be generic, right? Now, you handle it with zend_wrong_parameter_type_error.

[Girgias]

Yes. This is the purpose of this ZPP specifier, and has always been this way.

You get the same error message either way, fast ZPP only uses zend_wrong_parameter_type_error to generate error messages.

[kamil-tekiela]

Ok, but one thing I don't understand. Previously if (Z_TYPE_P(arg) == IS_STRING) { was after ZVAL_DEREF(arg) but now in the zend_wrong_parameter_type_error, you have this check but I cannot find any ZVAL_DEREF(arg) up the call stack. Am I missing something?

Do we have a unit test for something like this:

$s = "abc\0def";
$ref = &$s;
fopen($ref, 'r'); 

EDIT: So I asked Claude to explain this to me and it told me that it's not possible for zend_parse_arg/zend_parse_arg_impl to receive a reference, so there is no need for dereference checks. I can't say that I understand why it's in one place and not in the other, but at least it doesn't cause a regression.

[Girgias]

I'm not interested in a "review" from Claude.

[kamil-tekiela]

Just to clarify, to avoid any misunderstanding, the review is mine. I only used Claude to help me understand the code. I am genuinely asking why we have ZVAL_DEREF(arg) but not in the other place.

[arnaud-lb]

diff --git a/ext/zend_test/test.c b/ext/zend_test/test.c
index a880c09fc1f..fe670890f7d 100644
--- a/ext/zend_test/test.c
+++ b/ext/zend_test/test.c
@@ -1969,3 +1969,14 @@ static PHP_FUNCTION(zend_test_gh19792)
        zend_error(E_WARNING, "a warning");
        zend_throw_error(NULL, "an exception");
 }
+
+static PHP_FUNCTION(zend_test_recv_path_by_ref_old_zpp)
+{
+       zend_string *path;
+
+       if (zend_parse_parameters(ZEND_NUM_ARGS(), "p", &path) == FAILURE) {
+               RETURN_THROWS();
+       }
+
+       RETURN_STR(path);
+}
diff --git a/ext/zend_test/test.stub.php b/ext/zend_test/test.stub.php
index 7c96ea176a8..6c8ffd4ca08 100644
--- a/ext/zend_test/test.stub.php
+++ b/ext/zend_test/test.stub.php
@@ -392,6 +392,8 @@ function zend_test_uri_parser(string $uri, string $parser): array { }
 
     /** @compile-time-eval */
     function zend_test_gh19792(): void {}
+
+    function zend_test_recv_path_by_ref_old_zpp(string &$path): void {}
 }
 
 namespace ZendTestNS {

$ php -r '$a = "foo\0bar"; zend_test_recv_path_by_ref_old_zpp($a);'

Fatal error: Uncaught TypeError: zend_test_recv_path_by_ref_old_zpp(): Argument #1 ($path) must be of type string, string given in Command line code:1

[Girgias]

Do we even allow this sort of usage from ZPP? Every single by-ref param I've ever seen uses the Z_PARAM_ZVAL/z specifiers as I don't know how you'd assign a new value to the reference?

[arnaud-lb]

Good point indeed!

[Girgias]

I guess what we probably should do is add an assertion that the zval is not IS_REF (nor IS_INDIRECT) except for the 'z' specifier.

[arnaud-lb]

Yes this would make sense

[kamil-tekiela]

<?php
declare(strict_types=1);
get_class_vars(123);

This will throw a different message now, correct? We don't seem to have a test for this. With declare(strict_types=1)

[kamil-tekiela]

<?php
declare(strict_types=1);
get_class_vars(123);

This will throw a different message now, correct? We don't seem to have a test for this. With declare(strict_types=1)

Regarding this, my question is why there are no changes in the tests that reflect the different behaviour I observed with that code sample.
Error before:

Argument #1 ($class) must be a valid class name, 123 given

Error after:

Argument #1 ($class) must be of type string, int given 

[arnaud-lb]

Nit / not for this PR: Both zend_parse_arg_func() and zend_is_callable_at_frame() zero *error. I'm wondering if we could move this responsibility to the caller. In the case of zend_parse_arg_impl(), *error is always NULL before calling zend_parse_arg_func() and zend_is_callable_at_frame().

[arnaud-lb]

Nit / not for this PR: We should unify ownership of error and maybe make it a zend_string*.

[Girgias]

Agreed, this was really confusing to me. But error is currently how the "advanced callable errors" are done, so I'll need to think how to change this to a zend_string.