Passing 'none' as 'menu_icon' to 'register_post_type' is not working correctly

[andrewleap]

The resultant menu item contains:

<img src="http://none" alt="">

This is presumably because

$menu_icon = esc_url( $ptype_obj->menu_icon );

was wrapped in esc_url in wp-admin/menu.php at some point, so it can no longer be correctly compared in wp-admin/menu-header.php with

 'none' === $item[6] || 'div' === $item[6] 

[andrewleap]

patch

[andrewleap]

forked from correct version

[ironprogrammer]

Thanks for the report, @andrewleap, and welcome back to Trac!

register_post_type() expects that menu_icon is a URL, Dashicon, or base64-encoded SVG. Please see ​https://developer.wordpress.org/reference/functions/register_post_type/#menu_icon. Is your intent to not display any icon at all for the menu?

Background

esc_url( $ptype_obj->menu_icon ) has existed since https://core.trac.wordpress.org/browser/trunk/wp-admin/menu.php?rev=14097#L119, and provided a way to add a menu icon by URL.

Looking deeper into the intent behind wp-admin/menu-header.php's check for none or div (see r21877), it seems these values were to be assigned internally by wp-admin/menu.php in the case that menu_icon was null. That behavior has since evolved to ​display the default posts icon.

[andrewleap]

Replying to ironprogrammer:

Thanks for the report, @andrewleap, and welcome back to Trac!

register_post_type() expects that menu_icon is a URL, Dashicon, or base64-encoded SVG. Please see ​https://developer.wordpress.org/reference/functions/register_post_type/#menu_icon. Is your intent to not display any icon at all for the menu?

Background

esc_url( $ptype_obj->menu_icon ) has existed since https://core.trac.wordpress.org/browser/trunk/wp-admin/menu.php?rev=14097#L119, and provided a way to add a menu icon by URL.

Looking deeper into the intent behind wp-admin/menu-header.php's check for none or div (see r21877), it seems these values were to be assigned internally by wp-admin/menu.php in the case that menu_icon was null. That behavior has since evolved to ​display the default posts icon.

The intention was to include an icon from Goole material icons via css by passing the "none" option as documented here: (see the menu_icon parameter) ​https://developer.wordpress.org/reference/functions/register_post_type/#parameters but it does not work. Rather than emitting and empty div, it emits <img src="http://none" alt="">

This is because $menu_icon is escaped with esc_url, so when checking to see if it is equal to 'none', this check fails

[andrewleap]

If more clarification is needed, search the source for:

		/*
		 * If the string 'none' (previously 'div') is passed instead of a URL, don't output
		 * the default menu image so an icon can be added to div.wp-menu-image as background
		 * with CSS. Dashicons and base64-encoded data:image/svg_xml URIs are also handled
		 * as special cases.
		 */

Hopefully it will be clear what I'm referring to.

[ironprogrammer]

Formatting and comment adjusted for readability.

[ironprogrammer]

I understand now, thanks, @andrewleap 👍🏻 The patch looks good for addressing this issue, and I've made some slight formatting adjustments for readability, reflected in attachment:58361.3.diff​.

I've most often seen the div's background set for custom menu icons (and using display: none on the img), but I suppose there's a chance an extender has tried to "fix" the broken img by setting a valid src 🤔 Unsure what backward compatibility implications this might have, since the use of esc_url( $ptype_obj->menu_icon ) has been around ​since WP 3.0. Additional input from a committer would be helpful here. CC @azaozz in case he has a chance to take a look.

Test Report

Patch tested: attachment:58361.3.diff​

Steps to Reproduce or Test

1. Register a custom post type, setting the menu_icon argument to 'none' (​e.g. this example gist to use as mu-plugin).
2. Load WP admin and inspect the HTML for the "Books" menu item.
3. Observe the markup in div.wp-menu-image.

Expected Results

When reproducing the bug:

• 🐛 A broken image, <img src="http://none" alt=""> is rendered in the div.

When testing a patch to validate it works as expected:

• 👍🏻 A line break, <br>, should be rendered in the div when menu_icon is set to 'none' or 'div'.

Environment

• Hardware: MacBook Pro Apple M1 Pro
• OS: macOS 13.6
• Browser: Safari 17.0
• Server: nginx/1.25.3
• PHP: 8.2.12
• WordPress: 6.5-alpha-56966-src
• Theme: twentytwentyfour v1.0
• Active Plugins:
  • gutenberg v17.0.0
  • test-cpt (the CPT test plugin noted in steps to repro above)

Actual Results

When reproducing the bug:

• ✅ I was able to reproduce this issue.

  <div class="wp-menu-image dashicons-before" aria-hidden="true">
      <img src="http://none" alt="">
  </div>
  <div class="wp-menu-name">Books</div>
  

When testing the patch:

• ✅ Issue was resolved with patch.

  <div class="wp-menu-image dashicons-before" aria-hidden="true">
      <br>
  </div>
  <div class="wp-menu-name">Books</div>
  

[azaozz]

Thanks for the ping @ironprogrammer. 58361.3.diff seems correct: none and div should not be escaped with esc_url(), they are special cases not URLs.

[SergeyBiryukov]

In 57159:

Administration: Don't unnecessarily escape none or div in the admin menu.

This matches a similar conditional in wp-admin/menu-header.php, where these values are handled as special cases and don't output the default menu image so that an icon could be added to div.wp-menu-image as CSS background.

Follow-up to [9578], [21877], [26664].

Props andrewleap, ironprogrammer, azaozz.
Fixes #58361.