Pass along identical arguments in dt_push_post actions

[barryceelen]

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:

• NetworkSiteConnection do_action( 'dt_push_post', $new_post, $post_id, $args, $this );
• WordPressExternalConnection 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.

[tlovett1]

Mind creating a PR?

[stephanieleary]

It would be really nice if dt_pull_post had the same arguments as well.

[jeffpaul]

Related to #370.

[helen]

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.

[dhanendran]

@jeffpaul I have given a try to unify this in https://github.com/10up/distributor/pull/712

[peterwilsoncc]

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 dt_push_post using the do_action_deprecated() function.

There are two options:

• the same action is used in both locations with the same parameters
• different action names are used and the current parameters are retained

[dhanendran]

@peterwilsoncc I like this approach. We can deprecate the dt_push_post action and introduce 2 new actions for each scenario something like below.

1. dt_push_external_post in includes/classes/ExternalConnections/WordPressExternalConnection.php as this specifically handles the external connection.

/**
* Fires after a post is pushed via Distributor before `restore_current_blog()`.
*
* @hook dt_push_external_post
*
* @param {WP_Post}    $response	The newly created post.
* @param {WP_Post}    $post_id     The original post.
* @param {array}      $args        The arguments passed into wp_insert_post.
* @param {Connection} $this        The Distributor connection being pushed to.
*/
do_action( 'dt_push_external_post', $response, $post, $args, $this );

2. dt_push_network_post in includes/classes/InternalConnections/NetworkSiteConnection.php as this specifically handles the network site connections.

/**
* Fires after a post is pushed via Distributor before `restore_current_blog()`.
*
* @hook dt_push_network_post
*
* @param {WP_Post}    $new_post	The newly created post.
* @param {WP_Post}    $post_id     The original post.
* @param {array}      $args        The arguments passed into wp_insert_post.
* @param {Connection} $this        The Distributor connection being pushed to.
*/
do_action( 'dt_push_network_post', $new_post, $post, $args, $this );

[peterwilsoncc]

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.

[ravinderk]

@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.
} );

cc: @dkotter @jeffpaul

[peterwilsoncc]

@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.

[ravinderk]

@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

[peterwilsoncc]

Yes, I understand that but don't see how we can cover removing actions once they ave been registered.

[dkotter]

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.