Make WordPress Core

Opened 14 years ago

Last modified 7 hours ago

#19085 new defect (bug)

Removing First Submenu Page in Admin Menu breaks URL for Menu Page

Reported by: mikeschinkel's profile mikeschinkel Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.1
Component: Administration Keywords: has-patch needs-test-info needs-docs
Focuses: Cc:

Description

If you attempt to remove the Post Type Submenu Page in the Admin it breaks the Menu Page URL; it causes the Menu Page URL to be the same as the new first Submenu Page URL:

http://screenshots.newclarity.net/skitched-20111029-142108.png

Here is a simple class you can drop into the theme's functions.php file to experience this bug. This example is a minimum to trigger the error (I simplified the register_post_type() call so the example code would have fewer lines):

<?php

class Trigger_Admin_Menu_Bug {
  static function on_load() {
    add_action( 'init', array( __CLASS__, 'init' ) );
    add_action( 'parent_file', array( __CLASS__, 'parent_file' ) );
  }
  static function init() {
    global $wpdb;
    register_post_type( 'test-cpt', array(
      'label'   => 'Test CPT',
      'show_ui' => true,
    ));
  }
  static function parent_file( $parent_file ) {
    remove_submenu_page( 'edit.php?post_type=test-cpt', 
                         'edit.php?post_type=test-cpt' );
    return $parent_file;
  }
}
Trigger_Admin_Menu_Bug::on_load();

I'd provide a patch but the admin menu code is more complex than I can fully understand. Maybe the person who originally wrote it could fix it?

Note: Sadly, this is a blocker for one of my client projects. The client wants the admin menus simplified to reduce the conceptual load on their end users because we are adding many other submenu pages. Plus I've traced through the core WP code with a debugger for many hours looking for hooks that would allow me to get around this issue, but there simply are no hooks where it would be needed to hack a fix for this.

Attachments (2)

19085.patch (1.1 KB) - added by pentatonicfunk 12 years ago.
submenuasparent.19085.diff (971 bytes) - added by chrisguitarguy 10 years ago.
Alternative take on filtering $submenu_as_parent

Download all attachments as: .zip

Change History (17)

#1 @mikeschinkel
14 years ago

  • Cc mikeschinkel@… added
  • Summary changed from Deleting First Submenu Page in Admin Menu breaks URL for Menu Page to Removing First Submenu Page in Admin Menu breaks URL for Menu Page

#2 @ocean90
14 years ago

  • Version changed from 3.3 to 3.2

#3 follow-up: @ocean90
14 years ago

  • Version changed from 3.2 to 3.1

This also happens for the default post post type.

add_action( 'admin_menu', 'ds_remove_first' );

function ds_remove_first() {
	remove_submenu_page( 'edit.php', 'edit.php' );
}

"If the first submenu is not the same as the assigned parent, make the first submenu the new parent."
http://core.trac.wordpress.org/browser/trunk/wp-admin/includes/menu.php#L74

Last edited 14 years ago by ocean90 (previous) (diff)

#4 in reply to: ↑ 3 @mikeschinkel
14 years ago

Replying to ocean90:

"If the first submenu is not the same as the assigned parent, make the first submenu the new parent."
http://core.trac.wordpress.org/browser/trunk/wp-admin/includes/menu.php#L74

Ugh. Good catch.

Obviously unintended side effects?

BTW, that is not the only place that takes control of the admin menus; it is upstream of _wp_menu_output() which itself outputs things that are different from what's in the global $submenu array. I know this because I tried to fix the $submenu array in the 'parent_file' hook, but that only made matters worse.

Version 1, edited 14 years ago by mikeschinkel (previous) (next) (diff)

#5 @rmarks
12 years ago

I tested this in 3.7-alpha and found that the parent menu item honored the URL of the first submenu item. So, Test CPT and Add New both had the URL of /wp-admin/post-new.php?post_type=test-cpt

#6 @plocha
12 years ago

  • Keywords has-patch added; needs-patch removed

I wrote a patch for that issue. You can remove the first submenu entry of any menu if you add a filter which returns false. This patch does not introduce new functionality. It only make it possible to change an as yet unused parameter for '_wp_menu_output'.

Last edited 12 years ago by plocha (previous) (diff)

#7 @plocha
12 years ago

#25205 was marked as a duplicate.

#8 @eclare
12 years ago

  • Cc eclare added

This is a bigger issue - one can not make a link to the Menu item that is different from the 1st Submenu item.
I know that "it's not a bug - it's a feature", but the side effect - like said above - is quite bad.
This prevents removing some submenus, for example if I want to remove the "Home" that is the 1st submenu of the Dashboard menu, I use the following code:

add_action('admin_menu', 'register_custom_admin_menus');
function register_custom_admin_menus() {
remove_submenu_page('index.php', 'index.php');
}

This will remove the "Home" menu, but as stated above, the current WP code makes it so the menu link for the Dashboard changes to the next submenu item, which is update-core.php (Update). This makes it impossible to access the Dashboard. This is just an example.

I do not believe that plocha's patch fixes this. It looks like it only allows you to remove a submenu that has the same link as the main menu (which I believe is working without this patch), but it still doesn't fix the issue that the Menu links gets the new link from the 1st Submenu page.

Therefore, I think this should be marked as needs-patch. Please correct me if I'm wrong.

#9 @kpdesign
12 years ago

  • Keywords needs-patch added; has-patch removed

Fixing keywords, no patches currently attached to this ticket.

#10 @pentatonicfunk
12 years ago

  • Keywords needs-testing added; needs-patch removed

