Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#33154 closed defect (bug) (fixed)

Issues with Editing Comments where the post ID is zero or post is deleted

Reported by: johnnietb's profile johnnietb Owned by: pento's profile pento
Milestone: 4.3 Priority: normal
Severity: normal Version: 4.2.3
Component: Role/Capability Keywords: fixed-major
Focuses: administration Cc:

Description

Issue after an update - dont know if was the 4.1.3 or later...

Its not possible to edit, delete or change comments where post ID is zero or post has been deleted.
This causes two issues for my clients:

  • One client has a plugin that creates "fishing reports" based on comments, and these are all collected by post ID being zero.
  • Second client has issues because the "actions bar" not to be displayed on the comments where post isn't found - so he cant delete/clean old comments.

So far i figured this out:
You guys added this on line 1147 in wp-includes/capabilities.php:

$caps[] = 'do_not_allow';

Is it possible to add two lines after line 1262:

if ( empty( $post ) )
 break;

Best regards
Johnnie Bertelsen
A keen WP user :)

Attachments (5)

33154.diff (3.1 KB) - added by pento 9 years ago.
33154.2.diff (3.4 KB) - added by pento 9 years ago.
33154.3.diff (10.5 KB) - added by dd32 9 years ago.
33154.4.diff (5.1 KB) - added by ocean90 9 years ago.
33154.5.diff (4.4 KB) - added by pento 9 years ago.

Download all attachments as: .zip

Change History (45)

#1 @pento
9 years ago

  • Milestone changed from Awaiting Review to 4.2.4
  • Owner set to pento
  • Status changed from new to assigned

Thanks for the report, @johnnietb!

I'll need to investigate this a bit more, but that's not the behaviour we want to see. :-)

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


9 years ago

#3 @michaelbeil
9 years ago

Hey @pento,

We have seen some issues with our premium plugin Lasso with this change to capabilities.php as well where it caused users to not be able to save. After pushing a new release, we were able to resolve the issue, but we definitely were not expecting a change like this in a point release.

What was the intention of this change?

https://fossies.org/diffs/wordpress/4.2.2_vs_4.2.3/wp-includes/capabilities.php-diff.html

#4 @pento
9 years ago

The intention of this patch was to prevent editing of posts that don't exist.

Can you describe what the problem was you were running into with Lasso, and how you solved it?

#5 @nphaskins
9 years ago

We were using edit_post without passing a post_id, and previously this worked.

https://core.trac.wordpress.org/browser/tags/4.2.2/src/wp-includes/capabilities.php#L1143

We just changed from edit_post to edit_posts, which is merely a temp patch, but we were able to at least stop the bleeder with that.

#6 @miqrogroove
9 years ago

  • Milestone changed from 4.2.4 to 4.2.5

#7 @dd32
9 years ago

Personally I see it as a bug that current_user_can( 'edit_post', $not_a_post_id ) would return anything other than false. If you want a primitive capability, you'd use edit_posts, calling edit_post without providing a valid post should always be false.

However, failing to be able to edit a comment where the post doesn't exist does seem like a legitimate bug.
It seems like, that edit_comment should use edit_post when the post exists, and failing that, falling back to edit_posts. edit_posts feels incorrect here though, but is the only real choice for backwards compatibility.

Last edited 9 years ago by dd32 (previous) (diff)

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


9 years ago

@pento
9 years ago

#9 @pento
9 years ago

33154.diff fixes this, as well as a warning caused by trying to edit an orphan comment.

Checking edit_post without a post ID is not fixed, I'm not sure that it should be.

#10 @pento
9 years ago

  • Keywords has-patch added

#11 @valendesigns
9 years ago

We should add inline docs to these conditionals explaining the edge case it's solving.

@pento
9 years ago

#12 @pento
9 years ago

Good point, @valendesigns. 33154.2.diff adds some comments, explaining the situation.

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


9 years ago

@dd32
9 years ago

#14 @dd32
9 years ago

33154.3.diff builds off 33154.2.diff and simply removes references to get_post() where it can (replacing with $comment->comment_post_ID) or returns early when it's going to operate with that post.

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


9 years ago

@ocean90
9 years ago

#16 follow-up: @ocean90
9 years ago

With 33154.3.diff the response column is empty for existing posts too. $post = get_post( $comment->comment_post_ID ); in WP_Comments_List_Table::single_row() sets the global $post variable so that get_post() in WP_Comments_List_Table::column_response() returns the correct post object.

33154.4.diff fixes this issue by using get_post( $comment->comment_post_ID ) in WP_Comments_List_Table::column_response().

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


9 years ago

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


9 years ago

#19 in reply to: ↑ 16 ; follow-up: @dd32
9 years ago

Replying to ocean90:

