Make WordPress Core

Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#17528 closed defect (bug) (fixed)

Comments menu has sub-menu despite only having one item

Reported by: kawauso's profile kawauso Owned by: ryan's profile ryan
Milestone: 3.2 Priority: normal
Severity: normal Version: 3.2
Component: Administration Keywords: reporter-feedback
Focuses: Cc:

Description

Under 3.1.2 menus with only one sub-menu item (their own entry) don't display it, but under 3.2 the Comments menu always displays the "All Comments" sub-menu item. The Plugins menu (in the site admin on a network) does not do the same when the same scenario occurs.

Attachments (3)

17528.diff (826 bytes) - added by kawauso 13 years ago.
Drop comment $sub_menu entry
17528.2.diff (857 bytes) - added by ryan 13 years ago.
A generic approach
17528.fix-notices.patch (549 bytes) - added by ocean90 13 years ago.

Download all attachments as: .zip

Change History (23)

#1 @kawauso
13 years ago

  • Summary changed from Comments has sub-menu despite having one item to Comments menu has sub-menu despite only having one item

@kawauso
13 years ago

Drop comment $sub_menu entry

#2 follow-up: @greuben
13 years ago

  • Keywords close added

See [17864]

#3 in reply to: ↑ 2 @kawauso
13 years ago

Replying to greuben:

See [17864]

My issue isn't with the sub-menu naming, but rather the UI implications of it being present for one item only. There is a related issue with the inheritance of top menu names for auto-generated sub-menus clashing though.

@ryan
13 years ago

A generic approach

#4 @ryan
13 years ago

  • Milestone changed from Awaiting Review to 3.2

#5 @ryan
13 years ago

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

In [18034]:

If there is only one submenu and it is has same destination as the parent, remove the submenu. fixes #17528

#6 @ocean90
13 years ago

17528.fix-notices.patch for

NOTICE: wp-admin/includes/menu.php:107 - Undefined index: separator1
NOTICE: wp-admin/includes/menu.php:107 - Undefined index: separator2
NOTICE: wp-admin/includes/menu.php:107 - Undefined index: separator-last

#7 @ryan
13 years ago

In [18038]:

Fix notices. Props ocean90. fixes #17528

#8 @kevinB
13 years ago

  • Cc kevin@… added
  • Keywords needs-patch added; close removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

The current patch (as included in 3.2 RC1) breaks permissions handling under some configurations.

A plugin or theme may remove the post-new.php submenu if user capabilities are filtered to allow editing but not creation. If so, user_can_access_admin_page() now always returns false for edit.php even if a current_user_can( 'edit_posts' ) cap check would pass.

After the remaining "All Posts" item is removed from $submenu, get_admin_page_parent() no longer finds a match for $pagenow and $typenow... and returns nullstring.

In my use case, this can be remedied by uncommenting this block in get_admin_page_parent() :

if ( !empty ( $parent_file ) ) {
   if ( isset( $_wp_real_parent_file[$parent_file] ) )
	$parent_file = $_wp_real_parent_file[$parent_file];

   return $parent_file;
}

#9 @ryan
13 years ago

That block was added 3 years ago in [8734] and promptly commented out in [8991]. I wouldn't trust it to not have nasty side-effects.

I'm trying to understand what's going on here. Can you provide example code of what the plugin is doing?

#10 @ryan
13 years ago

  • Keywords reporter-feedback added; has-patch needs-patch removed

#11 @kevinB
13 years ago

Here's a paraphrase of what said plugin does if the logged user has been granted editing capabilities for certain posts, but not post creation capability:

// Remove "Add New" submenu items if user can only edit existing posts.
// This causes permissions failure if executed on the 'admin_menu' action.
foreach ( get_post_types() as $type ) {
   if ( ! plugin_func_user_can_add_posts( $type ) ) {
      $edit_key = 'edit.php';
      $add_key = 'post-new.php';

      if ( 'post' != $type ) {
         $edit_key .= "?post_type=$type";
         $add_key .= "?post_type=$type";
      }

      remove_submenu_page( $edit_key, $add_key );
   }
}

I remember some discussion of core support for a "create_posts" capability, so this may not be just a plugin issue.

Last edited 13 years ago by kevinB (previous) (diff)

#12 @kevinB
13 years ago

The other condition to recreate this symptom is no other submenu items in the Posts / Pages menu (i.e. user cannot manage categories or tags either).

#13 follow-up: @azaozz
13 years ago

