Remove duplicated separators in admin menu

[rilwis]

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.

[SergeyBiryukov]

Related: [9989], #23182

[SergeyBiryukov]

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

[chriscct7]

Refreshed patch

[wonderboymusic]

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.

[johnjamesjacoby]

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

[johnjamesjacoby]

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

[johnjamesjacoby]

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]

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]

(Lengthy description imminent.)

[johnjamesjacoby]

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.

[ashokrane]

Menu with separators working as expected

[ashokrane]

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.

[johnjamesjacoby]

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

[wonderboymusic]

In 35416:

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

Props johnjamesjacoby.
Fixes #24104.

[johnjamesjacoby]

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