WP_Locale doesn't initialize property arrays before using them

[tyxla]

Currently, WP_Locale does not initialize its internal property arrays:

​https://github.com/wordpress/wordpress-develop/blob/trunk/src/wp-includes/class-wp-locale.php#L24

However, it starts using them immediately:

​https://github.com/wordpress/wordpress-develop/blob/trunk/src/wp-includes/class-wp-locale.php#L130

In some setups this has caused warnings from PHP 7.4 on:

Warning: array_values() expects parameter 1 to be array, null given in /wordpress/plugins/gutenberg/14.8.4/lib/compat/wordpress-6.1/date-settings.php on line 48

This is because from PHP 7.4 on, using null values as arrays will trigger a warning:

​https://www.php.net/manual/en/migration74.incompatible.php#migration74.incompatible.core.non-array-access

To fix this, the most straightforward way forward is to provide default values for all array properties in the WP_Locale class. That way they'll always be declared as arrays when we use them as such.

Currently, WP_Locale does not initialize its internal property arrays:

​https://github.com/wordpress/wordpress-develop/blob/trunk/src/wp-includes/class-wp-locale.php#L24

However, it starts using them immediately:

​https://github.com/wordpress/wordpress-develop/blob/trunk/src/wp-includes/class-wp-locale.php#L130

In some setups this has caused warnings from PHP 7.4 on:

Warning: array_values() expects parameter 1 to be array, null given in /wordpress/plugins/gutenberg/14.8.4/lib/compat/wordpress-6.1/date-settings.php on line 48

This is because from PHP 7.4 on, using null values as arrays will trigger a warning:

​https://www.php.net/manual/en/migration74.incompatible.php#migration74.incompatible.core.non-array-access

To fix this, the most straightforward way forward is to provide default values for all array properties in the WP_Locale class. That way they'll always be declared as arrays when we use them as such.

That's what this PR suggests.

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

[tyxla]

Here's a patch that adds default values to all array properties of WP_Locale:

​https://github.com/WordPress/wordpress-develop/pull/3824

[SergeyBiryukov]

Hi there, thanks for the ticket!

It looks like the PHP warning comes from ​these lines in Gutenberg:

$scripts->add_inline_script(
	'wp-date',
	sprintf(
		'wp.date.setSettings( %s );',
		wp_json_encode(
			array(
				'l10n'     => array(
					'locale'        => get_user_locale(),
					'months'        => array_values( $wp_locale->month ),
					'monthsShort'   => array_values( $wp_locale->month_abbrev ),
					'weekdays'      => array_values( $wp_locale->weekday ),
					'weekdaysShort' => array_values( $wp_locale->weekday_abbrev ),
					'meridiem'      => (object) $wp_locale->meridiem,
					...
				),
				...
			)
		)
	),
	'after'
);

However, I'm not quite sure exactly why, since these values should be initialized via WP_Locale::__construct()​ when the $wp_locale object is created in wp-settings.php​. Does the code in question run earlier than that?

So the warning appears to be only indicator at the moment that the code in question somehow runs when the $wp_locale object is not fully initialized and the locale data is not yet available.

• Would removing this warning make it harder to determine why the locale data is not loaded?
• Should the code in question perhaps run later, when the locale data is initialized?

[tyxla]

I believe you're asking great questions! Thanks for the swift reply!

Does the code in question run earlier than that?

Yes, it appears so! It's worth noting that WP_Locale can be used in other contexts also, like REST API requests or WP CLI, and there the load order can be different, especially when the setup is customized.

Would removing this warning make it harder to determine why the locale data is not loaded?

I personally don't think so. If the locale data doesn't load, chasing why that happens will be all the same with or without the warning, since its place of occurrence doesn't hint to a newly introduced error.

Should the code in question perhaps run later, when the locale data is initialized?

Likely. However, my $0.02 are that WP_Locale, as a module, should be abstracted in a way that allows it to work as a module, regardless of where it's dropped. The fact is that if we take WP_Locale as a single piece, and initialize it, it relies on external factors to set it up, which may not always be the case; thus the warnings that we see. An easy and painless way to ger around the warnings without affecting any other functionality would be to add the default values, as proposed in the related PR above.

[azaozz]

Replying to tyxla:

Yes, it appears so!

Don't think that's possible unless used incorrectly? :)

As @SergeyBiryukov mentioned WP_Locale is initialized (in wp-settings.php) relatively early. $scripts->add_inline_script() becomes available later, after the WP_Dependencies API is initialized and is expected to be used on or after the init action (or throws "Doing it wrong!").

