-
Notifications
You must be signed in to change notification settings - Fork 155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pass along identical arguments in dt_push_post actions #123
Comments
Mind creating a PR? |
It would be really nice if |
Related to #370. |
This is not fixed by #371, punting to 1.6 but we can bring this into a minor if we get a PR open and reviewed earlier. |
It looks like the PR got stalled because of a lack of clarity on how to break backward compatibility. As it's possible a developer is using the existing actions and determining the context by type checking the first parameter, I think it's probably best to deprecate There are two options:
|
@peterwilsoncc I like this approach. We can deprecate the
|
Thanks for the logic check @dhanendran The PR for this issue can remain on hold as the fix is on the 2.0.0 milestone which is a while away. That will give us the chance to fine tune the actions if needs be. |
@peterwilsoncc I like creating new action hooks and deprecating existing ones, mentioned in this comment, but it does not follow the same naming convention. Most users did not raise a concern about it because it is a rare user case when websites push content internally (on the network, multisite) or externally. I also considered why we named action hooks the same for two connect types. I suggest keeping the same name for action hooks and passing one argument as DTO. This way, we will be able to make changes with backward compatibility. /**
* Fires after a post is pushed via Distributor before `restore_current_blog()`.
*
* @hook dt_push_post
* @param {DT_Network_Push_Post_Data|DT_External_Push_Post_Data} $dt_push_post_data The distributor 'dt_push_post' action hook data
*/
do_action( 'dt_push_post', $dt_push_post_data );
// -------- Data object for "dt_push_post" action hook.
class DT_Push_Post_Data{
public string $post_id;
public Connection connection;
// Create an object
public function static makeFromArray(array $data){
$self = new static();
// Assign properties,
return $self;
}
}
class DT_Network_Push_Post_Data extends DT_Push_Post_Data{
public string $remote_post_id;
public array $push_args;
}
class DT_External_Push_Post_Data extends DT_Push_Post_Data{
public $rest_api_query_response = null;
public array function $request_body = null;
public string $type_url;
}
// -------- Handle backward compatibility
add_action( 'dt_push_post', function( DT_Push_Post_Data $dt_push_post_data){
global $wp_filters;
// Extract callback from global param.
// De-register them.
// Handle callables that require multiple parameters to maintain backward compatibility.
}, 0, 1 );
// -------- Register hook with new param.
// 1. if the developer wants to implement the logic for network and external connection push
add_action( 'dt_push_post', function( DT_Push_Post_Data $dt_push_post_data){
// logic.
} );
// 2. if the developer wants to implement the logic for the network
add_action( 'dt_push_post', function( DT_Network_Push_Post_Data|DT_External_Push_Post_Data $dt_push_post_data){
if( ! ( dt_push_post_data instanceof DT_Network_Push_Post_Data ) ) return;
// logic.
} );
// 2. if the developer wants to implement the logic for the network
add_action( 'dt_push_post', function( DT_Network_Push_Post_Data|DT_External_Push_Post_Data $dt_push_post_data){
if( ! ( dt_push_post_data instanceof DT_External_Push_Post_Data ) ) return;
// logic.
} ); |
@ravinderk For // -------- Handle backward compatibility
add_action( 'dt_push_post', function( DT_Push_Post_Data $dt_push_post_data){
global $wp_filters;
// Extract callback from global param.
// De-register them.
// Handle callables that require multiple parameters to maintain backward compatibility.
}, 0, 1 ); I worry that this would cause issues for developers doing something like this: add_action( 'dt_push_post', 'Some::thing' );
// Allow code to run that fires the action the first time
remove_action( 'dt_push_post', 'Some::thing' );
// Further code that fires the action a second time. By deregistering and re-registering with the compatibility shim, I think we'd end up removing the ability for extenders to remove their own hooks. |
@peterwilsoncc I think this will not happen because we can identify legacy hook setups based on their first param type. $function = function (string $abstract, Closure|string|null $concrete): mixed {
// function body
};
$reflection = CallableReflection::fromCallable($function);
var_dump($reflection->isFunction()); // false
var_dump($reflection->isMethod()); // false
var_dump($reflection->isClosure()); // true
[$p1, $p2] = $reflection->getParameters();
var_dump($p1->->getType()); // "DT_Push_Post_Data" for new setup, other for legacy setup |
Yes, I understand that but don't see how we can cover removing actions once they ave been registered. |
Reading through this thread (and discussed some with Ravinder) my preference would be to deprecate the existing hooks and add new hooks that are specific to the context. While I like the idea of trying to find a solution that allows us to clean this up without breaking any backwards compat, I think the easiest and cleanest approach is to deprecate and add new ones. One thing Ravinder brought up in our discussion was if we went the path of renaming the hooks, it might be worth looking at all of the existing custom hooks we have, especially any that deal with connections, and ensure none of those have similar issues. If any are found, ideal to fix all of these at once. |
The
dt_push_post
action can be fired by NetworkSiteConnection and WordPressExternalConnection but they do not pass along identical or the same number of arguments:do_action( 'dt_push_post', $new_post, $post_id, $args, $this );
do_action( 'dt_push_post', $response, $post_body, $type_url, $post_id, $args, $this );
This makes it impractical to reliably use the hook and access the variables.
The text was updated successfully, but these errors were encountered: