Zend: refactor zend_parse_arg_impl() to return zend_expected_type - #23052
Zend: refactor zend_parse_arg_impl() to return zend_expected_type#23052Girgias wants to merge 1 commit into
Conversation
feae6eb to
c471e54
Compare
c471e54 to
3ccd8f2
Compare
This effectively mimics part of what Fast ZPP does and allows us to re-use the fast ZPP error APIs
3ccd8f2 to
750ff30
Compare
| if (Z_TYPE_P(arg) == IS_STRING) { | ||
| zend_spprintf(error, 0, "must not contain any null bytes"); | ||
| return ""; | ||
| } else { | ||
| return check_null ? "?string" : "string"; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm not interested in a "review" from Claude.
There was a problem hiding this comment.
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.
This will throw a different message now, correct? We don't seem to have a test for this. With |
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.