Ticket #11763 (closed defect (bug): fixed)

Opened 2 years ago

Last modified 2 years ago

Inappropriate admin menu items aren't necessarily removed

Reported by: Denis-de-Bernardy Owned by: ryan
Priority: normal Milestone: 3.0
Component: Menus Version: 3.0
Severity: normal Keywords: multisite
Cc:

Description

With MU going mainstream alongside an increasing number of plugins that disable admin menus, we're bound to see sites where the following lines of code no longer work:

http://core.trac.wordpress.org/browser/trunk/wp-admin/includes/ms.php?rev=12603#L565

	if( !is_site_admin() )
		unset( $submenu['plugins.php'][10] ); // always remove the plugin installer for regular users
	unset( $submenu['plugins.php'][15] ); // always remove the plugin editor
	unset( $submenu['themes.php'][10] ); // always remove the themes editor

instead, they should not be introduced in the first place. and ideally, we should add some kind of remove_menu_item() and remove_submenu_item() functionality.

Attachments

11763.diff Download (686 bytes) - added by Denis-de-Bernardy 2 years ago.
11763.2.diff Download (6.4 KB) - added by Denis-de-Bernardy 2 years ago.

Change History

there are a few more such items further down in wpmu_menu()...

comment:2   ryan2 years ago

Let's kill that stuff and do is_multisite() checks directly in menu.php.

  • Keywords multisite added

I've attached an extra fix

comment:6   ryan2 years ago

(In [12614]) Show theme installer only to super admins. Props Denis-de-Bernardy. see #11763

second patch completely empties the function.

comment:8   ryan2 years ago

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

(In [12618]) Move wpmu_menu() logic into menu.php. Props Denis-de-Bernardy. fixes #11763

I like the idea of leaving the skeleton of any functions we're deprecating like this, that way we can go back up and clean up everything that was deprecated later.

That said, we should also account for any actions that are removed as well. MU plugin developers are having their codebase being completely reworked and will want to know what has disappeared; I've also been thinking about a _deprecated_hook function for the half-dozen that are in 2.9 and however many more we'll be deprecating in 3.0.

comment:10 follow-up: ↓ 11   dd322 years ago

  • Status changed from closed to reopened
  • Resolution fixed deleted

How about getting rid of that create_function() there in trunk/wp-admin/plugins.php? Theres nothing there that cant simply be added to a function.

Replacement code: (Dont have Diff on me right now)

if ( is_multisite() && is_super_admin() ) {
	$menu_perms = get_site_option('menu_items', array());
	if ( !$menu_perms['plugins'] ) {
		add_action( 'admin_notices', '_ms_plugins_not_visible');
		function _ms_plugins_not_visible() {
			echo "<div class='error'><p>";
			printf( __( 'The plugins page is not visible to normal users. It must be activated first. %s' ), '<a href="ms-options.php#menu">' . __( 'Activate' ) . '</a>' );
			echo "</p></div>";
		}
	}
}

comment:11 in reply to: ↑ 10   westi2 years ago

Replying to dd32:

How about getting rid of that create_function() there in trunk/wp-admin/plugins.php? Theres nothing there that cant simply be added to a function.

Agreed. We should avoid create_function as much as possible due to the possible security issues with the way it handles the php code passed to it.

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

(In [12636]) Switch from create_function to a fixed function for the plugin page activation admin notice. Fixes #11763 props dd32.

Note: See TracTickets for help on using tickets.