WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#32472 closed enhancement (fixed)

is_user_member_of_blog() can be very slow when a user is a member of hundreds of sites

Reported by: sammybeats Owned by: johnbillion
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.0
Component: Users Keywords: needs-testing has-patch
Focuses: multisite, performance Cc:

Description

This problem would partly be fixed if the solution in #31746 were adopted, however it is still a separate issue.

is_user_member_of_blog() uses get_blogs_of_user() which is very slow when the user is a member of hundreds of sites. And because is_user_member_of_blog() is called on every admin page load by wp_user_settings() in admin-header.php, this adds a TON of database queries to every admin page load.

get_blog_details(), which is called on every blog of which the user is a member in get_blogs_of_user(), only needs to be done on the blog in question when is_user_member_of_blog() is called. I propose a solution like this, though it may duplicate too much code from get_blogs_of_user():

function is_user_member_of_blog( $user_id = 0, $blog_id = 0 ) {
    global $wpdb;

    $user_id = (int) $user_id;
    $blog_id = (int) $blog_id;

    if ( empty( $user_id ) )
	$user_id = get_current_user_id();

    if ( empty( $blog_id ) )
	$blog_id = get_current_blog_id();

    if ( empty( $user_id ) )
        return false;

    if( ! is_multisite() )
        return true;

    $keys = get_user_meta( $user_id );
    if ( empty( $keys ) )
        return false;

    $blog = get_blog_details( $blog_id );

    if( ! $blog || ! isset( $blog->domain ) || $blog->archived || $blog->spam || $blog->deleted )
        return false;

    if ( isset( $keys[ $wpdb->base_prefix . 'capabilities' ] ) && defined( 'MULTISITE' ) && $blog_id == 1 )
        return true;

    $is_member = false;

    $keys = array_keys( $keys );

    foreach ( $keys as $key ) {
        if ( 'capabilities' !== substr( $key, -12 ) )
            continue;
        if ( $wpdb->base_prefix && 0 !== strpos( $key, $wpdb->base_prefix ) )
            continue;
        $key_blog_id = str_replace( array( $wpdb->base_prefix, '_capabilities' ), '', $key );
        if ( ! is_numeric( $key_blog_id ) )
            continue;
        if ( $key_blog_id == $blog_id ) {
            $is_member = true;
            break;
        }
    }

    return $is_member;

}

I just tested it and it reduced my admin page load time by 5 seconds.

Attachments (5)

is_user_member_of_blog.diff (1.5 KB) - added by BinaryKitten 4 years ago.
Patch for is_user_member_of_blog
is_user_member_of_blog.2.diff (1.5 KB) - added by BinaryKitten 4 years ago.
Corrected Patch File
is_user_member_of_blog.3.diff (1.7 KB) - added by BinaryKitten 4 years ago.
V3 diff - better base point
is_user_member_of_blog_(v4).diff (1.6 KB) - added by BinaryKitten 4 years ago.
v4 - with coding standards added to phpStorm
is_user_member_of_blog_(v5).diff (2.7 KB) - added by BinaryKitten 4 years ago.
Updated Patch - Based from Develop.git.wordpress.org repo, updated unit tests and code to match.

Download all attachments as: .zip

Change History (20)

#1 in reply to: ↑ description ; follow-up: @BinaryKitten
4 years ago

Replying to sammybeats:
Hi Sammybeats,

I like this conceptually, but possibly there is further scope for improvement

function is_user_member_of_blog( $user_id = 0, $blog_id = 0 ) {
    global $wpdb;

    if( ! is_multisite() )
        return true;

    $user_id = (int) $user_id;
    $blog_id = (int) $blog_id;

    if ( empty( $user_id ) )
        $user_id = get_current_user_id();

    if ( empty( $blog_id ) )
        $blog_id = get_current_blog_id();

    $blog = get_blog_details( $blog_id );

    if( ! $blog || ! isset( $blog->domain ) || $blog->archived || $blog->spam || $blog->deleted )
        return false;

    $keys = get_user_meta( $user_id );
    if ( empty( $keys ) )
        return false;

    $base_capabilities_key = $wpdb->base_prefix . '_capabilities';
    $site_capabilities_key = $wpdb->base_prefix . $blog_id . '_capabilities';

    if ( isset( $keys[ $base_capabilities_key ] ) && defined( 'MULTISITE' ) && $blog_id == 1 )
        return true;

    if ( isset( $keys[ $site_capabilities_key ] ) )
        return true;
 
}

Where there are many sites and many plugins that have added to the usermeta this prevents the need for the iteration.

#2 in reply to: ↑ 1 ; follow-up: @sammybeats
4 years ago

Replying to BinaryKitten:

Yes! Definitely an improvement on what I posted, with one (questionable) change and one correction:

function is_user_member_of_blog( $user_id = 0, $blog_id = 0 ) {
    global $wpdb;

    if( ! is_multisite() )
        return true;

    $user_id = (int) $user_id;
    $blog_id = (int) $blog_id;

    if ( empty( $user_id ) )
        $user_id = get_current_user_id();

    //Technically not needed, but does save calls to get_blog_details and get_user_meta
    //in the event that the function is called when a user isn't logged in
    if ( empty( $user_id ) )
        return false;

    if ( empty( $blog_id ) )
        $blog_id = get_current_blog_id();

    $blog = get_blog_details( $blog_id );

    if( ! $blog || ! isset( $blog->domain ) || $blog->archived || $blog->spam || $blog->deleted )
        return false;

    $keys = get_user_meta( $user_id );
    if ( empty( $keys ) )
        return false;

    //no underscore before capabilities in $base_capabilities_key
    $base_capabilities_key = $wpdb->base_prefix . 'capabilities';
    $site_capabilities_key = $wpdb->base_prefix . $blog_id . '_capabilities';

    if ( isset( $keys[ $base_capabilities_key ] ) && defined( 'MULTISITE' ) && $blog_id == 1 )
        return true;

    if ( isset( $keys[ $site_capabilities_key ] ) )
        return true;

    return false;

}

#3 in reply to: ↑ 2 ; follow-up: @BinaryKitten
4 years ago

Replying to sammybeats:

Yes! Definitely an improvement on what I posted, with one (questionable) change and one correction:

I can live with that. Those errors teach me not to try and code in a browser text box.

I've taken the code and created a patch/diff (see attached)

@BinaryKitten
4 years ago

Patch for is_user_member_of_blog

#4 in reply to: ↑ 3 @sammybeats
4 years ago

Replying to BinaryKitten:

I've taken the code and created a patch/diff (see attached)

Thanks!

#5 @BinaryKitten
4 years ago

Patch i think is broken due to where i made it from. Attempting 2nd attempt from better location

@BinaryKitten
4 years ago

Corrected Patch File

@BinaryKitten
4 years ago

V3 diff - better base point

#6 @rmccue
4 years ago

@BinaryKitten: Thanks for the patch. :) Please ensure you have your editor set to use tabs rather than spaces, as per the WordPress coding standards.

@BinaryKitten
4 years ago

v4 - with coding standards added to phpStorm

#7 @BinaryKitten
4 years ago

@rmccue - thanks for the reminder. I'm usually set to PSR-2 coding standards, forgot that core WP coding style is different.

updated the patch with the standards - hopefully this is working right.

#8 follow-up: @johnbillion
4 years ago

  • Keywords has-patch needs-unit-tests added
  • Type changed from defect (bug) to enhancement

This looks like a sane way to handle this. +1

It'll need tests, including ones to make sure that it continues to function as expected when a user is added to then removed from a blog, has their role removed, etc.

#9 in reply to: ↑ 8 @BinaryKitten
4 years ago

Replying to johnbillion:

This looks like a sane way to handle this. +1

It'll need tests, including ones to make sure that it continues to function as expected when a user is added to then removed from a blog, has their role removed, etc.

Hi @johnbillion

Would there be some guidance for this process?

cheers

#10 @johnbillion
4 years ago

  • Keywords needs-testing added; needs-unit-tests removed
  • Milestone changed from Awaiting Review to Future Release

Sorry I missed your comment @BinaryKitten.

The core handbook contains information on unit testing. The section on writing new tests is bare, but there are already two tests for is_user_member_of_blog(). The single site test is here and the multisite test is here.

With is_user_member_of_blog_(v4).diff applied, the single site test fails with the following error:

1) Tests_User::test_is_user_member_of_blog
Failed asserting that true is false.
~/tests/phpunit/tests/user.php:365

This means that there may be a caching issue when users are deleted, or your patch fails to account for deleted users. Take a look and see where you get to.

This ticket was mentioned in Slack in #core by binarykitten. View the logs.


4 years ago

#12 @johnbillion
4 years ago

  • Keywords needs-patch added; has-patch removed

Chatted with binarykitten about this last week. Awaiting an updated patch.

@BinaryKitten
4 years ago

Updated Patch - Based from Develop.git.wordpress.org repo, updated unit tests and code to match.

#13 @BinaryKitten
4 years ago

  • Keywords has-patch added; needs-patch removed

Added New patch file - /cc @johnbillion

#14 @johnbillion
4 years ago

  • Milestone changed from Future Release to 4.4
  • Owner set to johnbillion
  • Status changed from new to reviewing

#15 @johnbillion
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 33771:

Improve the efficiency of is_user_member_of_blog() by removing its use of get_blogs_of_user(). Adds additional tests.

Fixes #32472
Props BinaryKitten, sammybeats, johnbillion

Note: See TracTickets for help on using tickets.