Imho it seems that "something" is trying to load/initialize the WP_Dependencies API without loading WP. This is a big "Doing it wrong!" and seems unrelated to the code in Gutenberg.

Would removing this warning make it harder to determine why the locale data is not loaded?

Yes, seems it will "obfuscate" it.

On the other hand I'm +1 for the patch/PR as marking something as array in the docblock but then setting it to null is a pretty bad practice. Why use the wrong type? There should be another way to indicate improper usage.

WP_Locale, as a module, should be abstracted in a way that allows it to work as a module, regardless of where it's dropped.

Hmmm, but it is not a module? It depends on other parts of WP, is not designed to be self-contained, it is impossible to be used "stand alone", and there is no reason to use it as such.

[tyxla]

I can't disagree with all the great points brought here.

On the other hand I'm +1 for the patch/PR as marking something as array in the docblock but then setting it to null is a pretty bad practice. Why use the wrong type? There should be another way to indicate improper usage.

This summarizes what I would have replied to your comment. We can talk about load order, legacy usages, how the current architecture came to be, obfuscating any improper usage, and so on, but the issue that this PR addresses is in the very foundation and has nothing to do with all of that. It addresses a type issue, but also a clear violation of the PHP syntax, within the boundaries of the class. It also provides more safety and makes that class future-proof in case any future refactoring causes a difference in load or execution order.

Hmmm, but it is not a module? It depends on other parts of WP, is not designed to be self-contained, it is impossible to be used "stand alone", and there is no reason to use it as such.

I see, and I think that's one of the problems with the original WordPress architecture in general. Things often tend to be too tightly coupled with each other, and not always for a good reason. I see your point about why it's not supposed to be used standalone, but then why do we even have that functionality abstracted to its separate class? Just for keeping things tidy? I think that even makes things more confusing, as we fail to keep the classes self-sufficient and to follow the single-responsibility principle. I understand that there are historical reasons for how things are right now, and that's why this PR suggests minimal changes at no real cost.

I'm happy to let this go, but at this point, I am not convinced there are any real downsides to landing it.

[hellofromTonya]

Though instantiated in wp-settings.php, within the class object itself, WP_Locale properties are being set in the WP_Locale::init() method (called from the constructor) before first being initialized as an array data type. I agree with the approach in ​PR 3824 as it reuses Core code designs and clearly communicates design intent at the property definition itself.

I'd like to see a unit test added to the PR, which I will do today.

[hellofromTonya]

Follow-up to my previous comment. I spent some time today re-reading the discussion and investigating the problem.

tl;dr

• Initializing the properties at the definition point is a good design practice.

• But something else is going on at the point where Gutenberg's code throws the array_values() expects parameter 1 to be array, null given notice|warning.

• I was not able to reproduce the notice|warning.

More details

Thrown when attempting to use, not when setting.

Warning: array_values() expects parameter 1 to be array, null given

This notice|warning happens when using a non-array variable as an array. Notice, it says the property's data type was null, not array.

It does not throw the notice|warning when setting. Why? PHP will type juggle from null to array before setting the element (as it does in WP_Locale::init()). See it in action here ​https://3v4l.org/m1QUYX#v7.4.33 and ​https://3v4l.org/PtmGf#v7.4.33.

Something else is happening

When instantiating WP_Locale, WP_Locale::init() runs from the constructor. So invoking new WP_Locale automatically sets the properties to an array data type. For the notice|warning to happen:

• the properties would have been reset to null somewhere in the call stack
• Or a different object is in the global $wp_locale.

For the latter, maybe the object inherits from WP_Locale and overloads its constructor or init(), thus not presetting the properties.

If the $wp_locale global is an instance of WP_Locale and the properties were not reset, then the properties are array type and the warning|notice should not occur.

So something else is going on.

I'm curious of how to reproduce it for further investigation into the call stack.

Why is initializing the properties a good design practice?

I agree with @azaozz and @tyxla about pre-defining a default state of the WP_Locale properties. Why?

• The documentation and code are clearly expecting these properties to be an array data type.

• Setting each to a default array() state further helps to clearly communicate the code design.

• If another class inherits from WP_Locale but does not run init() from the constructor, then the properties would be null instead of array. Given that this is not a final class, it is possible that some real world sites and applications could be doing something custom. By initializing the properties as the patch does, any class extending from WP_Locale gains the benefits of the default value.

Hiding the problem

Would removing this warning make it harder to determine why the locale data is not loaded?

As @azaozz noted, it could obfuscate it. However, it is also possible that it resolves the issue if a custom implementation is being used OR the properties are being reset on purpose.

Next steps

