-
Notifications
You must be signed in to change notification settings - Fork 7.9k
zend_string: Support NUL bytes in ZSTR_*_LITERAL() and zend_string_*literal*()
#19582
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
5addc69 to
c76cf66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm around a -0.5 on this generally, and maybe -0.75 on this for PHP 8.5 - @edorian, what do you think?
UPGRADING.INTERNALS
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dropping of support for non-literal char* seems like a disadvantage - I'm not fully convinced of the benefits of this patch (were there any places that the macro was specifically avoided because of NUL bytes?) especially given where we are in the release cycle, where external extensions have likely already started updating for PHP 8.5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
were there any places that the macro was specifically avoided because of NUL bytes?
I did not check. Your comment on the other PR prompted me to look into the definition of the macro and then I noticed the issue.
The dropping of support for non-literal char* seems like a disadvantage
The "literal" in the name of the macro to me is an indication that it was not meant to support non-literal strings. Just like we have zend_string_starts_with_cstr() (char*) + length and zend_string_starts_with_literal().
But I'm seeing that zend_string_starts_with_literal() has the same issue as ZSTR_INIT_LITERAL(). They should all be adjusted at the same time then and then it also makes sense to me to move this to 8.6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah zend_string_equals_literal() is fine, zend_string_starts_with_literal() and zend_string_starts_with_literal_ci() are not.
c76cf66 to
d7bfdcc
Compare
ZSTR_INIT_LITERAL() (削除ここまで)ZSTR_*_LITERAL() and zend_string_*literal*() (追記ここまで)
As the person who introduced ZSTR_INIT_LITERAL(), restricting it to actual string literals makes sense to me. Whether this should be delayed to 8.6 is at the RMs discretion.
d7bfdcc to
3ba7252
Compare
Following #19565 (comment).