WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 7 months ago

Last modified 7 months ago

#24104 closed defect (bug) (fixed)

Remove duplicated separators in admin menu

Reported by: rilwis Owned by: wonderboymusic
Milestone: 4.4 Priority: normal
Severity: normal Version: 2.7
Component: Plugins Keywords: has-patch has-screenshots commit
Focuses: administration Cc:

Description

I'm working on WordPress admin menu, and I found that the code to remove duplicated separators in admin menu in wp-admin/includes/menu.php doesn't work correctly.

Assuming we have these items in the $menu:

$menu = array(
	100 => array(
		0 => 'Admin Style',
		2 => 'wp-admin-style.php',
		1 => 'read',
		4 => 'menu-top menu-icon-generic toplevel_page_wp-admin-style menu-top-first menu-top-last',
		6 => 'none',
	),
	110 => array(
		0 => '',
		1 => 'read',
		2 => 'separator1',
		3 => '',
		4 => 'wp-menu-separator',
	),
	130 => array(
		0 => '',
		1 => 'read',
		2 => 'separator2',
		3 => '',
		4 => 'wp-menu-separator',
	),
	140 => array(
		0 => '',
		1 => 'read',
		2 => 'separator-last',
		3 => '',
		4 => 'wp-menu-separator',
	),
	150 => array(
		0 => 'Hide Menu',
		1 => 'manage_options',
		2 => 'hide-admin-menu',
		3 => 'Hide Admin Menu',
		4 => 'menu-top toplevel_page_hide-admin-menu',
		5 => 'toplevel_page_hide-admin-menu',
		6 => 'http://localhost:8080/wp/wp-content/plugins/hide-admin-menu/img/icon.png',
	),
);

WordPress will remove only the 1st duplicated separator (130) and still keeps the 2nd one (140).

Another problem with current code is if separator menu item has classes like 'wp-menu-separator woocommerce' (WooCommerce does this), it won't be recognized as a separator, because the function to check separator uses strcmp.

Attachments (7)

