WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 2 weeks ago

#30991 accepted defect (bug)

Post type object capability 'delete_posts' is referenced in the posts list table but does not exist unless 'map_meta_cap' is set to true for post type

Reported by: bamadesigner Owned by: johnbillion
Milestone: Future Release Priority: normal
Severity: normal Version: 4.1
Component: Posts, Post Types Keywords: has-patch dev-feedback needs-unit-tests needs-dev-note
Focuses: administration Cc:

Description

I'm getting the following error when viewing the main edit screen of a custom post type:

Undefined property: stdClass::$delete_posts at wp-admin/includes/class-wp-posts-list-table.php:209

When I looked up the line, the following code is run:

if ( current_user_can( $post_type_obj->cap->delete_posts ) ) {

The problem is that the capability, 'delete_posts', is only applied to a post type (via get_post_type_capabilities()) if the 'map_meta_cap' argument is set to true when you're registering the post type (via register_post_type()).

Attachments (4)

30991-check_for_optional_variable_set_before_using.diff (629 bytes) - added by jipmoors 5 years ago.
isset on capability option check for optional capability
30991.diff (980 bytes) - added by dipesh.kakadiya 4 years ago.
Add "delete_posts" as default_capabilities in get_post_type_capabilities function
30991.2.diff (1010 bytes) - added by Mte90 18 months ago.
refreshed
30991.3.diff (3.1 KB) - added by johnbillion 11 months ago.

Download all attachments as: .zip

Change History (42)

@jipmoors
5 years ago

isset on capability option check for optional capability

#1 @jipmoors
5 years ago

  • Keywords has-patch added

This is a problem spawned by 20175, since the discussion concluded that the option should not be present when the capability is not set a simple check should solve this.

#2 @jipmoors
5 years ago

I'm sorry, where are my manners..
Thanks Bamadesigner for noticing and posting this bug, I guess it is a minor bug and does not have any impact on functionality or performance, but it's the kind of stuff that really pokes my eyes out when I see it.

#3 @bamadesigner
5 years ago

No worries @jipmoors. :) Thanks!

#4 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 4.1.1

Introduced in [29757].

#5 @dd32
5 years ago

  • Milestone changed from 4.1.1 to 4.2

Although this is new in this location, it's not a new notice, you'll also see this in the Publish metabox for unpublishes CPT's, and probably a few other places.

WordPress assumes that certain capabilities will be set, delete_posts is one of them. I'd suggest that instead of adding isset() checks, we should pre-define these as do_not_allow in get_post_type_capabilities() when not provided by the CPT registration.

I'm shifting to 4.2 based on that, as this doesn't cause any negative issues, and is really a good reminder to developers to assign capabilities when they're not using the builtins.

This ticket was mentioned in Slack in #core by drew. View the logs.


4 years ago

#7 @DrewAPicture
4 years ago

  • Owner set to dd32
  • Status changed from new to reviewing

@dd32: Is this still something we should try to pursue for 4.2, or would be better off getting a patch committed first thing in 4.3 and testing later?

#8 @valendesigns
4 years ago

#31500 was marked as a duplicate.

#9 @DrewAPicture
4 years ago

  • Milestone changed from 4.2 to Future Release
  • Owner dd32 deleted

Actually, let's just take another look at this in 4.3. The right thing to do here is actually fix the underlying problem instead of instituting a stopgap solution.

#10 @dd32
4 years ago

@nacin might be able to give some input here, as #14122 implemented a lot of the logic around this.

As far as I can see, it's a developer error to not set delete_posts in the capabilities array, and leave map_meta_cap as false.

The more I look at it, the more the patch seems correct, even though we shouldn't need to. The other option is that using delete_posts in that context could be seen as wrong to start with.

#11 @SergeyBiryukov
4 years ago

The Codex page says: "There are also seven other primitive capabilities which are not referenced directly in core, except in map_meta_cap()..."

That is no longer true since [29757], which uses delete_posts directly in the posts list table.

#12 @dd32
4 years ago

#32442 was marked as a duplicate.

@dipesh.kakadiya
4 years ago

Add "delete_posts" as default_capabilities in get_post_type_capabilities function

#13 follow-up: @dipesh.kakadiya
4 years ago

  • Keywords dev-feedback added

@SergeyBiryukov @dd32

Added "delete_posts" as default_capabilities in get_post_type_capabilities function

Is this right solution?

Last edited 4 years ago by dipesh.kakadiya (previous) (diff)

#14 @wonderboymusic
4 years ago

  • Owner set to SergeyBiryukov

#15 in reply to: ↑ 13 @johnjamesjacoby
3 years ago

Replying to dipesh.kakadiya:

@SergeyBiryukov @dd32

Added "delete_posts" as default_capabilities in get_post_type_capabilities function

Is this right solution?

As far as I can tell, this is the correct solution.

[29757] set the precedent that the delete_posts capability is assumed to exist in the cap array of a post type.

map_meta_cap also assumes this capability will exist, but the conditions of that assumption are much more strict:

  • Editing...
  • your own trashed post...
  • that was previously published or future dated

