Skip to content

Refactor multisite and network admin checks for plugin screen #10146

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
wants to merge 1 commit into from
+23 −20
Closed

Refactor multisite and network admin checks for plugin screen #10146

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
43 changes: 23 additions & 20 deletions src/wp-admin/plugins.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
$plugin = isset( $_REQUEST['plugin'] ) ? wp_unslash( $_REQUEST['plugin'] ) : '';
$s = isset( $_REQUEST['s'] ) ? urlencode( wp_unslash( $_REQUEST['s'] ) ) : '';

$is_multisite = is_multisite();
$is_network_admin = is_network_admin();
Comment on lines +24 to +25
Copy link
Member

Choose a reason for hiding this comment

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

It looks like these functions don't really do any work. They're either just looking at constants or the value of a class member variable. So doubt there would be any measurable improvement to doing this, but I'm happy to be wrong!

Copy link
Member Author

Choose a reason for hiding this comment

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

I do the benchmark for function call and local variable use over 100 iterations and found that it will improve ~50% time.

Average time spent for 1 x 100 iterations:
Use function - 5583
Store in local var - 1958
Change: -64.9%

Copy link
Member

Choose a reason for hiding this comment

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

What is the unit of time here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Benchmark script 
// Bootstrapping.

define('WP_USE_THEMES', true);

/** Loads the WordPress Environment and Template */
require ('wp-load.php');

// Setup.


$repetitions = 1;
$iterations  = 100;

// Profiling.

for ( $repetition = 0; $repetition <= $repetitions; $repetition ++ ) {

	if( $repetition == 0 ) {
		continue;
	}

	$time_start = hrtime( true );

	for ( $index = 0; $index < $iterations; $index ++ ) {
        is_multisite();
		is_network_admin();
	}

	$time_end                    = hrtime( true );
	$function[ $repetition ] = $time_end - $time_start;

	$time_start = hrtime( true );

    $is_multisite     = is_multisite();
    $is_network_admin = is_network_admin();
	for ( $index = 0; $index < $iterations; $index ++ ) {
		$call_m = $is_multisite;
        $call_n = $is_network_admin;
	}

	$time_end                 = hrtime( true );
	$local_var[ $repetition ] = $time_end - $time_start;
}

$function_avg  = array_sum( $function ) / $repetitions;
$local_var_avg = array_sum( $local_var ) / $repetitions;
$percentage    = round( $local_var_avg / $function_avg * 100, 1 ) - 100;

printf( "Average time spent for {$repetitions} x {$iterations} iterations:<br>" );
printf( "Use function - {$function_avg} <br>");
printf( "Store in local var - {$local_var_avg}<br>" );
printf( "Change: {$percentage}%%" );

Copy link
Member

Choose a reason for hiding this comment

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

OK, so hrtime() returns nanoseconds. That means 5,583 nanoseconds is 0.005583 milliseconds. This is extremely tiny. I don't think it's necessarily bad to make this change, except for possibility that a plugin could override these globals and cause something unexpected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, @westonruter! What do you think should be the next step here?

Copy link
Member

Choose a reason for hiding this comment

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

Well, I don't really see the need for this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Close the Ticket and PR.


// Clean up request URI from temporary args for screen options/paging uri's to work as expected.
$query_args_to_remove = array(
'error',
Expand Down Expand Up @@ -50,14 +53,14 @@
wp_die( __( 'Sorry, you are not allowed to activate this plugin.' ) );
}

