Skip to content

ext/standard: handle html entities empty string before processing #19220

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jul 28, 2025
Merged

ext/standard: handle html entities empty string before processing #19220

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e210de8
handle html entities empty string
xepozz Jul 23, 2025
a784b17
apply review suggestions
xepozz Jul 23, 2025
987f327
use empty string pointer
xepozz Jul 23, 2025
8cca28d
remove expectations
xepozz Jul 25, 2025
1615039
remove unnecessary return stmt
xepozz Jul 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ext/standard/html.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1320,6 +1320,10 @@ static void php_html_entities(INTERNAL_FUNCTION_PARAMETERS, int all)
Z_PARAM_BOOL(double_encode);
ZEND_PARSE_PARAMETERS_END();

if (EXPECTED(ZSTR_LEN(str) == 0)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This most certainly is not expected. You should not just benchmark the case of passing an empty string, but also non-empty strings of varying lengths.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean? as you can see the new benchmark is hitting non empty string as well
https://gist.github.com/xepozz/8a7cbb52dd2087634d2ddaa90a21acbe#file-htmlspecialchars-php-L25

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should I replace it with UNEXPECTED(len > 0)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, instead, this should not have a branch hint at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, is there a guide how it works?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No guide. But UNEXPECTED should only be used for situations that are actually unexpected: it can move the code to the cold text section and pessimize optimizations. I only use it for error conditions. EXPECTED is less aggressive, but still inappropriate here as calling it with length 0 is an edge case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So,
UNEXPECTED is preferable for error (exceptions?) branch: call strlen() with null/int/resource
EXPECTED is preferable for most popular branch: call strlen() with non empty string
for the rest just don't write any hints?

RETURN_EMPTY_STRING();
return;
}
replaced = php_escape_html_entities_ex(
(unsigned char*)ZSTR_VAL(str), ZSTR_LEN(str), all, (int) flags,
hint_charset ? ZSTR_VAL(hint_charset) : NULL, double_encode, /* quiet */ 0);
Expand Down
Loading