• IMO the patch (once a few tweaks are done) should be committed (as noted above).
• A deeper investigative dive is needed into any impacted sites to better understand what's different on these sites.
  • Is the object in GLOBALS['wp_locale'] an instance of WP_Locale?
  • If yes, then search for where the properties are being reset.
  • If no, look at the object's class code to determine how it presets the properties.
• Step-by-step "how to reproduce instructions" are needed to help contributors investigate if this is a Core issue.

I've added tests including a custom implementation that inherits from WP_Locale but does not invoke init(). The tests:

• fail before fixes of initializing each property to an empty array.
• pass after the fixes.

[hellofromTonya]

The PR has been updated and is ready for commit. The commit is for design practice and code improvement, but may or may not resolve the root cause issue. So once committed, likely this issue needs to remain open.

Thanks a bunch, @hellofromtonya for commandeering it and adding tests.

I've addressed the recent feedback, please let me know if I can aid with anything else before we land it.

[hellofromTonya]

Preparing the commit.

[hellofromTonya]

In 55047:

I18N: Initialize WP_Locale array properties.

Initializing the WP_Locale array properties to an empty array at the class definition point. Why?

• Ensure the properties initialize to an array data type at instantiation (rather than null).

This initialization is needed to ensure the properties are not null if another class inherits from WP_Locale but does not run WP_Locale::init() from the constructor. In this case, the initialization prevents

Warning: array_values() expects parameter 1 to be array, null given

when Core uses any of the properties.

• Good design practice.

The code and documentation are clearly expecting these properties to be an array data type. Setting each to a default array() state further helps to clearly communicate the code design.

Follow-up to [37889], [36292], [31078], [3676], [6589].

Props tyxla, SergeyBiryukov, azaozz, hellofromTonya, mukesh27.
See #57427.

Committed via changeset https://core.trac.wordpress.org/changeset/55047. Thank you everyone for your contributions!

[hellofromTonya]

Initializing the WP_Locale properties was committed in [55047]. Thank you everyone for your contributions!

Leaving this ticket open for further investigation as noted above. Resetting keywords to the ticket's current state.

[hellofromTonya]

From the next steps:

A deeper investigative dive is needed into any impacted sites to better understand what's different on these sites.

• Is the object in GLOBALSwp_locale? an instance of WP_Locale?
• If yes, then search for where the properties are being reset.
• If no, look at the object's class code to determine how it presets the properties.

In some setups this has caused warnings from PHP 7.4 on:

I did a deeper dive and still haven't been successful at replicating the issue or identifying if there's a deeper issue within Core itself. More information is needed to further investigate what's different in "some setups" that are causing the warnings.

To confirm, @tyxla did [55047] solve the problem?

[tyxla]

Thanks for the ping, @hellofromTonya!

I can't tell for sure yet, since this change hasn't landed in production yet. We'll be able to tell once 6.2 is out and sites have updated to use it.

[hellofromTonya]

Replying to tyxla:

I can't tell for sure yet, since this change hasn't landed in production yet. We'll be able to tell once 6.2 is out and sites have updated to use it.

Okay thanks. Once the sites you're monitoring have updated to 6.2.0, @tyxla could you circle back here to share feedback on if the problem was resolved or not?

[tyxla]

Will do - thank you @hellofromTonya!

[hellofromTonya]

What is the current state of this ticket?

tl;dr

• [55047] was the original request.
• But something else is going on to cause the PHP 7.4 Warnings
• Unknown if [55047] resolves the reported issue on the affected "some setup" sites.
• Won't know until after the final 6.2.0 release and those sites upgrade to it.
• Deeper dive into the code and call stack analysis did not reveal clues of why or if this is a Core issue.

What is the next step?

• More information is needed to continue the investigation.
• Wait for feedback from @tyxla.

IMO this ticket is not yet resolved and won't be resolved in 6.2.0 cycle. It'll need to move to 6.2.1 (though that milestone is not yet available).

[hellofromTonya]

Hmm, thinking about how best to handle the milestone for this ticket. I'm moving it back to Awaiting Review. Why?

IMO this ticket is not yet resolved and won't be resolved in 6.2.0 cycle. It'll need to move to 6.2.1 (though that milestone is not yet available).

Once there's feedback, then it can be re-milestoned for further action.

[swissspidy]

Given the existing commit(s) in the 6.2 cycle and no further reports or anything, I'd suggest closing this with the correct milestone & opening new tickets for any new bugs being found.

[tyxla]

Oops, this one slipped through the cracks. I'm still seeing warnings and at this point, I'm convinced that something else is altering these default values, something unrelated to the core codebase.

I'll have to double-check what's going on our side.

In the meantime, I agree that it should be closed.