Make WordPress Core

Opened 12 months ago

Closed 6 months ago

#58361 closed defect (bug) (fixed)

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

Reported by: andrewleap's profile andrewleap Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.5 Priority: normal
Severity: minor Version: 3.0
Component: Administration Keywords: has-patch needs-testing commit
Focuses: Cc:

Description

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] 

Attachments (3)

58361.diff (817 bytes) - added by andrewleap 12 months ago.
patch
58361.2.diff (823 bytes) - added by andrewleap 12 months ago.
forked from correct version
58361.3.diff (930 bytes) - added by ironprogrammer 6 months ago.
Formatting and comment adjusted for readability.

Download all attachments as: .zip

Change History (12)

@andrewleap
12 months ago

patch

@andrewleap
12 months ago

forked from correct version

This ticket was mentioned in Slack in #core-test by ironprogrammer. View the logs.


12 months ago

#2 follow-up: @ironprogrammer
12 months ago

  • Keywords reporter-feedback added

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.

#3 in reply to: ↑ 2 @andrewleap
7 months ago

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 using by passing the "none" option as documented here: 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

Version 0, edited 7 months ago by andrewleap (next)

This ticket was mentioned in Slack in #core-test by andrewleap. View the logs.


7 months ago

#5 @andrewleap
7 months ago

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
6 months ago

Formatting and comment adjusted for readability.

#6 @ironprogrammer
6 months ago

  • Keywords has-patch needs-testing dev-feedback added; reporter-feedback removed
  • Version changed from 6.2.1 to 3.0

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>
    

#7 @azaozz
6 months ago

  • Keywords commit added; dev-feedback removed

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.

#8 @SergeyBiryukov
6 months ago

  • Milestone changed from Awaiting Review to 6.5

#9 @SergeyBiryukov
6 months ago

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

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.

Note: See TracTickets for help on using tickets.