WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 8 years ago

#17109 closed enhancement (fixed)

Add post-success action hooks to edit/create/delete calls.

Reported by: ericmann Owned by: westi
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.4
Component: XML-RPC Keywords: has-patch needs-testing commit
Focuses: Cc:

Description

It was suggested in #14283 that we add call_success hooks for post creation using the Blogger API. The ticket was later expanded to add hooks for all relevant post/page methods.

We should extend this workflow concept and add hooks to all edit/create/delete calls for different data types (comments, media, etc) to make the XML-RPC system more pluggable.

These hooks should use the following format:

do_action( 'xmlrpc_call_success_{method name}', {object id}, $args );

These hooks should be called before returning data to the client, but should not be called before any processing is done or before error messages are created.

Attachments (4)

17109.diff (2.1 KB) - added by ericmann 9 years ago.
17109-2.diff (2.0 KB) - added by ericmann 9 years ago.
Uses filters where appropriate.
17109-3.diff (2.0 KB) - added by ericmann 8 years ago.
Switch new action hooks to filters.
17109-refresh.diff (2.1 KB) - added by ericmann 8 years ago.
Refresh patch to match latest version of trunk.

Download all attachments as: .zip

Change History (16)

@ericmann
9 years ago

#1 @ericmann
9 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

Add action hooks to the following calls:

  • wp_newCategory
  • wp_deleteCategory
  • wp_deleteComment
  • wp_editComment
  • wp_newComment
  • mw_newMediaObject

Post-related hooks are added by #14283.

#2 follow-ups: @josephscott
9 years ago

In order to potentially modify the data before returning it shouldn't we be filtering instead and returning the filter variable?

#3 in reply to: ↑ 2 @ericmann
9 years ago

Replying to josephscott:

In order to potentially modify the data before returning it shouldn't we be filtering instead and returning the filter variable?

That's a good question. But I would say no.

For many of these calls, the only thing returned at this stage is the ID or a boolean representing whether or not the call succeeded. Adding a filter doesn't add any value, but adding an action hook provides the user with a way to further manipulate data or act on the content of a successful call after it's completed. But there's no further output at this stage to be filtered.

#4 in reply to: ↑ 2 @ericmann
9 years ago

Replying to josephscott:

In order to potentially modify the data before returning it shouldn't we be filtering instead and returning the filter variable?

Actually, I've changed my mind. With the methods that actually return data, we should be using filters rather than actions. But not everything returns data. Some only return Boolean values that don't make sense being passed through a filter.

The new patch uses filters where they make sense, but sticks to actions where they don't.

@ericmann
9 years ago

Uses filters where appropriate.

#5 @daniloercoli
8 years ago

  • Cc ercoli@… added

#6 follow-up: @josephscott
8 years ago

Any reason not to make these all filters? Seems strange to allow for filtering the return value of some methods, but not others.

#7 in reply to: ↑ 6 @ericmann
8 years ago

Replying to josephscott:

Any reason not to make these all filters? Seems strange to allow for filtering the return value of some methods, but not others.

It was based on return values. For example, the functions that return data (i.e. IDs) are using filters on the IDs. The functions that return bools (i.e. success/failure) fire an action instead.

My intention was to only use filters when the result of the filter would be returned by the function and use actions elsewhere to still allow devs to hook in to the function.

@ericmann
8 years ago

Switch new action hooks to filters.

#8 follow-up: @markoheijnen
8 years ago

  • Cc marko@… added

To be honest I don't get the patch. Why use filters over actions and not use the data that get returned. I guess that is a mistake.

I think it is better to put in actions like #14283. Changing data that should be returned feels weird to me. Maybe that I can't think of any situation where this would be necessary.

#9 in reply to: ↑ 8 @ericmann
8 years ago

Replying to markoheijnen:

Why use filters over actions and not use the data that get returned. I guess that is a mistake.

If you look at the patch history, originally I was using filters only when data was being returned and actions elsewhere. But @josephscott recommended using filters for all methods for the sake of consistency in the API. Functionally, they serve the same purpose.

@ericmann
8 years ago

Refresh patch to match latest version of trunk.

#10 @ericmann
8 years ago

  • Keywords commit added

Since actions are more appropriate hooks here, all filters have been replaced in the latest patch refresh.

#11 @westi
8 years ago

  • Milestone changed from Awaiting Review to 3.4
  • Owner set to westi
  • Status changed from new to accepted
  • Version set to 3.4

This looks good.

I agree that actions are more appropriate here to allow a plugin to action on the change but not to affect the returned data which is already a success value.

#12 @westi
8 years ago

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

In [20157]:

XMLRPC: Add some xmlrpc_call_success_* actions to some of the credit/edit/delete calls. Fixes #17109 props ericmann.

Note: See TracTickets for help on using tickets.