Skip to:
Content

BuddyPress.org

Opened 3 years ago

Closed 3 years ago

#7409 closed enhancement (fixed)

Action request: cover image deleted

Reported by: jdgrimes Owned by: boonebgorges
Milestone: 2.8 Priority: normal
Severity: normal Version: 2.5.0
Component: Core Keywords: good-first-bug has-patch
Cc:

Description

There is an action ("{$object_data['component']}_cover_image_uploaded") that fires in bp_attachments_cover_image_ajax_upload(), and there should be a complementary one in bp_attachments_cover_image_ajax_delete(). Otherwise it is possible to do something when a cover image is uploaded, but not to reverse any of the effects when the cover image is deleted.

Attachments (2)

7409.diff (1.1 KB) - added by jdgrimes 3 years ago.
Adds the deleted action
7409-2.diff (1.1 KB) - added by jdgrimes 3 years ago.
Correct the copy-pasted action arg description

Download all attachments as: .zip

Change History (8)

#1 @johnjamesjacoby
3 years ago

  • Keywords good-first-bug needs-patch added
  • Milestone changed from Awaiting Review to 2.8
  • Version set to 2.5.0

@jdgrimes
3 years ago

Adds the deleted action

@jdgrimes
3 years ago

Correct the copy-pasted action arg description

#2 @jdgrimes
3 years ago

  • Keywords has-patch added; needs-patch removed

I've uploaded 7409-2.diff as a first pass.

#3 @slaFFik
3 years ago

  • Keywords 2nd-opinion added

As an idea.
What about the difference in approaches:

do_action( "{$component}_cover_image_deleted", (int) $cover_image_data['item_id'] );
vs
do_action( "bp_component_cover_image_deleted", (int) $cover_image_data['item_id'], $component );

Personally I would go with the second one, as it will allow much easier targeting in a hook of all components that have cover image deleted.
Currently in a patch by @jdgrimes we need to get the list of components, and among them those that are using cover images to be able to target ANY covers deletion. That's a lot of extra work where it's easy to make a mistake.

#4 follow-up: @boonebgorges
3 years ago

  • Keywords 2nd-opinion removed

I agree with @slaFFik that we should generally avoid dynamic action/filter names, as they're harder to read and harder to document. The one time they're really advantageous is when a hook might be fired very, very frequently; in this case, it's better for performance to avoid invoking extra callbacks. The case of deleting a cover image isn't one of these instances.

That being said, if we already have a dynamic hook name for updating, we should probably do the same thing for deletion, just for the sake of consistency.

#5 in reply to: ↑ 4 @jdgrimes
3 years ago

Replying to boonebgorges:

That being said, if we already have a dynamic hook name for updating, we should probably do the same thing for deletion, just for the sake of consistency.

Yes, I was just following the pattern of the "{$object_data['component']}_cover_image_uploaded" action.

#6 @boonebgorges
3 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 11357:

Fire an action when a cover image is deleted.

The new {$component}_cover_image_deleted action provides
parity with the existing {$component}_cover_image_uploaded
action.

Props jdgrimes.
Fixes #7409.

Note: See TracTickets for help on using tickets.