Make WordPress Core

Opened 8 years ago

Last modified 6 months ago

#36208 new defect (bug)

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

Reported by: clorith's profile Clorith Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.4
Component: Comments Keywords: bulk-reopened has-patch has-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 (6)

36208.patch (2.1 KB) - added by Clorith 8 years ago.
36208.diff (1.8 KB) - added by rachelbaker 8 years ago.
post_type or post_status = any should match WP_Query behavior
34918.diff (794 bytes) - added by chriscct7 7 years ago.
Patch from #34918
36208.4.diff (1.1 KB) - added by donmhico 4 years ago.
36208.5.patch (1.2 KB) - added by techjewel 18 months ago.
Unit Tests for Ticket #36208
36208.6.patch (25.3 KB) - added by imath 18 months ago.
Alternative approach

Download all attachments as: .zip

Change History (37)

@Clorith
8 years ago

#2 @rachelbaker
8 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
8 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
8 years ago

Suggestion from @boonebgorges via Slack:

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

#5 @rachelbaker
8 years ago

  • Milestone changed from Awaiting Review to Future Release

@rachelbaker
8 years ago

post_type or post_status = any should match WP_Query behavior

#6 @rachelbaker
8 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 8 years ago by rachelbaker (previous) (diff)

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


8 years ago

#8 @rachelbaker
8 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
8 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.


7 years ago

#11 @chriscct7
7 years 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 7 years ago by chriscct7 (previous) (diff)

#12 @chriscct7
7 years ago

  • Focuses administration added

#13 @chriscct7
7 years ago

#34918 was marked as a duplicate.

@chriscct7
7 years ago

Patch from #34918

#16 @kal123
5 years ago

  • Keywords bulk-reopened added

See this issue on 5.1 today:

[11-Mar-2019 00:16:54 UTC] PHP Notice: map_meta_cap was called <strong>incorrectly</strong>. The post type tribe-ea-record is not registered, so it may not be reliable to check the capability "read_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 path]/wp-includes/functions.php on line 4667

I have removed the tribe plugins, and still see this message 98 times in the debug log.
Happens every time I load the page. About about 20 times per page load.

First, print it ONCE, do NOT fill up the log, or the entire screen. Rate Limit all your messages.

#17 @kal123
5 years ago

I have 100 entries from 09 26 2018. Why are these messages been printed for old data in the database, even after the plugin removal using wp cli ?
These kind of messages should not be printed by just searching the database. If the plugin in no longer installed, it should stop.

Have a way to acknowledge the message, and disable further log spam.

#19 @jjazzbel
5 years ago

I am having this problem as well. it really seems to be dragging our site down to the point where it is barely responsive. Any suggestions would be welcome.

#20 @SergeyBiryukov
4 years ago

#49266 was marked as a duplicate.

@donmhico
4 years ago

#21 @donmhico
4 years ago

  • Keywords needs-patch removed

36208.4.diff - Cleaned up the patch a little.

#22 @ocean90
4 years ago

#49437 was marked as a duplicate.

#23 @ocean90
4 years ago

#50150 was marked as a duplicate.

This ticket was mentioned in PR #3045 on WordPress/wordpress-develop by techjewel.


21 months ago
#24

  • Keywords has-patch added

Fixing Comment queries should ignore comments associated with non-active custom post types for wp_dashboard_recent_comments function

Trac ticket: https://core.trac.wordpress.org/ticket/36208

#25 @techjewel
21 months ago

I have made a pull request regarding this issue. This PR will solve the PHP notice

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.

Now it will pass the post_type array that is only registered to WordPress. I am open to discussion to resolve this issue as it's annoying notice when we have custom post type registered before.

#26 @helgatheviking
18 months ago

What can we do to finally get this fixed? @techjewel's [PR](https://github.com/WordPress/wordpress-develop/pull/3045) looks promising to me, though it could also be argued that the issue traces back to get_comments() which should perhaps only return comments attached to active post types. However, easy to see how that would have much broader implications than the suggested PR.

#27 @techjewel
18 months ago

Yes, It's really frustrating bug. As a plugin developer I installs lots of plugins and those plugin create different posts (cpts) and once I deactivate any I get PHP warning all the time and my debug log file get messy. If anyone have any better idea, we should implement that or a reviewer can review my pull request and merge.

@techjewel
18 months ago

Unit Tests for Ticket #36208

#28 @techjewel
18 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

Just added Unit Tests for this ticket.

Thanks to @imath for helping about this test.

I have a PR from the last few months here: https://github.com/WordPress/wordpress-develop/pull/3045

Looking forward to seeing this issue resolved.

#29 @desrosj
18 months ago

  • Milestone set to Future Release

Readding a milestone to help surface this better in reports.

#30 @techjewel
18 months ago

From Slack Conversation:

This one will not be considered for 6.1.1, and at the surface, this seems like something that may be best suited for a major release (6.2 being the next one)

#31 @helgatheviking
18 months ago

Not clear to me why a 7+ year old bug needs to wait for a major release... when a patch is present.

@imath
18 months ago

Alternative approach

#32 @imath
18 months ago

Hi @techjewel you're very welcome 👍.

If your PR is the selected fix, I'll be very happy.

I've added an alternative approach a bit similar to the one @chriscct7 shared above.

If removing the "unregistered post type" comments from the comments query is pretty straightforward, imho it's looking like a developer's way of dealing with the problem. It's probably a very efficient way (simple is beautiful) but, even if I may overthink this issue, I'm not feeling comfortable about "hiding" some information/content from the site's administrator because it simply generates doing it wrong notices.

I believe an admin should be able to at least remove these comments or keep them and eventually reactivate the plugin registering the post type.

I hope this will be solved soon 👋

KoolPal commented on PR #3045:


6 months ago
#33

Quick check! How many more years to go before we get this fixed?

Note: See TracTickets for help on using tickets.