Skip to content

User roles property should always be an array, but they sometimes become an object in localized data #8790

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
haruncpi wants to merge 17 commits into from
Closed

User roles property should always be an array, but they sometimes become an object in localized data #8790

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
4e32ecf
Adds array reindexing to ensure roles remains a proper indexed array
haruncpi May 10, 2025
6e53e6d
unit test added
haruncpi May 12, 2025
3958cca
wpcs issue fix
haruncpi May 12, 2025
db42295
wpcs issue fix
haruncpi May 12, 2025
70a0ad2
wpcs reserved keyword array param changed to arr
haruncpi May 12, 2025
d2cb3e0
Refactor: Improve performance of role filtering logic
haruncpi Jun 15, 2025
2fe2fe8
Update: test method name updated and removed unnecessary comment.
haruncpi Jun 15, 2025
7fcc427
Update: foreach loop with key-value pairs in caps array
haruncpi Jun 16, 2025
a0753b1
Merge branch 'WordPress:trunk' into trunk
haruncpi Nov 1, 2025
7c6213f
Merge branch 'trunk' into haruncpi/trunk
peterwilsoncc Nov 10, 2025
01fba79
removed @return void
haruncpi Nov 10, 2025
9323c3e
Merge branch 'trunk' of https://github.com/haruncpi/wordpress-develop…
haruncpi Nov 10, 2025
39fb17b
ticket number removed from helper method
haruncpi Nov 10, 2025
3c3f459
explained each assertion
haruncpi Nov 10, 2025
be680ac
improved doc comment of test helper
haruncpi Nov 10, 2025
8e155fd
unit test case updated
haruncpi Nov 10, 2025
cc0c584
converted to message param
haruncpi Nov 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/wp-includes/class-wp-user.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,12 @@ public function get_role_caps() {

// Filter out caps that are not role names and assign to $this->roles.
if ( is_array( $this->caps ) ) {
$this->roles = array_filter( array_keys( $this->caps ), array( $wp_roles, 'is_role' ) );
$this->roles = array();
foreach ( $this->caps as $cap => $granted ) {
if ( $wp_roles->is_role( $cap ) ) {
$this->roles[] = $cap;
}
}
Comment on lines +520 to +525
Copy link
Contributor

@peterwilsoncc peterwilsoncc Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be simplified to use array_values() instead of a loop.

Copy link
Author

@haruncpi haruncpi Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@peterwilsoncc peterwilsoncc Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there numbers on the performance effect?

Copy link
Author

@haruncpi haruncpi Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// Build $allcaps from role caps, overlay user's $caps.
Expand Down
38 changes: 38 additions & 0 deletions tests/phpunit/tests/user.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1931,6 +1931,44 @@ public function test_send_confirmation_on_profile_email_html_entities_decoded()
$this->assertStringNotContainsString( ''Test' blog's "name" has <html entities> &', $email->subject, 'Email subject does contains HTML entities' );
}

/**
* Check array is a sequential.
*
* @param array $arr array.
*
* @ticket 63427
*
* @return bool
*/
private function is_sequential( array $arr ) {
return array_keys( $arr ) === range( 0, count( $arr ) - 1 );
}

/**
* Tests that the `roles` property is an sequential array.
*
* @ticket 63427
*
* @return void
*/
public function test_user_roles_property_is_sequential_array() {
Copy link
Author

@haruncpi haruncpi Jun 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@SirLouen SirLouen Jun 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh my bad, they are actually unit tests 🤣 I was reading this part this morning on my phone and I completely mixed up

$user = new WP_User( self::$author_id );
$this->assertTrue( $this->is_sequential( $user->roles ) );

$user->remove_role( 'author' );
$this->assertIsArray( $user->roles );
$this->assertSame( array(), $user->roles );

$user->add_role( 'author' );
$this->assertSame( array( 'author' ), $user->roles );
$this->assertTrue( $this->is_sequential( $user->roles ) );

$user->add_role( 'custom_role' );
$user->add_role( 'subscriber' );
$this->assertSame( array( 'author', 'subscriber' ), $user->roles );
$this->assertTrue( $this->is_sequential( $user->roles ) );
}

/**
* @ticket 42564
*/
Expand Down
Loading