So we're less likely to see that debug notice than in get_bulk_actions() where it's prominently displayed.


Also, this bug only manifests itself when registering a post-type using capability_type without capabilities (and map_meta_cap is assumed falsey by default. In this case, you end up with caps that look like:

object(stdClass)[77]
  public 'edit_post' => string 'edit_event' (length=10)
  public 'read_post' => string 'read_event' (length=10)
  public 'delete_post' => string 'delete_event' (length=12)
  public 'edit_posts' => string 'edit_events' (length=11)
  public 'edit_others_posts' => string 'edit_others_events' (length=18)
  public 'publish_posts' => string 'publish_events' (length=14)
  public 'read_private_posts' => string 'read_private_events' (length=19)
  public 'create_posts' => string 'edit_events' (length=11)

But when map_meta_cap is true, we get:

object(stdClass)[77]
  public 'edit_post' => string 'edit_event' (length=10)
  public 'read_post' => string 'read_event' (length=10)
  public 'delete_post' => string 'delete_event' (length=12)
  public 'edit_posts' => string 'edit_events' (length=11)
  public 'edit_others_posts' => string 'edit_others_events' (length=18)
  public 'publish_posts' => string 'publish_events' (length=14)
  public 'read_private_posts' => string 'read_private_events' (length=19)
  public 'read' => string 'read' (length=4)
  public 'delete_posts' => string 'delete_events' (length=13)
  public 'delete_private_posts' => string 'delete_private_events' (length=21)
  public 'delete_published_posts' => string 'delete_published_events' (length=23)
  public 'delete_others_posts' => string 'delete_others_events' (length=20)
  public 'edit_private_posts' => string 'edit_private_events' (length=19)
  public 'edit_published_posts' => string 'edit_published_events' (length=21)
  public 'create_posts' => string 'edit_events' (length=11)

To my eyes, it looks like delete_posts is unintentionally being used as a primitive cap outside of map_meta_cap, in such a way that it is assumed to always exist vs. maybe being mapped. This may not have been the intended behavior, though.


The only other fix I can imagine, is to stop looking for:

$post_type_object->caps->delete_posts

and instead use

current_user_can( "delete_{$post_type_plural}" )

but this won't take into account any literal capability names that might be passed in when registering post types and passing in caps with the capabilities argument (like if I wanted to map delete_posts to maybe_delete_events.)

Last edited 3 years ago by johnjamesjacoby (previous) (diff)

#16 @johnjamesjacoby
3 years ago

I also think we need to demystify the three capability arguments by asking:

  • What do they do now?
  • What is their intended purpose?
  • What do we want them to do?

Experimenting with various combinations of arguments results in unexpected results, that do not directly mirror their respective documentation.

Last edited 3 years ago by johnjamesjacoby (previous) (diff)

This ticket was mentioned in Slack in #core by dipeshkakadiya. View the logs.


3 years ago

#18 @xedin.unknown
3 years ago

Weird, unexplained behaviour observed in plugin.

We've had a CPT definition in the code that has remained almost unchanged for years. This definition did not have neither the map_meta_cap => true, nor capabilities explicitly defined. However, neither us nor the client ever experienced (or at least never reported) the symptoms of this issue. Recently, I changed that definition to include the capabilities index, with one element: create_posts. The symptom naturally presented. Upon changing back (entirely removing capabilities from the array), the symptom persisted. Begs the questions:

  1. Our plugin is used by 40k+ people. The CPT definition is obviously wrong. Why didn't the symptoms ever present?
  2. Why did the symptoms present now?
  3. Why didn't they go away when code changes were reverted?
  4. Is this some sort of caching (I didn't find any in the DB)? If so, why didn't it work with the original CPT definition, but now works with the changed one?
  5. If this is the result of some caching, where did it get a working CPT definition from? The symptoms do not present on a fresh WP install.

#19 @SergeyBiryukov
3 years ago

#39698 was marked as a duplicate.

#20 @Mte90
19 months ago

Just to revamp this ticket, this is the output on my plugin:

[17-Jan-2018 01:40:17 UTC] Array
(
    [error] => Notice
    [file] => /home/domain/wp-admin/includes/class-wp-posts-list-table.php
    [line] => 400
    [context] => Array
        (
            [actions] => Array
                (
                    [edit] => Edit
                )

            [post_type_obj] => WP_Post_Type Object
                (
                    [name] => glossary
                    [label] => Glossary Terms
                    [labels] => stdClass Object
                        (
                            [name] => Glossary Terms
                            [singular_name] => Glossary Term
                            [add_new] => Add New Glossary Term
                            [add_new_item] => Add New Glossary Term
                            [edit_item] => Edit Glossary Term
                            [new_item] => New Glossary Term
                            [view_item] => View Glossary Term
                            [view_items] => View Posts
                            [search_items] => Search Glossary Terms
                            [not_found] => No Glossary Terms
                            [not_found_in_trash] => No Glossary Terms found in Trash
                            [parent_item_colon] => 
                            [all_items] => All Glossary Terms
                            [archives] => All Glossary Terms
                            [attributes] => Post Attributes
                            [insert_into_item] => Insert into post
                            [uploaded_to_this_item] => Uploaded to this post
                            [featured_image] => Featured Image
                            [set_featured_image] => Set featured image
                            [remove_featured_image] => Remove featured image
                            [use_featured_image] => Use as featured image
                            [filter_items_list] => Filter posts list
                            [items_list_navigation] => Posts list navigation
                            [items_list] => Posts list
                            [menu_name] => Glossary Terms
                            [name_admin_bar] => Glossary Term
                        )

                    [description] => 
                    [public] => 1
                    [hierarchical] => 
                    [exclude_from_search] => 
                    [publicly_queryable] => 1
                    [show_ui] => 1
                    [show_in_menu] => 1
                    [show_in_nav_menus] => 1
                    [show_in_admin_bar] => 1
                    [menu_position] => 
                    [menu_icon] => dashicons-book-alt
                    [capability_type] => glossary
                    [map_meta_cap] => 
                    [register_meta_box_cb] => 
                    [taxonomies] => Array
                        (
                            [0] => glossary-cat
                        )

                    [has_archive] => 1
                    [query_var] => glossary
                    [can_export] => 1
                    [delete_with_user] => 
                    [_builtin] => 
                    [_edit_link] => post.php?post=%d
                    [cap] => stdClass Object
                        (
                            [edit_post] => edit_glossary
                            [read_post] => read_glossary
                            [delete_post] => delete_glossary
                            [edit_posts] => edit_glossaries
                            [edit_others_posts] => edit_others_glossaries
                            [publish_posts] => publish_glossaries
                            [read_private_posts] => read_private_glossaries
                            [create_posts] => edit_glossaries
                        )

                    [rewrite] => Array
                        (
                            [slug] => glossary
                            [with_front] => 1
                            [pages] => 1
                            [feeds] => 1
                            [ep_mask] => 1
                        )

                    [show_in_rest] => 1
                    [rest_base] => glossary
                    [rest_controller_class] => WP_REST_Posts_Controller
                    [yarpp_support] => 1
                )

        )

    [url] => https://domain/glossary/wp-admin/edit.php?post_type=glossary
    [filter] => 
    [message] => Notice (8): Undefined property: stdClass::$delete_posts
)

With this parameters this error happen on WordPress 4.9.2.

#21 @Mte90
19 months ago

In my case I am using:

'map_meta_cap' => false,
'capability_type' => array( 'glossary', 'glossaries' ),

#22 @Mte90
19 months ago

Anyone has suggestions how to fix this error by plugin? I mean this error happen with a register post type, maybe passing a specific parameters the errors in the log will disappear.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


18 months ago

@Mte90
18 months ago

refreshed

#24 @Mte90
18 months ago

Patch refreshed and I am looking for a plugin or a code to test it.
Unit tests with this changes are not crashed but with my tests I am not sure about how to replicate the issue pretty well.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


18 months ago

#26 @Mte90
15 months ago

Seems that my own plugin https://wordpress.org/plugins/glossary-by-codeat/ generate this error, someone can test this patch with this patch?

#27 @SergeyBiryukov
12 months ago

#44807 was marked as a duplicate.

#28 @johnbillion
11 months ago

  • Milestone changed from Future Release to 4.9.9
  • Owner changed from SergeyBiryukov to johnbillion

#29 @johnbillion
11 months ago

30991.2.diff is a refreshed patch and includes docs updates.

@flixos90 Would you mind double checking this one?

#30 @flixos90
11 months ago

  • Keywords needs-unit-tests needs-dev-note added

This approach makes sense to me.

I think we still need tests for the change though, and developers should receive an extra heads-up about this, as this might affect folks who interact closely with post type capabilities. It could be part of a general dev-note about capability improvements.

#31 @pento
11 months ago

  • Milestone changed from 4.9.9 to 5.1

#32 @johnbillion
10 months ago

  • Milestone changed from 5.1 to 5.0.1
  • Status changed from reviewing to accepted

#33 @pento
8 months ago

  • Milestone changed from 5.0.1 to 5.0.2

#34 @pento
8 months ago

  • Milestone changed from 5.0.2 to 5.0.3

#35 @audrasjb
8 months ago

  • Milestone changed from 5.0.3 to 5.1

Hello,

5.0.3 is going to be released in a couple of weeks. We are currently sorting the remaining tickets in the milestone.

https://core.trac.wordpress.org/ticket/30991#comment:30

The ticket will needs some testing and a dev note, and 5.0.3 is going to be a "normal" minor with only self-contained patches and regression/bug fixes. Let's address this ticket in 5.1 which is scheduled for February.

#36 @pento
7 months ago

  • Milestone changed from 5.1 to Future Release

This patch still needs unit tests.

#37 @swissspidy
7 months ago

#46181 was marked as a duplicate.

#38 @SergeyBiryukov
2 weeks ago

#47836 was marked as a duplicate.

Note: See TracTickets for help on using tickets.