This seems like a 1 / 1,000,000,000 case. If I understand it correctly you're setting a user with a permission to edit all posts (Editor) but removing permission to edit tags, cats, or publish new posts. And your problem is how to hide the Add New (post) submenu.

If user_can_access_admin_page() breaks with these "unusual" permissions, perhaps we should be looking to fix this function instead of tweaking the menus.

#14 follow-up: @kevinB
13 years ago

Note the correction to my code comments. The permission failure I described occurred when I removed "Add New" submenu items on the 'admin_menu' action. My current workaround is to instead remove them on the 'admin_init' action... which prevents the removal of "All Pages" and the subsequent user_can_access_admin_page() failure.

So you could call this resolved. Still, supporting this dependance between menu UI and permissions does not feel right. If this ticket was motivated by a cosmetic concern, why are we talking about permissions at all?

#15 in reply to: ↑ 13 ; follow-up: @kevinB
13 years ago

Replying to azaozz:

This seems like a 1 / 1,000,000,000 case. If I understand it correctly you're setting a user with a permission to edit all posts (Editor) but removing permission to edit tags, cats, or publish new posts. And your problem is how to hide the Add New (post) submenu.

Not exactly. The typical use case is editing access to some subset of posts, without the ability to create new posts. And yes, those users would not normally be allowed to edit terms either.

The use case is certainly not WP core but not that bizarre. Based on WP and plugin usage I would say 1/1,000 is a fairer characterization. And there's been some talk of a separate create_posts capability in WP core.

If user_can_access_admin_page() breaks with these "unusual" permissions, perhaps we should be looking to fix this function instead of tweaking the menus.

Agreed. Given that remove_submenu_page() is supported API, you should be able to remove one submenu without breaking access to another.

Last edited 13 years ago by kevinB (previous) (diff)

#16 in reply to: ↑ 14 @azaozz
13 years ago

  • Milestone 3.2 deleted
  • Resolution set to worksforme
  • Status changed from reopened to closed

Replying to kevinB:

So you could call this resolved...

Indeed this ticket is mostly about a UI problem. I am aware that the menus code needs streamlining but thinking we can attempt this after any changes to the permission system/API.

...The use case is certainly not WP core but not that bizarre...

Perhaps. The problematic thing here is that even low-end users (Contributors) have access to creating new posts but not to publishing them. So creating an Editor user (high-level) without the ability to add new posts is unusual from the current permission system's point of view.

#17 in reply to: ↑ 15 @jane
13 years ago

Replying to kevinB:

The use case is certainly not WP core but not that bizarre. Based on WP and plugin usage I would say 1/1,000 is a fairer characterization.

45+ million users of WordPress. I am hesitant to thin 1/1000 is a realistic number.

#18 @azaozz
13 years ago

  • Milestone set to 3.2
  • Resolution worksforme deleted
  • Status changed from closed to reopened

Actually the proper resolution for this ticket is "fixed" with [18034].

#19 @azaozz
13 years ago

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

#20 @kevinB
13 years ago

Replying to jane:

Replying to kevinB:

The use case is certainly not WP core but not that bizarre. Based on WP and plugin usage I would say 1/1,000 is a fairer characterization.

45+ million users of WordPress. I am hesitant to thin 1/1000 is a realistic number.

As stated above, I can live with this ticket being re-closed because I have a workaround.

But since Jane brought up the topic of relevance, I will relate my thoughts on this interaction as a case in point. As with my last several trac interactions, I went in with some experience-based observations or suggestions regarding plugin API... and was very quickly sent away with the distinct message that my ideas/time/self are irrelevant in the face of the great WP juggernaut. Surely the core contributors have an overall mastery of WP far beyond me, but for the specific issue at hand they have often seemed unable or unwilling to consider the merits of even a slight shift in API behavior. I suspect that's a combination of their busyness, the increased stakes as usage continues to increase, and my low status on the "meritocracy" chain.

This is not intended as a personal gripe or self-promotion. I like and respect WP and its community. But with the astronomical growth you run the risk of being increasingly unapproachable, even to "outsiders" who are immersed in the code and API. Based on the wordpress.org/extend download and version stats, my permissions plugin is on around 45 thousand sites. It has been my primary occupational focus for the last 3.5 years, and the irrelevance you so quickly disregard is the center of my user base. If 1/100 is the standard, what percentage of plugins would warrant the slightest change to WP core?

Time is precious to all of us, and I don't want to waste mine or yours.

Note: See TracTickets for help on using tickets.