WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 10 months ago

Last modified 10 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 11 months ago.
Screen Shot 2015-10-07 at 18.11.49.png (212.7 KB) - added by johnjamesjacoby 11 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 11 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 11 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 11 months ago.
(Lengthy description imminent.)
separators.png (18.2 KB) - added by ashokrane 10 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
3 years ago

  • Component changed from Administration to Menus
  • Focuses administration added

#3 @SergeyBiryukov
3 years ago

  • Component changed from Menus to Plugins

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

@chriscct7
11 months ago

#4 @chriscct7
11 months ago

Refreshed patch

#5 @chriscct7
11 months ago

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

#6 @wonderboymusic
11 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
11 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
11 months ago

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

#8 @SergeyBiryukov
11 months ago

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

@johnjamesjacoby
11 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
11 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
11 months ago

(Lengthy description imminent.)

#9 @johnjamesjacoby
11 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 11 months ago by johnjamesjacoby (previous) (diff)

#10 @johnjamesjacoby
11 months ago

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

@ashokrane
10 months ago

Menu with separators working as expected

#11 @ashokrane
10 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 10 months ago by ashokrane (previous) (diff)

#12 @johnbillion
10 months ago

  • Keywords commit added

#13 @johnjamesjacoby
10 months ago

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

#14 @wonderboymusic
10 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
10 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.