WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#17528 closed defect (bug) (fixed)

Comments menu has sub-menu despite only having one item

Reported by: kawauso Owned by: 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 3 years ago.
Drop comment $sub_menu entry
17528.2.diff (857 bytes) - added by ryan 3 years ago.
A generic approach
17528.fix-notices.patch (549 bytes) - added by ocean90 3 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 kawauso3 years ago

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

kawauso3 years ago

Drop comment $sub_menu entry

comment:2 follow-up: greuben3 years ago

  • Keywords close added

See [17864]

comment:3 in reply to: ↑ 2 kawauso3 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.

ryan3 years ago

A generic approach

comment:4 ryan3 years ago

  • Milestone changed from Awaiting Review to 3.2

comment:5 ryan3 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

ocean903 years ago

comment:6 ocean903 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

comment:7 ryan3 years ago

In [18038]:

Fix notices. Props ocean90. fixes #17528

comment:8 kevinB3 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;
}

comment:9 ryan3 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?

comment:10 ryan3 years ago

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

comment:11 kevinB3 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 3 years ago by kevinB (previous) (diff)

comment:12 kevinB3 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).

comment:13 follow-up: azaozz3 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.

comment:14 follow-up: kevinB3 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?

comment:15 in reply to: ↑ 13 ; follow-up: kevinB3 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/1000 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.

Version 0, edited 3 years ago by kevinB (next)

comment:16 in reply to: ↑ 14 azaozz3 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.

comment:17 in reply to: ↑ 15 jane3 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.

comment:18 azaozz3 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].

comment:19 azaozz3 years ago

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

comment:20 kevinB3 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.