Fix: Provide `_is_utf8_charset()` in `compat.php` for early use. by dmsnell · Pull Request #7052 · WordPress/wordpress-develop

dmsnell · 2024-07-17T17:03:12Z

When is_utf8_charset() was introduced, the mb_strlen() and mb_substr() compat functions were modified to call it, but they are defined in compat.php before is_utf8_charset() is defined in functions.php.

Certain code calling these compat functions early in the boot process before functions.php is included and on hosts without the multi-byte extension would thus crash.

In this patch the is_utf8_charset() function is split into pure and stateful components. The pure version is recreated as _is_utf8_charset() and defined in compat.php while the existing function (which defaults to calling get_option( 'blog_charset' )) is left in place in functions.php. This ensures that code calling it will be able to call a form of the function even in early sequences.

Follow-up to [58169].

Props dmsnell, donncha, hellofromTonya, jeherve, slyall, spacedmonkey.
Fixes #61680.

When `is_utf8_charset()` was introduced, the `mb_strlen()` and `mb_substr()` compat functions were modified to call it, but they are defined in `compat.php` before `is_utf8_charset()` is defined in `functions.php`. Certain code calling these compat functions early in the boot process before `functions.php` is included and on hosts without the multi-byte extension would thus crash. In this patch the `is_utf8_charset()` function is split into pure and stateful components. The pure version is recreated as `_is_utf8_charset()` and defined in `compat.php` while the existing function (which defaults to calling `get_option( 'blog_charset' )`) is left in place in `functions.php`. This ensures that code calling it will be able to call a form of the function even in early sequences. Follow-up to [58169]. Props dmsnell, donncha, hellofromTonya, jeherve, slyall, spacedmonkey. Fixes #61680.

github-actions · 2024-07-17T17:03:38Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props dmsnell, jorbin, jonsurrell, joemcgill.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-07-17T17:16:03Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

sirreal

This change looks like it will fix the issue. I've been unsuccessful reproducing the original issue so far, so it's difficult for me to confirm the fix.

sirreal · 2024-07-17T17:31:34Z

 }

