User roles property should always be an array, but they sometimes become an object in localized data#8790
User roles property should always be an array, but they sometimes become an object in localized data#8790haruncpi wants to merge 17 commits intoWordPress:trunkfrom
Conversation
|
Hi @haruncpi! 👋 Thank you for your contribution to WordPress! 💖 It looks like this is your first pull request to No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description. Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making. More information about how GitHub pull requests can be used to contribute to WordPress can be found in the Core Handbook. Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook. If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook. The Developer Hub also documents the various coding standards that are followed:
Thank you, |
|
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. |
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. |
Test ReportI’ve tested this PR and can confirm that it works as expected. After applying the changes, the $current_user->roles array is properly reindexed using array_values(), ensuring sequential keys. The localized JSON output now correctly produces an array instead of a JavaScript object, which resolves the client-side issue. Tested with multiple role combinations, including cases where some roles were removed — everything behaves correctly. ✅ Testing environment:WordPress Version: 6.8.1 Thanks for the Fix |
| * @return void | ||
| */ | ||
| public function test_user_roles_property_should_be_reindexed() { | ||
| public function test_user_roles_property_is_sequential_array() { |
There was a problem hiding this comment.
This test case method is not from earlier code—it was specifically written for this PR and is directly related to ticket #63427. It’s safe and will not cause any breakage.
| $this->roles = array(); | ||
| foreach ( $this->caps as $key => $value ) { | ||
| if ( $wp_roles->is_role( $key ) ) { | ||
| $this->roles[] = $key; | ||
| } | ||
| } |
There was a problem hiding this comment.
This could be simplified to use array_values() instead of a loop.
There was a problem hiding this comment.
At the very first commit of this PR, I used an array_values() check this commit: 4e32ecf
However, that approach had a downside — chaining multiple array functions such as array_keys(), array_filter(), and array_values() increases both time complexity and memory usage. This can lead to unnecessary overhead, as each function call duplicates or re-indexes data.
Following feedback from @SirLouen , I replaced the chained array operations with a simple foreach loop. This improves code performance, reduces time complexity, and makes the logic easier to read and maintain.
You can see the related discussion in the commit linked above.
There was a problem hiding this comment.
Are there numbers on the performance effect?
There was a problem hiding this comment.
If a simpler approach can make the code a bit faster and cleaner, why not use it? Even small improvements like this contribute to making WordPress overall a little more performant.
In the previous version, with the addition of array_values(), each of the array functions (array_keys(), array_filter(), array_values()) creates a new array in memory. When these functions are chained, it results in:
- Multiple iterations — each function loops through the array once.
- Temporary arrays — each function returns a new array copy, increasing memory usage.
- Re-indexing overhead — array_values() in particular reassigns numeric indexes, adding another full pass.
A simple foreach loop, by contrast:
- Iterates only once.
- Doesn’t duplicate or re-index data.
- Keeps the logic in a single, predictable control flow.
|
@haruncpi It appears the unit tests are failing. Could you investigate whether the problem lies in the logic itself or if the tests need to be updated? |
|
@t-hamano unit test case updated, now it's working fine. |
When filtering user roles using
array_filter()byget_role_capsmethod inwp-includes/class-wp-user.php, the resulting$this->rolesarray can end up with non-sequential numeric keys if some roles are removed. This causesWP_User->rolesto be treated as a JavaScript object instead of an array when passed throughwp_localize_script()— breaking client-side expectations.For example:
If roles contains keys like
[0 => 'editor', 2 => 'custom'], the resulting JSON becomes:Instead of
This patch fixes the issue by reindexing the array with
array_values()to ensure the result is always a proper, sequential array.Ticket’s URL
https://core.trac.wordpress.org/ticket/63427