Make WordPress Core

Opened 11 years ago

Last modified 3 months ago

#32422 new defect (bug)

add_menu_page() returns incorrect $hookname when using plugin filename as $menu_slug

Reported by: quinncom's profile quinncom Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.2.2
Component: Plugins Keywords: has-patch reporter-feedback 2nd-opinion
Focuses: administration Cc:

Description

When using add_menu_page() and add_submenu_page() with filenames in the $menu_slug parameter, the resulting hookname does not match the expected do_action('load-' …) hookname.

In the following examples the expected hookname from do_action( 'load-' . $plugin_page ); (in wp-admin/admin.php) is: load-myplugin/admin/resources.php. However, the hookname returned by $hookname = get_plugin_page_hookname( $menu_slug, $parent_slug); (from add_submenu_page() in wp-admin/includes/plugin.php) is myplugin_page_myplugin/admin/resources.

The result is, this code does not work:

$hookname = add_submenu_page("myplugin/dashboard.php", 'Resources', 'Resources', 'edit_pages', "myplugin/resources.php");
add_action($hookname, 'add_resources_options' );
function add_resources_options() {
    add_screen_option('per_page', array(
        'label' => 'Resources',
        'default' => 10,
        'option' => 'resources_per_page'
    ));
}

However, the hooknames do match when using add_menu_page() and add_submenu_page() with a callback slugname. This code works (notice only the first line has been changed to use callback slugs instead of filename slugs):

$hookname = add_submenu_page('dashboard', 'Resources', 'Resources', 'edit_pages', 'myresources', 'myresources');
add_action($hookname, 'add_resources_options' );
function add_resources_options() {
    add_screen_option('per_page', array(
        'label' => 'Resources',
        'default' => 10,
        'option' => 'resources_per_page'
    ));
}

The solution is either:

  • Modify function get_plugin_page_hookname() … to return a hookname that is caught by do_action( 'load-' . $plugin_page );.
  • Or modify do_action( 'load-' . $plugin_page ); to use the hookname currently returned by get_plugin_page_hookname().

Attachments (1)

32422.1.patch (1.1 KB) - added by johnjamesjacoby 10 years ago.

Download all attachments as: .zip

Change History (6)

#1 @johnjamesjacoby
10 years ago

Related: #16238, #28226.

Perhaps also related, the menu manipulation functions also do not return adequate -network or -user suffixes.

Incoming patch will attempt to address all of these issues together.

#2 @johnjamesjacoby
10 years ago

32422.1.patch introduces the following changes:

  • Remove the -network & -user appending bit in class-wp-screen.php WP_Screen::get()
  • Append -network or -user in get_plugin_page_hookname() instead

This assumes get_plugin_page_hookname() is not contextually strict, and might be a problem if you use it in the site context in the network admin. In this case, the -network suffix is added via a not-network-specific hook. Now that I mention it, this might already happen but inversely the way the code is today.

#3 @johnjamesjacoby
10 years ago

  • Keywords has-patch needs-testing reporter-feedback added

#4 @DrewAPicture
9 years ago

Related-ish #35305.

#5 @huzaifaalmesbah
3 months ago

  • Keywords 2nd-opinion added; needs-testing removed

I attempted to test the attached patch using:

npm run grunt patch:32422

During patching, the following message appeared:

Reversed (or previously applied) patch detected! Assume -R? [y]

This usually indicates that the patch may have already been applied, partially applied via another ticket, or that the patch is too outdated to apply cleanly against current trunk.

This is a very old ticket and core has changed significantly since the patch was created. It is also possible that the original issue has been addressed elsewhere over time.

Because of this:

  • Removing needs-testing
  • Keeping reporter-feedback to confirm whether the issue still exists in current WordPress
  • Adding 2nd-opinion due to uncertainty whether the issue is still reproducible or already solved indirectly

A confirmation that the issue still exists in current trunk would help move this ticket forward.

Note: See TracTickets for help on using tickets.