+/**
+ * Indicates if a given slug for a character set represents the UTF-8 text encoding.


Some other functions in this file mention that they're for internal use, like this:

Internal compat function to mimic mb_substr().

Should we mention that this is an internal function only intended to prevent load-order issues and that is_utf8_charset should be used in most cases?

I considered this, but also I don't see a reason why this shouldn't be made available 🤷‍♂️

is_utf8_charset is the API we expect folks to use, not this new function. To me, that's a good reason this should include @ignore and be noted as an internal compact function

I also think we need a note for why this is here since it's not a compat function like everything else

sirreal

I have tested this and done code review. I'm approving on those grounds.

I haven't been able to reproduce, so I haven't confirmed the fix.

joemcgill

This looks like it should fix the issue as reported in Automattic/jetpack#38358. However, a similar problem can happen any time a compat function in this file is called by code prior to when functions.php is loaded. For example, if _mb_substr() is called with $encoding set to null then a similar undefined function error would be triggered when get_option() is called, since the option.php file is loaded at the top of functions.php.

Maybe a better strategy for these polyfills is to always do a function_exists() check before calling any WordPress functions that might be loaded after these polyfills are available and providing a graceful fallback when the function is not yet loaded.

dmsnell · 2024-07-17T17:50:14Z

Thanks @joemcgill - I also noted the apparent call to get_option() but I didn't want to address it in this bug-fix. I could see value in calling function_exists() first, but also I don't see the need to perform this check at runtime when mostly they will always exist.

My initial reaction was that perhaps we could create a new CI task to check if anything in compat() is calling code that hasn't yet been defined. This, of course, would flag the mb_ functions 🤷‍♂️

In any case the situations where these things run is rare enough. I would imagine that we could get away with the runtime checks and have it not matter much. IMO, good work to discuss in an enhancement.

joemcgill · 2024-07-17T21:19:07Z

I also noted the apparent call to get_option() but I didn't want to address it in this bug-fix.

Totally agree that handling that separately makes sense.

I could see value in calling function_exists() first, but also I don't see the need to perform this check at runtime when mostly they will always exist.

This is the tricky part. While I appreciate not calling function_exists() when it's not needed in a majority of cases, it seems like the alternative is to either accept the potential fatals (expecting third party code that trigger these fatals to fix the issue) or do what you have done here, which is to create additional compat shims for those functions that splits out parts of the implementation here. For maintainability, I have some reservations to creating private stateless versions of any WP functions that are called from this file.

dmsnell · 2024-07-17T22:40:46Z

Thanks @joemcgill

For maintainability, I have some reservations to creating private stateless versions of any WP functions that are called from this file.

Would you care to share more about this? From my perspective, fixing this makes the code better by introducing this split, particularly since is_utf8_charset() is calling the more testable unit.

The only complication I see is that it's in compat.php, but then again, it's addressing some very low-level stuff required by other compatability code.

I considered duplicating the logic of this function here directly, but given that it was necessary in both of the _mb_substr() and _mb_strlen() functions, I thought it was more susceptible to problems than creating a new function would leave them.

Do you see this as a blocker to the patch as proposed?

Without trying to derail this conversation, I'm only more convinced each week that a good future direction for Core is UTF-8 everywhere, and to that end, some UTF-8 functionality is missing, and there have been many problems lately related to whether the multibyte extension is available.

I've been exploring a native UTF-8 handling system in #6883 and think it has merit. Similar to how the HTML API has replaced in user-space some PHP-provided functionality, incorporating our own UTF-8 encoding and decoding would let us finally get rid of a lot of needless and inconsistent checks and conversions.

I've been exploring some of this code inside Automattic too. When I feel it's ready for actual discussion I'll start posting on Make about it.

joemcgill · 2024-07-18T14:33:43Z

Would you care to share more about this?

Sure! Besides the use of an option value, is_utf8_charset() is a pretty simple function currently. Splitting the implementation into two different functions which exist in two separate files could be a bit confusing for future maintainers.

I don't see that maintainability concern as a blocker.

However, the other problem that I just noticed when looking at this again, is that now any time the _mb_substr() and _mb_strlen() polyfills are used, they will not be able to make use of the site's option setting, even though in a majority of cases, the need to avoid the get_option() call is unnecessary and unexpected. That seems like a bigger problem. That does seem like something we want to avoid.

aaronjorbin · 2024-07-18T14:35:28Z

I think there are two points (in addition to the UTF-8 everwhere one):

We need to figure out a way to prevent issues like this in the future.
The fatal at hand.

I think we should focus on 2 right now and create a separate core ticket and one where we can at a minimum include some documentation at the top of the file and ideally include some automated tests as well.

aaronjorbin · 2024-07-18T14:38:09Z

 }

+/**
+ * Indicates if a given slug for a character set represents the UTF-8 text encoding.


is_utf8_charset is the API we expect folks to use, not this new function. To me, that's a good reason this should include @ignore and be noted as an internal compact function

I also think we need a note for why this is here since it's not a compat function like everything else

aaronjorbin · 2024-07-18T14:38:59Z

@@ -7503,20 +7503,7 @@ function get_tag_regex( $tag ) {
 * @return bool Whether the slug represents the UTF-8 encoding.


This should include an @see _is_utf8_charset since this is now essentially a wrapper function

aaronjorbin · 2024-07-18T16:15:27Z

Follow up ticket: https://core.trac.wordpress.org/ticket/61694

dmsnell · 2024-07-18T18:34:01Z

Merged in [58764]
5a30482

WordPress deleted a comment from github-actions Bot Jul 17, 2024

sirreal reviewed Jul 17, 2024

View reviewed changes

sirreal approved these changes Jul 17, 2024

View reviewed changes

joemcgill reviewed Jul 17, 2024

View reviewed changes

aaronjorbin reviewed Jul 18, 2024

View reviewed changes

Update documenation to make clearer which function should be used where.

0de0a81

dmsnell closed this Jul 18, 2024

dmsnell deleted the fix/move-is-utf8-charset-earlier branch July 18, 2024 18:34

		@@ -7503,20 +7503,7 @@ function get_tag_regex( $tag ) {
		* @return bool Whether the slug represents the UTF-8 encoding.

Conversation

dmsnell commented Jul 17, 2024

Uh oh!

github-actions Bot commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 17, 2024

Test using WordPress Playground

Some things to be aware of

Uh oh!

sirreal left a comment

Choose a reason for hiding this comment

Uh oh!

sirreal Jul 17, 2024

Choose a reason for hiding this comment

Uh oh!

dmsnell Jul 17, 2024

Choose a reason for hiding this comment

Uh oh!

aaronjorbin Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

sirreal left a comment

Choose a reason for hiding this comment

Uh oh!

joemcgill left a comment

Choose a reason for hiding this comment

Uh oh!

dmsnell commented Jul 17, 2024

Uh oh!

joemcgill commented Jul 17, 2024

Uh oh!

dmsnell commented Jul 17, 2024

Uh oh!

joemcgill commented Jul 18, 2024

Uh oh!

aaronjorbin commented Jul 18, 2024

Uh oh!

aaronjorbin Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

aaronjorbin Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

aaronjorbin commented Jul 18, 2024

Uh oh!

dmsnell commented Jul 18, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions Bot commented Jul 17, 2024 •

edited

Loading