WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 18 months ago

#36208 new defect (bug)

Comment queries should ignore comments associated with non-active custom post types

Reported by: Clorith Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.4
Component: Comments Keywords: needs-patch needs-unit-tests
Focuses: administration Cc:

Description (last modified by chriscct7)

As of 4.4 we introduced the _doing_it_wrong() (r34091) when checking meta capabilities on custom post types that aren't registered.

This also spread and affected comments, primarily on the dashboard where we show comments and try to add links for these and check the users capability against them, they will produce this wonderful output:

PHP Notice: map_meta_cap was called <strong>incorrectly</strong>. The post type shop_order is not registered, so it may not be reliable to check the capability "edit_post" against a post of that type. Please see <a href="https://codex.wordpress.org/Debugging_in_WordPress">Debugging in WordPress</a> for more information. (This message was added in version 4.4.0.) in /wordpress/wp-includes/functions.php on line 3827

Ideally, WP_Comment_Query should bypass comments associated to non-existing post types as well.

In the attached patch I've introduced both post_type__in and post_type__not_in which accepts an array of post type string names.

I've also added the default value for post_type__in to get_post_types(), as by default you'd never want to query for non-existing data any way, but this allows the query to be overwritten via filters, or directly with a new construct for those who have the need.

I am wondering if the use of post_type = 'any' would need to short-circuit the new arguments to avoid breaking BC (current tests all pass with the patch applied though) ?

Related #16956

What this looks like (from #34918):

http://crosseyedeveloper.com/wp-content/uploads/2015/12/commentcap.jpg

It also shows up on the Dashboard in the comments widget: http://crosseyedeveloper.com/wp-content/uploads/2015/12/commentcap21.jpg

Attachments (3)

36208.patch (2.1 KB) - added by Clorith 2 years ago.
36208.diff (1.8 KB) - added by rachelbaker 2 years ago.
post_type or post_status = any should match WP_Query behavior
34918.diff (794 bytes) - added by chriscct7 18 months ago.
Patch from #34918

Download all attachments as: .zip

Change History (16)

@Clorith
2 years ago

#2 @rachelbaker
2 years ago

I wonder if a more straight-forward approach would be to add support for post_type => active? Downside would be there would be a conflict with any post_types named active.

#3 @Clorith
2 years ago

I wanted to avoid any potential collision from the use of keywords, and instead use a format that is more in line with, and subconsciously expected, across the other query classes.

#4 @rachelbaker
2 years ago

Suggestion from @boonebgorges via Slack:

Or distinguish between all (whitelist) and any (ignored)

#5 @rachelbaker
2 years ago

  • Milestone changed from Awaiting Review to Future Release

@rachelbaker
2 years ago

post_type or post_status = any should match WP_Query behavior

#6 @rachelbaker
2 years ago

In 36208.diff I am matching the behavior of the any value for the post_type and post_status arguments to the WP_Query class, which only includes valid types/statuses that are not excluded from search. See: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/query.php#L3053

Needs more unit tests and testing.

Last edited 2 years ago by rachelbaker (previous) (diff)

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


2 years ago

#8 @rachelbaker
2 years ago

  • Keywords needs-patch added; has-patch removed

This issue isn't as straightforward as I first thought. My approach in 36208.diff won't work as written.

Comments don't have to be related to posts. Defaulting to only valid post_types or valid post_statuses would actually exclude comments with a comment_post_ID of 0 which we don't want to do.

We need to somehow limit hiding comments from related posts if the comment_post_ID > 0 AND the post_type of the related post is registered.

#9 @Clorith
2 years ago

What if we explicitly look for the any keyword to trigger the addition of 36208.diff, and keep the empty check as it currently is with just a simple continue (so essentially spit up the if or into separate if statements on line 786).

That would avoid us breaking any existing queries that use the comment_post_ID as 0, yet give the enhancements we'd like, or am I missing something deeper perhaps?

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


18 months ago

#11 @chriscct7
18 months ago

  • Description modified (diff)

In #34918, a exact duplicate, there was discussion that followed a different approach than the one on this ticket. Therefore, while closing #34918 as duplicate of this ticket, I've updated this ticket's description with some information the other had, and will quickly summarize the patches + discussions of the other ticket, as it took a different approach to solving the issue.

It was pointed out that hiding the comments of post types that don't exist/inactive would make them inpossible to edit or remove. It was then noted that while this was likely by design, WordPress does not show posts from unregistered post types, and thus this served as an argument to hide comments that are orphaned.

The ticket consensus then agreed that it made sense to hide posts if the post type they are connected to is "gone", particularly from a consistency standpoint.

Component maintainer, @boonebgorges agreed to this approach (https://core.trac.wordpress.org/ticket/34918#comment:10), and then a patch was produced (attaching it below) that hides the errors in the comments list table by checking to see if the post type object exists. This patch however, did not address the implications of a plugin using a wp comment query or the dashboard widget.

Notifications from old ticket: @Funkatronic, @jdgrimes, @lukecavanagh, @nekojira, @obrienlabs, @wesm87

Last edited 18 months ago by chriscct7 (previous) (diff)

#12 @chriscct7
18 months ago

  • Focuses administration added

#13 @chriscct7
18 months ago

#34918 was marked as a duplicate.

@chriscct7
18 months ago

Patch from #34918

Note: See TracTickets for help on using tickets.