don't be harsh, my first patch

#11 @kpdesign
12 years ago

  • Keywords has-patch added

#13 @chriscct7
10 years ago

  • Severity changed from major to normal

@chrisguitarguy
10 years ago

Alternative take on filtering $submenu_as_parent

#14 @chrisguitarguy
10 years ago

The patch I just added is an alterntive take on filtering here that I think makes sense. Keep all the behavior the same as before, which works for 99% of people. But add two filters. One to change the global $submenu_as_parent value, the other to change the beavhior on individual admin pages. Meaning you can do something like this:

<?php
add_action('admin_menu', 'wpse206471_add_pages');
function wpse206471_add_pages()
{
    $hook = add_menu_page(
        __('WPSE206471 Main Page', 'wpse206471'),
        __('Main Page', 'wpse206471'),
        'manage_options',
        'wpse206471-main',
        'wpse206471_main_page'
    );
    add_submenu_page(
        'wpse206471-main',
        __('WPSE206471 Sub Page', 'wpse206471'),
        __('Sub Page', 'wpse206471'),
        'manage_options',
        'wpse206471-sub',
        'wpse206471_sub_page'
    );

    // remove the "main" submenue page
    remove_submenu_page('wpse206471-main', 'wpse206471-main');
    // tell `_wp_menu_output` not to use the submenu item as its link
    add_filter("submenu_as_parent_{$hook}", '__return_false');
}
Last edited 10 years ago by chrisguitarguy (previous) (diff)

#15 @SirLouen
7 hours ago

  • Keywords needs-test-info needs-docs added; needs-testing removed

Test Report

Description

🟠 This report partially validates that the indicated patch works as expected.

Patch tested: https://core.trac.wordpress.org/attachment/ticket/19085/submenuasparent.19085.diff

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 7.4.33
  • Server: nginx/1.27.5
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 7.4.33)
  • Browser: Chrome 137.0.0.0
  • OS: Windows 10/11
  • Theme: Twenty Twenty-Five 1.2
  • MU Plugins: None activated
  • Plugins:
    • Custom Plugin 1.0
    • Test Reports 1.2.0

Test Instructions

  • Keep adding new code to check everything. Add code in a plugin or functions.php or wherever it can be executed. Code samples taken from here and adapted to this testing report.
  1. Add Common menus code
  2. Check menus created
  3. ✅ Everything looks in place
  4. Add Removing Submenu Main
  5. Check that main menu has been removed
  6. 🐞 Both Main Page and Sub Page now point to subpage. It's impossible to go into Main page.

Expected Results

  • It should be able to point to Main page, from the Main page menu, despite of having removed the submenu.

Actual Results with PPatch

  • 🟠 Issue resolved with patch with some caveats explained in the following notes

Additional Notes

  1. With the proposed hook filter it is possible to solve this problem as shown in last supplemental artifact.
  1. But still, for the second filter some test info/example is required. Furthermore, that filter should be done in the classic way (add a variable, add some docs and pass the variable with the filter to _wp_menu_output instead of just passing the output form filter straight away)
  1. Hook Filter Docs are required

cc @chrisguitarguy

Supplemental Artifacts

Common menus code

add_action('admin_menu', 'wpse206471_add_pages');
function wpse206471_add_pages()
{
    $hook = add_menu_page(
        __('WPSE206471 Main Page', 'wpse206471'),
        __('Main Page', 'wpse206471'),
        'manage_options',
        'wpse206471-main',
        'wpse206471_main_page'
    );
    add_submenu_page(
        'wpse206471-main',
        __('WPSE206471 Sub Page', 'wpse206471'),
        __('Sub Page', 'wpse206471'),
        'manage_options',
        'wpse206471-sub',
        'wpse206471_sub_page'
    );
}

Result:
https://i.imgur.com/bHnaNjK.png

Removing Submenu Main

add_action('admin_menu', 'wpse206471_add_pages');
function wpse206471_add_pages()
{
    $hook = add_menu_page(
        __('WPSE206471 Main Page', 'wpse206471'),
        __('Main Page', 'wpse206471'),
        'manage_options',
        'wpse206471-main',
        'wpse206471_main_page'
    );
    add_submenu_page(
        'wpse206471-main',
        __('WPSE206471 Sub Page', 'wpse206471'),
        __('Sub Page', 'wpse206471'),
        'manage_options',
        'wpse206471-sub',
        'wpse206471_sub_page'
    );
+   remove_submenu_page('wpse206471-main', 'wpse206471-main');
}

Result:
https://i.imgur.com/Fhp8hX9.png

Patch + Filter

add_action('admin_menu', 'wpse206471_add_pages');
function wpse206471_add_pages()
{
    $hook = add_menu_page(
        __('WPSE206471 Main Page', 'wpse206471'),
        __('Main Page', 'wpse206471'),
        'manage_options',
        'wpse206471-main',
        'wpse206471_main_page'
    );
    add_submenu_page(
        'wpse206471-main',
        __('WPSE206471 Sub Page', 'wpse206471'),
        __('Sub Page', 'wpse206471'),
        'manage_options',
        'wpse206471-sub',
        'wpse206471_sub_page'
    );

    // remove the "main" submenue page
    remove_submenu_page('wpse206471-main', 'wpse206471-main');
    // tell `_wp_menu_output` not to use the submenu item as its link
+   add_filter("submenu_as_parent_{$hook}", '__return_false');
}
Last edited 7 hours ago by SirLouen (previous) (diff)
Note: See TracTickets for help on using tickets.