WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#13731 closed defect (bug) (fixed)

delete_post action called twice in wp_delete_post

Reported by: mdawaffe Owned by: westi
Milestone: 3.2 Priority: normal
Severity: normal Version: 2.9.2
Component: General Keywords: has-patch needs-testing commit
Focuses: Cc:

Description

wp_delete_post() calls the delete_post action twice.

http://core.trac.wordpress.org/browser/trunk/wp-includes/post.php?rev=15056#L1727
http://core.trac.wordpress.org/browser/trunk/wp-includes/post.php?rev=15056#L1782

Introduced in [11968]/#10750

I see that wp_delete_attachment() does delete_attachment, delete_post, deleted_post.

So wp_delete_post is following the same pattern, except that it ends up being: delete_post, delete_post, deleted_post.

Marking as 3.0, but it's technically an API change, so may have to bump.

Attachments (5)

remove_extra_delete_post_action.diff (447 bytes) - added by eddieringle 5 years ago.
remove_extra_delete_post_action2.diff (425 bytes) - added by eddieringle 5 years ago.
13731.inner.diff (493 bytes) - added by scribu 4 years ago.
13731.outer.diff (603 bytes) - added by scribu 4 years ago.
13731.diff (686 bytes) - added by scribu 4 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 @mdawaffe5 years ago

  • Version set to 3.0

comment:2 @eddieringle5 years ago

  • Cc eddie@… added
  • Keywords has-patch added
  • Owner set to eddieringle
  • Status changed from new to accepted

comment:3 follow-up: @nacin5 years ago

  • Owner eddieringle deleted
  • Status changed from accepted to assigned

I would tend to think the other action should be removed, the one immediately before the query.

Mike,
This was added in 2.9, but something tells me it would make a lot of sense to address this now and fix 2.9 in the process, versus waiting. This is a 2.8->2.9 regression if I had to classify it. (And running this filter twice actually has been double-logging something on one of my sites and I just never realized it.)

comment:4 @eddieringle5 years ago

There's my second patch with the other removed instead of the first.

comment:5 @filosofo5 years ago

Replying to nacin:

I would tend to think the other action should be removed, the one immediately before the query.

Agreed, because we should be able to intercept the associated postmetas before they're deleted.

comment:6 in reply to: ↑ 3 @mdawaffe5 years ago

Replying to nacin:

And running this filter twice actually has been double-logging something on one of my sites and I just never realized it.

That's how I found it.

I think anyone relying on this double call went into it asking for trouble.

That said, I'm not convinced there's an easy fix this late in the 3.0 process.

If we get rid of the second call in wp_delete_post(), that leaves an inconsistent call to delete_post in wp_delete_attachment.

If we delete the first call in wp_delete_post(), we lose the ability to look at comments, postmeta, children, etc.

Either way, that's a significant API change. Maybe the most robust fix would be to just rename the first call: pre_delete_post ore something. delete_post would then act the same in both wp_delete_post() and wp_delete_attachment().

comment:7 @westi5 years ago

  • Keywords needs-patch early added; has-patch removed
  • Milestone changed from 3.0 to 3.1
  • Owner set to westi
  • Version changed from 3.0 to 2.9.2

We also have two calls to deleted_post in there as well.

I don't think we should rush fix this for 3.0 as it has been out in the wild for a long while and this is the first report of an issue.

We need to review this more carefully and fix for 3.1 (we can then backport as appropriate to 3.0.x and 2.9.x)

Also the delete_comment and deleted_comment actions in there look bogus as they get passed an array of ids and then in wp_delete_comment we call the actions again but with each individual id

comment:8 @nacin5 years ago

  • Keywords early removed
  • Milestone changed from Awaiting Triage to 3.1

comment:9 @layotte4 years ago

  • Cc lew@… added

comment:10 @greuben4 years ago

Maybe we can rename the first action to "delete_post_before" or "before_delete_post" and make "delete_comment" and "deleted_comment" plurals( since they are array of ID's ).

comment:11 @ryan4 years ago

  • Milestone changed from 3.1 to Future Release

comment:12 @dnxpert4 years ago

Has there been any progress on this? I would like to rely on delete_post to update some meta data of a parent post before deleting child post but I can't because of the double issue.

comment:13 @solarissmoke4 years ago

  • Cc solaris.smoke@… added

comment:14 @scribu4 years ago

Same problem as dnxpert.

@scribu4 years ago

@scribu4 years ago

@scribu4 years ago

comment:15 @scribu4 years ago

  • Keywords has-patch 3.2-early added; needs-patch removed

13731.inner.diff removes the inner actions.

13731.outer.diff removes the outer actions.

13731.diff renames the outer actions. I would go with this one.

comment:16 @dd324 years ago

13731.diff renames the outer actions. I would go with this one.

If we were to go with that, i'd suggest pre_delete_post and deleted_post as the action names myself.

comment:17 @scribu4 years ago

The idea is that 'deleted_post' is already in use and 'post_delete_post' doesn't sound right.

comment:18 @westi4 years ago

  • Keywords needs-testing added; 3.2-early removed
  • Milestone changed from Future Release to 3.2

comment:19 @mau4 years ago

  • Cc mau added

comment:20 @nacin4 years ago

  • Keywords commit added

I agree with 13731.diff as it keeps consistency with many other inner actions of other queries.

comment:21 @ryan4 years ago

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

In [18012]:

Rename duplicate delete_post and deleted_post actions to before_delete_post and after_delete_post. Props scribu. fixes #13731

comment:22 @Denis-de-Bernardy4 years ago

Out of curiosity, after r18012 (which is a scary looking changeset due to plugin backwards compat, so some kind of notice on wpdevel might be in order)... does delete_post end up being called before or after the post is deleted?

Note: See TracTickets for help on using tickets.