WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#12453 closed defect (bug) (fixed)

Custom Post Types break admin menu

Reported by: TobiasBg Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: Posts, Post Types Keywords: has-patch needs-testing featured
Focuses: Cc:

Description

I have a plugin that uses add_pages_page() to add a menu item to the "Pages" menu of the admin menu.

Due to the Custom Post Types, the corresponding parent page was renamed from "edit-pages.php" to "edit.php?posttype=page".

This breaks the URLs of plugin menu entries that are added to the "Pages" menu, as they will look like "edit.php?posttype=page?page=plugin-slug" (note the two ?).

The reason for this is in the lower part of _wp_menu_output() in wp-admin/menu-header.php, where the "?page=" is hard-coded, like
href='{$item[2]}?page={$sub_item[2]}'
instead of using add_query_arg().

There are checks for ? in URLs in that function, but they don't seem to get applied for submenu entries.

Attachments (4)

demo-plugin.php (386 bytes) - added by TobiasBg 6 years ago.
Demo Plugin to show the issue, no magic in there
12453.1.patch (1.9 KB) - added by TobiasBg 6 years ago.
Patch to fix links in admin menu
12453.diff (4.1 KB) - added by ryan 6 years ago.
12453.2.diff (7.8 KB) - added by ryan 6 years ago.

Download all attachments as: .zip

Change History (18)

@TobiasBg6 years ago

Demo Plugin to show the issue, no magic in there

comment:1 @TobiasBg6 years ago

I attached a demo plugin.

Everything works fine on WP 2.9.2, but on WP 3.0 the described issue happens.

@TobiasBg6 years ago

Patch to fix links in admin menu

comment:2 @TobiasBg6 years ago

I attached a patch that fixes the links in the admin menu, by using add_query_arg, however there seems to remain a problem with user rights, as I now get redirected to /wp-admin/?c=1 when clicking /wp-admin/edit.php?post_type=page&page=demo-plugin-slug (with above demo plugin installed)

Details for the patch:

  • Changes strcmp(...) == 0 comparison to plain $string1 == $string2 comparison
  • uses add_query_arg to generate URLs (which remedies the problem of two ? in the URL)
  • fixes the if/ifelse/else logic, as the else part was never called, as either the first ( if $parent_exists ) or the second condition ( if !$parent_exists ) are always fulfilled.
  • $parent_exists is now longer necessary, so I removed it and put the condition into the if-clause

What's left: The links in the menu are looking ok now, but are not accessible (wp_redirect() to /wp-admin/?c=1), most probably coming from somewhere in /wp-admin/menu.php.

comment:3 @TobiasBg6 years ago

To add on:

The redirect to /wp-admin/?c=1 happens on a test site of trunk with Multisite enabled.

On trunk with Singlesite, I get a wp_die: You do not have sufficient permissions to access this page.

comment:4 @TobiasBg6 years ago

I narrowed this down some further:

The above mentioned redirect or wp_die is caused by user_can_access_admin_page() returning false.

It is returning false, because $_registered_pages[$hookname] is not set (here).

$hookname has the value "posts_page_demo-plugin-slug", while the expected hookname is "edit-phppost_typepage_page_demo-plugin-slug".

Both are generated by get_plugin_page_hookname() (here for the isset-check and here for the population in the array).

In get_plugin_page_hookname(), the difference between the two is caused by differing $admin_page_hooks[$parent] (here).

The hookname "edit-phppost_typepage_page_demo-plugin-slug" seems to come from here, while the hookname "posts_page_demo-plugin-slug" seems to come from here.

Any ideas?

comment:5 @TobiasBg6 years ago

  • Summary changed from _wp_menu_output() needs to use add_query_arg() to Custom Post Types break admin menu

I did some more digging:

It looks like the problem is caused by the call of get_admin_page_parent() without an argument here. It will then return "edit.php" instead of "edit.php?post_type=page".
Because of the empty argument, the function attempts to use $pagenow to find the $parent. For the "Pages" menu, that also is edit.php now, due to the Custom Post Types (it used to be edit-pages.php). This correlates to the parent of the "Posts" menu. Because of that, it is assumed that the menu entry belongs to the "Posts" menu, but it was registered for the "Pages" menu.

Some other weirdness (that's not of functional but more optical relevance) comes from this line. The basename() call on "edit.php?post_type=page" basically fails and just returns the input unmodified, so that sanitize_title cleans it to "edit-phppost_typepage". If basename would deliver "edit" as it does with the argument "edit.php", the problem would vanish, however the "Pages" menu would use an incorrect parent.

I changed the title of this ticket, because there's a more severe issue than just the missing usage of add_query_arg().

@ryan6 years ago

comment:6 @ryan6 years ago

That seems to fix parenting and access, but submenu highlight is still broken.

comment:7 @TobiasBg6 years ago

Thanks for the patch.

Access also works fine now.

I investigated on the submenu highlight. Problem is that the check ($item[2] == $self)in this line is false, because $self is == 'edit.php', while $self is still == 'edit.php?post_type=page'.

$self seems to come from here, while $item[2] eventually is defined here, I believe.

This might also be related to the change in menu.php of your patch. I don't know if just removing the querystring is a good idea, as then two menu entries could have the same hookname, although they are in different menus. A submenu under "Posts" has "posts_page_plugin-hook" before and after introduction of post types, while a submenu under "Pages" has "pages_page_plugin-hook" before but "posts_page_plugin-hook" after. Instead of removing the querystring, we should maybe map the page post type to "pages", i.e. with the $compat array here.

@ryan6 years ago

comment:8 @ryan6 years ago

Fixes submenu highlight and implements your suggestions.

comment:9 @TobiasBg6 years ago

  • Keywords has-patch needs-testing featured added; needs-patch removed

Thanks. Your patch works really nice as far as I can see from my tests. The demo plugin works and my actual plugin works, too.
Links, access and highlighting are fine. Your additional work for my suggestion should also make it easier and safer to add a submenu to other main menus created by Custom Post Types in the future.

However, we should probably get some more testing in. I'm changing the tags accordingly and maybe the ticket can be brought up in the dev chat, so that people already working with Custom Post Types are encouraged to test the patch.

comment:10 @ryan6 years ago

(In [13579]) Fix submenus for post types. Props TobiasBg. see #12453

comment:11 @ryan6 years ago

I committed what we have for easier testing. Leaving the ticket open for feedback.

comment:12 @TobiasBg5 years ago

Just noticed another tiny issue:

The $page_title variable of the add_submenu_page() call is not respected for custom post type parents. The <title> tag just has the default WP title.

I guess there is some more logic needed in get_admin_page_title().

comment:13 @scribu5 years ago

RelatedȘ #12985

comment:14 @dd325 years ago

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

The $page_title variable of the add_submenu_page() call is not respected for custom post type parents.

See this commit on #12985:

(In [14126]) Check $pagenow?post_type=$typenow for submenu titles, Fixes page titles for plugin pages added to custom post_type's. Fixes #12985

Closing this as fixed due to no other issues popping up.

Note: See TracTickets for help on using tickets.