PHP 8.1.: the default value of the flags parameter for htmlentities() et all needs to be explicitly set

[jrf]

From the PHP 8.1 changelog:

htmlspecialchars(), htmlentities(), htmlspecialchars_decode(),
html_entity_decode() and get_html_translation_table() now use
ENT_QUOTES | ENT_SUBSTITUTE rather than ENT_COMPAT by default. This means
that ' is escaped to ' while previously it was left alone.
Additionally, malformed UTF-8 will be replaced by a Unicode substitution
character, instead of resulting in an empty string.

Ref: ​https://github.com/php/php-src/blob/28a1a6be0873a109cb02ba32784bf046b87a02e4/UPGRADING#L149-L154

If effect this means that the output of the above mentioned functions may be different depending on the PHP version and the passed text string, unless the $flags parameter is explicitly passed.

I've run an initial scan over WordPress core with a new (not yet published) sniff for PHPCompatibility and this flags 33 issues.

• 1 issue in GetID3 which should be fixed upstream and the copy of GetID3 used in WP should be updated once the issue is fixed.
• 1 issue in PHPMailer which should be fixed upstream and the copy of PHPMailer used in WP should be updated once the issue is fixed.
• 1 issue in SimplePie which should be fixed upstream and the copy of SimplePie used in WP should be updated once the issue is fixed.
• And 30 issues in WP Core native code or code from external dependencies which are no longer maintained externally.

Detailed issue list: ​https://gist.github.com/jrfnl/9d56b4053faa62a0fe91dea1b14839bf

To fix this issue, the $flags parameter should be explicitly passed in each of these function calls.

Some investigation will be needed for each of these instances to determine what will be the optimal value for $flags.

Take note that the "old" parameter default in the function signature is documented as ENT_COMPAT, while in the parameter detail documentation, it states that the default, in actual fact, is ENT_COMPAT | ENT_HTML401.

However, by the looks of it, the full range of flag constants is available to us, which is at least one less problem.
There is no mention of any of the flags being added since PHP 5.6.
Ref: ​https://php-legacy-docs.zend.com/manual/php5/en/string.constants

It is strongly recommended to make sure that for each of these at least one unit test exists which exposes the difference in output between PHP < 8.1 and PHP 8.1 to safeguard the fixes which will be added for the future.

Also see:

• ​https://www.php.net/manual/en/function.htmlentities.php
• ​https://www.php.net/manual/en/function.html-entity-decode.php
• ​https://www.php.net/manual/en/function.htmlspecialchars.php
• ​https://www.php.net/manual/en/function.htmlspecialchars-decode.php
• ​https://www.php.net/manual/en/function.get-html-translation-table.php

[jrf]

PR for PHPMailer: ​https://github.com/PHPMailer/PHPMailer/pull/2365

[hellofromTonya]

With 4 days left until 5.9 Beta 1, moving this to 6.0.

[GaryJ]

According to ​https://www.php.net/manual/en/function.htmlentities.php, the default for flags for PHP 8.1 was "changed from ENT_COMPAT to ENT_QUOTES | ENT_SUBSTITUTE | ENT_HTML401".

I don't know what difference that ENT_HTML401 might make (there's also a ENT_HTML5 flag available as well), but might be something else to consider.

[jrf]

Replying to GaryJ:

According to ​https://www.php.net/manual/en/function.htmlentities.php, the default for flags for PHP 8.1 was "changed from ENT_COMPAT to ENT_QUOTES | ENT_SUBSTITUTE | ENT_HTML401".

I don't know what difference that ENT_HTML401 might make (there's also a ENT_HTML5 flag available as well), but might be something else to consider.

The ENT_HTML401 makes no difference as the value of that constant is 0, so the "bit" value remains the same with or without.
The ENT_HTML401 part is not an actual change, just better documentation.

See:

• ​https://github.com/php/doc-en/pull/361#issuecomment-925931381
• ​https://github.com/php/php-src/pull/7514

[costdev]

With 6.0 RC1 tomorrow, I'm moving this to the 6.1 milestone.

[rafiahmedd]

I think we can discuss this ticket on the next dev chat on slack. And it seems to be an important one.

[jrf]

FYI: If I remember correctly, @SergeyBiryukov is working on patches for this ticket together with other people from the Yoast WP Core team.

[SergeyBiryukov]