With 33154.3.diff the response column is empty for existing posts too. $post = get_post( $comment->comment_post_ID ); in WP_Comments_List_Table::single_row() sets the global $post variable so that get_post() in WP_Comments_List_Table::column_response() returns the correct post object.

33154.4.diff fixes this issue by using get_post( $comment->comment_post_ID ) in WP_Comments_List_Table::column_response().

Reliance on Globals, Ugh. Good catch, updated patch gets a +1 from me.

#20 in reply to: ↑ 19 @dd32
9 years ago

Replying to dd32:

Replying to ocean90:

With 33154.3.diff the response column is empty for existing posts too. $post = get_post( $comment->comment_post_ID ); in WP_Comments_List_Table::single_row() sets the global $post variable so that get_post() in WP_Comments_List_Table::column_response() returns the correct post object.

33154.4.diff fixes this issue by using get_post( $comment->comment_post_ID ) in WP_Comments_List_Table::column_response().

Reliance on Globals, Ugh. Good catch, updated patch gets a +1 from me.

I take that back, Reliance on Globals = Ugh. If my change broke core, imagine what custom columns could be doing :)
I guess we need to slip $GLOBALS['post'] = get_post( ... ) back in some form, be it through the original patch here, or through
33154.4.diff with some back-compat added in.

#21 @helen
9 years ago

I ran a plugin directory search for column_response( just to double check, even though directly calling that seemed really unlikely. One plugin appears to do it, nextgen-gallery-comments, but they already pass $comment to it. Seems safe.

@pento
9 years ago

#22 @pento
9 years ago

I agree that we can't break the global - it'll probably have terrible side effects.

33154.5.diff is 33154.3.diff with the $post = get_post( $comment->comment_post_ID ); line restored to single_row(). That'll keep the same behaviour for any plugins that rely on it, but it still fixes the warnings caused by orphan comments.

#23 @markjaquith
9 years ago

I like the approach in .5.diff

#24 @ocean90
9 years ago

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

In 33614:

Capabilities: Fall back to the edit_posts capability for orphaned comments.

Also avoid PHP notices because of orphaned comments in the comments list table.
Includes unit test.

props pento, dd32.
fixes #33154.

#25 @ocean90
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.2.5.

#26 @pento
9 years ago

  • Keywords fixed-major added; has-patch removed

#27 @pento
9 years ago

  • Milestone changed from 4.2.5 to 4.4

#28 @pento
9 years ago

  • Milestone changed from 4.4 to 4.3

#29 @pento
9 years ago

In 33972:

Capabilities: Fall back to the edit_posts capability for orphaned comments.

Merge of the capabilities.php part of [33614] to the 4.2 branch.

Props pento, dd32.

See #33154.

#30 follow-up: @pento
9 years ago

In 33973:

Capabilities: Fall back to the edit_posts capability for orphaned comments.

Merge of the capabilities.php part of [33614] to the 4.2 branch.

Props pento, dd32.

See #33154.

#31 @pento
9 years ago

In 33974:

Capabilities: Fall back to the edit_posts capability for orphaned comments.

Merge of the capabilities.php part of [33614] to the 4.0 branch.

Props pento, dd32.

See #33154.

#32 @pento
9 years ago

In 33975:

Capabilities: Fall back to the edit_posts capability for orphaned comments.

Merge of the capabilities.php part of [33614] to the 3.9 branch.

Props pento, dd32.

See #33154.

#33 in reply to: ↑ 30 @knutsp
9 years ago

Replying to pento:

Merge of the capabilities.php part of [33614] to the 4.2 branch.

You mean 4.1 branch, here?

#34 follow-up: @pento
9 years ago

Yes. Yes I did. :-)

#35 @pento
9 years ago

In 33976:

Capabilities: Fall back to the edit_posts capability for orphaned comments.

Merge of the capabilities.php part of [33614] to the 3.8 branch.

Props pento, dd32.

See #33154.

#36 in reply to: ↑ 34 @knutsp
9 years ago

Replying to pento:

Yes. Yes I did. :-)

Don't you forget the 4.3 branch, then :-)

Version 0, edited 9 years ago by knutsp (next)

#37 @pento
9 years ago

[33614] was committed to trunk during the 4.3 cycle: the 4.3 branch and 4.4 both include this fix already. :-)

#38 @pento
9 years ago

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

In 33977:

Capabilities: Fall back to the edit_posts capability for orphaned comments.

Merge of the capabilities.php part of [33614] to the 3.7 branch.

Props pento, dd32.

Fixes #33154.

#39 @miqrogroove
9 years ago

  • Milestone changed from 4.3 to 4.3.1

That just didn't look right... :)

#40 @miqrogroove
9 years ago

  • Milestone changed from 4.3.1 to 4.3

Oh, see comment:37

Note: See TracTickets for help on using tickets.