WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 20 months ago

Last modified 20 months ago

#31813 closed enhancement (wontfix)

delete_post_{post_type}

Reported by: GunGeekATX Owned by: DrewAPicture
Milestone: Priority: normal
Severity: normal Version:
Component: Posts, Post Types Keywords: has-patch
Focuses: Cc:

Description

Seeing as we have a save_post_{post_type} action, for convenience and consistency, we should also have delete_post_{post_type}.

Since there are before_delete_post and deleted_post actions, the attached patch also contains actions for these with the post_type.

Attachments (2)

delete_post_post_type.patch (1.5 KB) - added by GunGeekATX 2 years ago.
Proposed patch
31813.diff (1.8 KB) - added by DrewAPicture 21 months ago.
Refresh + docs

Download all attachments as: .zip

Change History (14)

@GunGeekATX
2 years ago

Proposed patch

#1 @GunGeekATX
2 years ago

  • Keywords has-patch added

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


21 months ago

#4 @mattheweppelsheimer
21 months ago

+1.

Among other uses, this will be very useful for transient-clearing processes organized by post type.

#5 @wonderboymusic
21 months ago

  • Keywords needs-refresh needs-docs added
  • Milestone changed from Awaiting Review to 4.4
  • Owner set to DrewAPicture
  • Status changed from new to assigned

#6 @DrewAPicture
21 months ago

  • Keywords commit added; needs-refresh needs-docs removed
  • Owner DrewAPicture deleted

31813.diff refreshes the patch after the move to post-functions.php.

I also went ahead and passed the post type value as a second parameter to all three hooks since it's reasonably likely we'd end up adding them later anyway.

@DrewAPicture
21 months ago

Refresh + docs

#7 @DrewAPicture
21 months ago

  • Keywords 2nd-opinion added; commit removed
  • Owner set to wonderboymusic

Actually, hmm. I took this ticket at face value to do the refresh and fix the docs, but it occurs to me on second thought that perhaps instead of essentially adding duplicate hooks here we should instead simply pass the post type value to the existing hooks?

I recognize the precedent in having pairs of hooks (generic and post-type-specific) but I wonder if it's really that useful to add a second hook (plus forever-maintenance) when we could instead add a second parameter to something we're already maintaining.

#8 @mattheweppelsheimer
21 months ago

I believe that it is very useful to have the delete_post{post_type} hook in addition to extending delete_post to pass the post type.

Here's my use case:

My (rather infant) transient-clearing manager Scrubber allows you to register a transient to be cleared when any action hook is fired.

In the places where I'm using this to update cached post data when a post is deleted, I have been just using delete_post. That works — but will clear transients much more often than needed that way. It's not the end of the world, but it would be nice to be more targeted, and limit the scope of the transient clearing.

I've tried to figure out how I could use delete_post differently in this context if it passed the post type, and I don't see a way that I like.

The transient-clearing method `Scrubber::scrub()` does not currently accept any parameters. I could add calls to doing_action() and interpret parameters from there, but that would open up a pandora's box — adding hook-specific logic to a method that might run on any hook — and make it much more complicated and less manageable.

Whereas, a delete_post{post_type} hook will allow that simple targeting, without complicating the method's logic.

I'm wide open to suggestions if there's something I haven't thought of.

#9 @wonderboymusic
20 months ago

  • Owner changed from wonderboymusic to DrewAPicture

#10 @johnbillion
20 months ago

  • Keywords close added; 2nd-opinion removed

While I rarely argue against the addition of hooks that have been proposed to solve a demonstrated problem, I do think that in this case these hooks are superfluous and don't provide enough value to warrant their introduction.

The Scrubber plugin suffers from a fundamental problem whereby it relies on enough context being available in the name of an action hook to clear an associated transient. If, for example, there's a requirement for a transient to be cleared whenever a post in a given category is edited, then Scrubber doesn't have the means to do this because it lacks the ability to inspect any of the contextual parameters passed to a hook.

Adding a delete_post_{$post_type} hook will fix the particular problem of clearing a transient when a post of a given type is deleted, but Scrubber is still left with its lack of contextual control.

The existing delete_post, before_delete_post, and deleted_post hooks are plenty sufficient, and context is available via the $postid parameter passed to each one.

Thanks for the patch and discussion, GunGeekATX and mattheweppelsheimer, but I'm recommending this to be closed as wontfix.

#11 @DrewAPicture
20 months ago

  • Keywords close removed
  • Milestone 4.4 deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

Closing as wontfix. See comment:10.

#12 @mattheweppelsheimer
20 months ago

FWIW you have persuaded me as well.

johnbillion I hadn't seen that flaw in Scrubber, but now it's quite clear thanks to your analysis. It's also something I can address thanks to some tips from Drew in Slack.

For anyone else who stumbles on this:

add_action( ‘delete_post’, function() use ( $id ) {
    $post_type = get_post_type( $id );
    do_action( "delete_post_{$post_type}”, $id );
} );

As drew said, "boom".

Note: See TracTickets for help on using tickets.