Yes, @aristath, @justinahinon, @poena, @afercia, and I worked on this recently in group sessions and should have a PR ready for review soon :)

[audrasjb]

With WP 6.1 RC 1 scheduled tomorrow (Oct 10, 2022), there is not much time left to address this ticket. Given it still needs a patch, let's move this ticket to 6.2.

Ps: if you were about to send a patch and if you feel it is realistic to commit it in the next few hours, please feel free to move this ticket back to milestone 6.1.

Trac ticket: https://core.trac.wordpress.org/ticket/53465

[costdev]

This ticket was discussed during the recent bug scrub.

@SergeyBiryukov Do you have time to give the PR a quick bump to see results on GitHub Actions?

[hellofromTonya]

The last scheduled 6.2 beta happened today. RC1 is next week. Moving this ticket to 6.3 and self-assigning ownership to help shepherd its completion.

[chaion07]

Thanks @jrf for continuing to work on this. We reviewed this ticket during a recent bug-scrub session and based on the feedback we are updating the milestone to 6.4. Cheers!

Props to @costdev & @mukesh27

[hellofromTonya]

Marking this work as a PHP 8.1 incompatibility. This is done for tracking purposes and for identifying documented exceptions to Core being compatible with PHP 8.1.

These functions will be included in the list of PHP 8.1 "compatible with documented exceptions" for WP 6.3.0 and beyond until resolved. See ​https://make.wordpress.org/core/2023/06/20/proposal-criteria-for-removing-beta-support-from-each-php-8-version/.

Besides the merge conflicts, is this patch ready for review @SergeyBiryukov?

[hellofromTonya]

Hey @SergeyBiryukov and @jrf, following up on the patch for this ticket as 6.4 RC1 is next week.

Is ​https://github.com/WordPress/wordpress-develop/pull/3429 ready for review and commit consideration? It's still marked as WIP and there are merge conflicts. I haven't yet gone through the code. But am I also curious if each instance has a test associated with it:

It is strongly recommended to make sure that for each of these at least one unit test exists which exposes the difference in output between PHP < 8.1 and PHP 8.1 to safeguard the fixes which will be added for the future.

@hellofromtonya If you don't mind, I'll review when the PR gets marked _ready for review_.

[hellofromTonya]

6.4 RC1 is in a few hours. The patch is not quite ready yet. Moving to 6.5.

[swissspidy]

@hellofromTonya Is this on your radar still for 6.5? Looks like a punt candidate given the lack of activity.

[SergeyBiryukov]

Replying to jrf:

From the PHP 8.1 changelog:

htmlspecialchars(), htmlentities(), htmlspecialchars_decode(),
html_entity_decode() and get_html_translation_table() now use
ENT_QUOTES | ENT_SUBSTITUTE rather than ENT_COMPAT by default. This means
that ' is escaped to ' while previously it was left alone.
Additionally, malformed UTF-8 will be replaced by a Unicode substitution
character, instead of resulting in an empty string.

...
To fix this issue, the $flags parameter should be explicitly passed in each of these function calls.

I had a question while revisiting this ticket and PR: What about the instances where this change is not relevant, e.g.:

• ' or malformed UTF-8 would never occur under normal circumstances.
• They might occur, but the escaping would not make any difference.

For example, instances like htmlentities( __( 'Unknown Feed' ) ). Do we still need to add the $flags parameter there?

Some investigation will be needed for each of these instances to determine what will be the optimal value for $flags.

Take note that the "old" parameter default in the function signature is documented as ENT_COMPAT, while in the parameter detail documentation, it states that the default, in actual fact, is ENT_COMPAT | ENT_HTML401.

The current draft PR adds ENT_COMPAT | ENT_HTML401 to most of the instances to keep the current behavior, whether or not the change is relevant to that particular instance. In some cases, it replaces the function call altogether with a more appropriate function.

Should there be any additional considerations when determining the optimal value for $flags?

[hellofromTonya]

With the last scheduled beta for 6.6 happening in ~2 hours, seems this ticket needs a wee bit more time to land. Moving to 6.7.

How about using a constant? Something like WP_LEGACY_ENTITY_FLAGS.

• This could help for future changes in the codebase, where the uses of that constant would be progressively replaced by something else. This way, the removals of the legacy flags could be more easily tracked.
• And of course, searching "WP_LEGACY_ENTITY_FLAGS" in the codebase would be easier then searching the flags.

