Conversation
`get_locale()`, `get_user_locale()` and `determine_locale()` are each documented to return a string, and each takes its value from a source that carries no type: the `WPLANG` option and site option, the `WPLANG` constant, the `$locale` and `$wp_local_package` globals, the `locale` user meta row, the `wp_lang` request parameter and cookie, and the `locale`, `option_WPLANG` and `determine_locale` filters. Only `empty()` and truthiness stood in the way, and a non-empty array passes both. A non-string locale reaches `WP_Textdomain_Registry::set()`, which uses it as an array key, so the first just-in-time translation for an unloaded text domain ends the request with "Cannot access offset of type array on array". On `wp-login.php` that needs no authentication: `sanitize_locale_name()` applies `preg_replace()`, which maps over an array subject and returns an array, so `?wp_lang[]=de_DE` carries one straight through. Add `is_string()` beside the non-empty checks that are already there, and fall back the way each function already falls back: to `en_US` for the site locale, to `get_locale()` for the user locale, and to the unfiltered value when a filter returns something else. Shape is left alone; only the type and emptiness are checked, so locales that work today keep working. Also ignore a non-string `locale` field in `wp_insert_user()`, the one core write path that stored it unchecked. That does not repair rows already in the database, which is why the read side is guarded too.
Test using WordPress PlaygroundThe 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
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
The single test named for `wp_insert_user()` only called `wp_update_user()`, so nothing exercised the field on user creation, which is the path the guard was added for. Split it in two: one test creates a user with a non-string `locale` field, the other updates one that already holds `de_DE`. The update case keeps its assertion that the row ends up empty, and its docblock now says so: a non-string does not leave the stored locale in place, it empties the row, which is the value the field takes when it is not passed at all.
The `@since` lines named only non-string values, but the guards also changed what an empty value does: on the `$locale` global short-circuit in `get_locale()`, and on the return of the `locale` and `determine_locale` filters, an empty value now falls back where it used to be returned. `wp_insert_user()` does not ignore a non-string `locale` field either. It stores an empty string, which for an update means the row is emptied rather than left alone. Also name the writers of the `$locale` global at the guard that reads it.
get_option() applies `option_{$option}` only when the option exists, and the
test database has no `WPLANG` row, so the filter never ran and the test passed
against unpatched source for all seven data sets.
Write the row before adding the filter. The test now fails on trunk for six of
the seven, the exception being the empty array, which `empty()` already caught.
Keep the one thing the reader cannot see from the code, where each value comes from, and cut the rest. Drop the `@since` lines. This fixes a fatal error, it does not change an API.
The getters now ignore non-string values, and other write paths such as meta_input and update_user_meta() store them unchanged. Validating one writer changes what wp_insert_user() stores for scalars without closing the gap.
Annotate the tests added for the locale getter type checks: void on the test methods and the raw option row helper, array on the data providers, and array shapes in place of array[] in their @return lines.
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Add @ticket 66106 to the 16 test methods this change adds. The data providers and the raw option row helper carry no ticket reference.
State what the tests cover in terms of types and the registry, without spelling out a request that ends in a fatal error on a released site.
…ring. determine_locale() passed request values to sanitize_locale_name() before its own type check. preg_replace() maps over an array, so the `sanitize_locale_name` filter received arrays in both arguments that it documents as strings.
One test per guarded source: the option, the `$locale` global, the `locale` filter, user meta, the `wp_lang` request parameter and the `determine_locale` filter, plus the translation call that threw the TypeError. Drop the data providers and the duplicate string-type assertion.
|
@josephscott This is ready for review if you'd like to take a look. |
| * | ||
| * @ticket 66106 | ||
| */ | ||
| public function test_sanitize_locale_name_returns_empty_string_for_a_non_string() { |
There was a problem hiding this comment.
Since the other test methods have void:
| public function test_sanitize_locale_name_returns_empty_string_for_a_non_string() { | |
| public function test_sanitize_locale_name_returns_empty_string_for_a_non_string(): void { |
| * | ||
| * @ticket 66106 | ||
| * | ||
| * @group ms-excluded |
There was a problem hiding this comment.
The methods could add global docs:
| * @group ms-excluded | |
| * @group ms-excluded | |
| * | |
| * @global wpdb $wpdb WordPress database abstraction object. | |
| * @global string $locale The current locale. |
| * @ticket 66106 | ||
| */ | ||
| public function test_sanitize_locale_name_returns_empty_string_for_a_non_string() { | ||
| $this->assertSame( '', sanitize_locale_name( array( 'de_DE' ) ) ); |
There was a problem hiding this comment.
| $this->assertSame( '', sanitize_locale_name( array( 'de_DE' ) ) ); | |
| $this->assertSame( '', sanitize_locale_name( array( 'de_DE' ) ) ); // @phpstan-ignore argument.type (Passing an array is intentional here.) |
| wp_cache_flush(); | ||
|
|
||
| $found = get_locale(); | ||
| $locale = $old_locale; |
There was a problem hiding this comment.
This should be done in tear_down. (And in the other test methods too.)
| $old_locale = $locale; | ||
| $locale = null; |
There was a problem hiding this comment.
This should be done in set_up. (And in the other test methods too.)
Prevent non-string locale values from causing a
TypeErrorduring just-in-time translation. Arrays can come from stored options, user meta, globals, filters or request parameters; the existing truthiness checks accept them.Add
is_string()checks toget_locale(),get_user_locale()anddetermine_locale().sanitize_locale_name()returns an empty string for a non-string, so thesanitize_locale_namefilter receives the documented type. Invalid values use the existing fallbacks:en_USfor the site locale,get_locale()for the user or request locale, and the unfiltered locale for invalid filter results. Empty globals and emptylocaleordetermine_localefilter results now use these fallbacks too. Locale string syntax is unchanged.Writers are unchanged:
wp_insert_user(),meta_inputand direct metadata writes can still store non-strings, so the getters validate on read.Adds regression tests for stored values, globals, filters and request parameters, including translation calls with array locales.
Trac ticket: https://core.trac.wordpress.org/ticket/66106
Use of AI Tools
AI assistance: Yes
Tools: Claude Code (Claude Opus 5, Claude Fable 5.1), Codex
Used for: Investigation, implementation, tests, review and description. Codex revised the description.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.