if ( is_multisite() && ! is_network_admin() && is_network_only_plugin( $plugin ) ) {
if ( $is_multisite && ! $is_network_admin && is_network_only_plugin( $plugin ) ) {
wp_redirect( self_admin_url( "plugins.php?plugin_status=$status&paged=$page&s=$s" ) );
exit;
}

check_admin_referer( 'activate-plugin_' . $plugin );

$result = activate_plugin( $plugin, self_admin_url( 'plugins.php?error=true&plugin=' . urlencode( $plugin ) ), is_network_admin() );
$result = activate_plugin( $plugin, self_admin_url( 'plugins.php?error=true&plugin=' . urlencode( $plugin ) ), $is_network_admin );
if ( is_wp_error( $result ) ) {
if ( 'unexpected_output' === $result->get_error_code() ) {
$redirect = self_admin_url( 'plugins.php?error=true&charsout=' . strlen( $result->get_error_data() ) . '&plugin=' . urlencode( $plugin ) . "&plugin_status=$status&paged=$page&s=$s" );
Expand All @@ -68,7 +71,7 @@
}
}

if ( ! is_network_admin() ) {
if ( ! $is_network_admin ) {
$recent = (array) get_option( 'recently_activated' );
unset( $recent[ $plugin ] );
update_option( 'recently_activated', $recent, false );
Expand Down Expand Up @@ -98,7 +101,7 @@

$plugins = isset( $_POST['checked'] ) ? (array) wp_unslash( $_POST['checked'] ) : array();

if ( is_network_admin() ) {
if ( $is_network_admin ) {
foreach ( $plugins as $i => $plugin ) {
// Only activate plugins which are not already network activated.
if ( is_plugin_active_for_network( $plugin ) ) {
Expand All @@ -108,7 +111,7 @@
} else {
foreach ( $plugins as $i => $plugin ) {
// Only activate plugins which are not already active and are not network-only when on Multisite.
if ( is_plugin_active( $plugin ) || ( is_multisite() && is_network_only_plugin( $plugin ) ) ) {
if ( is_plugin_active( $plugin ) || ( $is_multisite && is_network_only_plugin( $plugin ) ) ) {
unset( $plugins[ $i ] );
}
// Only activate plugins which the user can activate.
Expand All @@ -123,9 +126,9 @@
exit;
}

activate_plugins( $plugins, self_admin_url( 'plugins.php?error=true' ), is_network_admin() );
activate_plugins( $plugins, self_admin_url( 'plugins.php?error=true' ), $is_network_admin );

if ( ! is_network_admin() ) {
if ( ! $is_network_admin ) {
$recent = (array) get_option( 'recently_activated' );
} else {
$recent = (array) get_site_option( 'recently_activated' );
Expand All @@ -135,7 +138,7 @@
unset( $recent[ $plugin ] );
}

if ( ! is_network_admin() ) {
if ( ! $is_network_admin ) {
update_option( 'recently_activated', $recent, false );
} else {
update_site_option( 'recently_activated', $recent );
Expand Down Expand Up @@ -203,14 +206,14 @@

check_admin_referer( 'deactivate-plugin_' . $plugin );

if ( ! is_network_admin() && is_plugin_active_for_network( $plugin ) ) {
if ( ! $is_network_admin && is_plugin_active_for_network( $plugin ) ) {
wp_redirect( self_admin_url( "plugins.php?plugin_status=$status&paged=$page&s=$s" ) );
exit;
}

deactivate_plugins( $plugin, false, is_network_admin() );
deactivate_plugins( $plugin, false, $is_network_admin );

if ( ! is_network_admin() ) {
if ( ! $is_network_admin ) {
update_option( 'recently_activated', array( $plugin => time() ) + (array) get_option( 'recently_activated' ), false );
} else {
update_site_option( 'recently_activated', array( $plugin => time() ) + (array) get_site_option( 'recently_activated' ) );
Expand All @@ -232,7 +235,7 @@

$plugins = isset( $_POST['checked'] ) ? (array) wp_unslash( $_POST['checked'] ) : array();
// Do not deactivate plugins which are already deactivated.
if ( is_network_admin() ) {
if ( $is_network_admin ) {
$plugins = array_filter( $plugins, 'is_plugin_active_for_network' );
} else {
$plugins = array_filter( $plugins, 'is_plugin_active' );
Expand All @@ -250,14 +253,14 @@
exit;
}

deactivate_plugins( $plugins, false, is_network_admin() );
deactivate_plugins( $plugins, false, $is_network_admin );

$deactivated = array();
foreach ( $plugins as $plugin ) {
$deactivated[ $plugin ] = time();
}

if ( ! is_network_admin() ) {
if ( ! $is_network_admin ) {
update_option( 'recently_activated', $deactivated + (array) get_option( 'recently_activated' ), false );
} else {
update_site_option( 'recently_activated', $deactivated + (array) get_site_option( 'recently_activated' ) );
Expand Down Expand Up @@ -342,7 +345,7 @@
<?php if ( 1 === $plugins_to_delete ) : ?>
<h1><?php _e( 'Delete Plugin' ); ?></h1>
<?php
if ( $have_non_network_plugins && is_network_admin() ) :
if ( $have_non_network_plugins && $is_network_admin ) :
$maybe_active_plugin = '<strong>' . __( 'Caution:' ) . '</strong> ' . __( 'This plugin may be active on other sites in the network.' );
wp_admin_notice(
$maybe_active_plugin,
Expand All @@ -356,7 +359,7 @@
<?php else : ?>
<h1><?php _e( 'Delete Plugins' ); ?></h1>
<?php
if ( $have_non_network_plugins && is_network_admin() ) :
if ( $have_non_network_plugins && $is_network_admin ) :
$maybe_active_plugins = '<strong>' . __( 'Caution:' ) . '</strong> ' . __( 'These plugins may be active on other sites in the network.' );
wp_admin_notice(
$maybe_active_plugins,
Expand Down Expand Up @@ -435,15 +438,15 @@
wp_redirect( self_admin_url( "plugins.php?deleted=$plugins_to_delete&plugin_status=$status&paged=$page&s=$s" ) );
exit;
case 'clear-recent-list':
if ( ! is_network_admin() ) {
if ( ! $is_network_admin ) {
update_option( 'recently_activated', array(), false );
} else {
update_site_option( 'recently_activated', array() );
}

break;
case 'resume':
if ( is_multisite() ) {
if ( $is_multisite ) {
return;
}

Expand All @@ -469,7 +472,7 @@
wp_die( __( 'Sorry, you are not allowed to manage plugins automatic updates.' ) );
}

if ( is_multisite() && ! is_network_admin() ) {
if ( $is_multisite && ! $is_network_admin ) {
wp_die( __( 'Please connect to your network admin to manage plugins automatic updates.' ) );
}

Expand Down Expand Up @@ -765,7 +768,7 @@
</h1>

<?php
if ( ( ! is_multisite() || is_network_admin() ) && current_user_can( 'install_plugins' ) ) {
if ( ( ! $is_multisite || $is_network_admin ) && current_user_can( 'install_plugins' ) ) {
?>
<a href="<?php echo esc_url( self_admin_url( 'plugin-install.php' ) ); ?>" class="page-title-action"><?php echo esc_html__( 'Add Plugin' ); ?></a>
<?php
Expand Down
Loading