As a subsequent step, I'm thinking about adding another constant, WP_ENTITY_FLAGS, that would consist of more adequate flags:

• Replace ENT_COMPAT with ENT_QUOTES: Way more secure.
• Add ENT_SUBSTITUTE: Better show the substitution problems.
• Stay on ENT_HTML401: ENT_HTML5 seems to be tricky, and ENT_HTML401 is the most compatible.

Note it would match the newer PHP default flags. But by specifying our flags, it would prevent breakage if the PHP default flags were to change again (just like it happened here).

Then, you guessed it: WP_LEGACY_ENTITY_FLAGS would be progressively replaced with WP_ENTITY_FLAGS in the codebase. Then, WP_LEGACY_ENTITY_FLAGS could be removed.

Additionally, you may be interested by this ​very exhaustive answer on Stack Overflow.

However, this PR has not been merged yet, and probably many people are now running on at least PHP 8.1.

This means they are already using the newer flags, apparently without disruption.

If merging this PR or adding a WP_LEGACY_ENTITY_FLAGS constant as I suggested above, people would be switched back to the old flags.

Therefore, it may be better to skip the "legacy flags" step, and directly make the change to the newer flags, using the WP_ENTITY_FLAGS constant I suggested above.

[stoyangeorgiev]

This one was discussed at a bug scrub. With RC1 right around the corner, will move this one to 6.8. The PR may need some refreshing. Also the discussion for this one seems open, so maybe more time is needed for this one.

[wesrapyd]

Did this make 6.8

[joemcgill]

@SergeyBiryukov curious if you still had interest in pushing this forward for 6.8. I also am interested in your thoughts on the suggestion in the PR review that we should actually stick with the current behavior, given that as of today >40% of sites are already running on PHP 8.1+ based on ​current stats.

[johnbillion]

To reiterate the point that Joe is making, PHP 8.1 was released over three years ago and just shy of 50% of sites are now running PHP 8.1+. If the proposed change was to be made now, it would actually revert nearly 50% of sites to the previous behaviour which, for the most part, means single quotes that are encoded would no longer be encoded. I think this risks a greater negative effect than keeping the default behaviour.

I think one of the following approaches should be taken:

1. Leave everything as-is, unless specific cases are known where an encoded single quote is undesirable. This means there will remain a difference in behaviour between sites on PHP 8.1+ and <8.1, which is the situation that we have lived with for the last three years.
2. Do the opposite of the proposed change, which is to move to at least ENT_QUOTES so single quotes are encoded and decoded, which is the default behaviour in PHP 8.1+ and arguably safer as it protects against breaking out of single quoted attribute values.

I am tempted to suggest closing this as wontfix, but I appreciate that being more explicit might be preferable.

[SergeyBiryukov]

Replying to johnbillion:

To reiterate the point that Joe is making, PHP 8.1 was released over three years ago and just shy of 50% of sites are now running PHP 8.1+. If the proposed change was to be made now, it would actually revert nearly 50% of sites to the previous behaviour which, for the most part, means single quotes that are encoded would no longer be encoded. I think this risks a greater negative effect than keeping the default behaviour.

Thanks, similar thoughts here. Leaning towards the first approach, but curious to see what others think. Keeping the ticket open for now.

(My apologies for commenting on the GitHub ticket. I'm not planning to create an account on the Trac tracker…)

Apparently, johnbillion, SergeyBiryukov and myself agree: we should not go back to the pre‑PHP 8.1 flags.

Thus, as I already suggested earlier, I would suggest introducing a constant:

if ( ! defined( 'WP_ENTITY_FLAGS' ) ) {
    define( 'WP_ENTITY_FLAGS', ENT_QUOTES | ENT_SUBSTITUTE | ENT_HTML401 );
}

(I'm using PHP's current default flags, which, as I elaborated earlier, are well-chosen. Also, these flags are presently the de facto standard in use for WordPress.)

Then of course use this new constant across the codebase.

Benefits:

• By "freezing" the flags, we would prevent future issues like the one at hand, if ever the default flags were to be changed again in PHP. Of course, this wouldn't prevent from updating the flags in the constant, in some far future.
• If _really_ an user needs to get the pre‑PHP 8.1 flags (because he still hasn't updated to at least PHP 8.1…), he could define the constant himself, as a workaround.