menu.php.patch (615 bytes) - added by rilwis 3 years ago.
24104.patch (622 bytes) - added by chriscct7 8 months ago.
Screen Shot 2015-10-07 at 18.11.49.png (212.7 KB) - added by johnjamesjacoby 8 months ago.
Note the lack of separator between "Comments" & "Forums"
Screen Shot 2015-10-13 at 11.48.01.png (95.3 KB) - added by johnjamesjacoby 8 months ago.
Current state of trunk with WooCommerce, bbPress, and BuddyPress active. Note the missing separator above WooCommerce (and disregard the missing icon for "Products" as it's unrelated)
Screen Shot 2015-10-13 at 11.49.40.png (95.8 KB) - added by johnjamesjacoby 8 months ago.
State in WordPress 4.3, with WooCommerce, bbPress, and BuddyPress active. My imminent patch will aim to restore trunk to this state and address the bug noted in this ticket.
24104.02.patch (1.4 KB) - added by johnjamesjacoby 8 months ago.
(Lengthy description imminent.)
separators.png (18.2 KB) - added by ashokrane 7 months ago.
Menu with separators working as expected

Download all attachments as: .zip

Change History (22)

@rilwis
3 years ago

#1 @SergeyBiryukov
3 years ago

  • Component changed from Menus to Administration
  • Version changed from trunk to 2.7

Related: [9989], #23182

#2 @jeremyfelt
2 years ago

  • Component changed from Administration to Menus
  • Focuses administration added

#3 @SergeyBiryukov
2 years ago

  • Component changed from Menus to Plugins

Admin menu API now belongs to Plugins, according to the new component structure.

@chriscct7
8 months ago

#4 @chriscct7
8 months ago

Refreshed patch

#5 @chriscct7
8 months ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 4.4

#6 @wonderboymusic
8 months ago

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

In 34861:

Admin Menu: remove duplicated separators, and use strpos() (instead of strcmp()) when determining if the separator CSS class is present.

Props rilwis, chriscct7.
Fixes #24104.

#7 @johnjamesjacoby
8 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Vote to reopen; looks like this broke the separator approach that BuddyPress & bbPress use.

@johnjamesjacoby
8 months ago

Note the lack of separator between "Comments" & "Forums"

#8 @SergeyBiryukov
8 months ago

  • Keywords needs-patch added; has-patch commit removed

@johnjamesjacoby
8 months ago

Current state of trunk with WooCommerce, bbPress, and BuddyPress active. Note the missing separator above WooCommerce (and disregard the missing icon for "Products" as it's unrelated)

@johnjamesjacoby
8 months ago

State in WordPress 4.3, with WooCommerce, bbPress, and BuddyPress active. My imminent patch will aim to restore trunk to this state and address the bug noted in this ticket.

@johnjamesjacoby
8 months ago

(Lengthy description imminent.)

#9 @johnjamesjacoby
8 months ago

24104.02.patch proposes the following changes:

  • Use stristr(). It performs a case-insensitive haystack/needle string search, which biases the code towards being greedy and deleting duplicate separators even if someone is being clever with uppercase letters.
  • Rename the $separator_found variable to $prev_menu_was_separator to more accurately describe it's intended use. This helped me during the fix, so my thinking here is that it might help the next tortured soul that looks at it.
  • Add a bunch of inline documentation to try and make plain-english sense out of what this code is supposed to do. Again, this helped me maintain some sanity while unwinding this bit.
  • Do not falsey the $prev_menu_was_separator variable prematurely. This retains the part of [34861] that is responsible for removing triplicates and higher. If there are 8 separators in a row, only use one of them.
  • Unset the $prev_menu_was_separator variable (the previous $separator_found variable was never unset.)
  • I inverted the comparisons (false before true) because the majority of menu items will not be separators. This follows our "bail early" approach but violates our "look for proof" ideology. I'm not married to the comparison order by any means, so if someone feels strongly about maintaining a standard, please feel free to reverse this.

And finally...

  • Move this entire routine after the custom_menu_order and menu_order filters have fired, but before the $last_menu_key separator check removes a trailing separator. This is important because plugins might be adding (and also relocating) separators to the $menu global at any point in menu.php's life-cycle, and WordPress cannot confidently guess at adjacent separators until plugins have had the opportunity to reorder their custom menu items.
  • We've always referred to this code as "checking for duplicates" when it really needs to "prevent adjacency." As such, it's never worked the way we've needed it to, though it did work the way it was originally described.

This patch restores the plugins-with-separators under my watch to WordPress 4.3-and-earlier visual-status, while also addressing the adjacent-and-triplicate separator issue raised by the original reporter.

What should happen next, IMO:

  • Scour the plugins directory for other plugins using separators
  • Test with WordPress 4.3, trunk, with-and-without 02.patch applied to confirm results
  • Get a friend at Automattic to test this on WordPress.com, where there are all kinds of extra menu items that do neat things that may-or-may-not have custom separators in obscure places (network dashboard, user dashboard, etc...)
  • Celebrate our collective triumph, or mourn my having mucked this all up even further & kill menu.php with extreme prejudice in a future release.
Last edited 8 months ago by johnjamesjacoby (previous) (diff)

#10 @johnjamesjacoby
8 months ago

  • Keywords has-patch has-screenshots needs-testing added; needs-patch removed

@ashokrane
7 months ago

Menu with separators working as expected

#11 @ashokrane
7 months ago

  • Keywords needs-testing removed

The 24104.02.patch was tested with 4.3.1, trunk (with & without the patch). The patch works as expected. The plugins it was tested with: bbPress, buddyPress, Stream & WooCommerce.

This was tested during WordCamp Sofia Contributor's day.

https://core.trac.wordpress.org/raw-attachment/ticket/24104/separators.png

Last edited 7 months ago by ashokrane (previous) (diff)

#12 @johnbillion
7 months ago

  • Keywords commit added

#13 @johnjamesjacoby
7 months ago

Thanks everyone for testing & helping here. WordCamp Sofia looked like it was a ton of fun!

#14 @wonderboymusic
7 months ago

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

In 35416:

Admin Menu: after [34861], prevent adjacent separators.

Props johnjamesjacoby.
Fixes #24104.

#15 @johnjamesjacoby
7 months ago

Nice work everyone! This is an oldie and a goodie. Feels good to get this bug squished.

Note: See TracTickets for help on using tickets.