Test Results Summary

✍🏽 E2E fails seem unrelated and are being investigated. Internal link: p1664377590875839-slack-C01BWDDTGKX

One question, though: what are your thoughts on adding these hooks to the CPT datastore too? It might help with compatibility.

That's a good question 🤔

It probably would be helpful, since then only one hook is needed, but for them to be truly equivalent I think we would need to hook into the equivalent WordPress actions, such as (illustrative only):

add_action( 
    'delete_post', 
    function( $post_id, $post ) {
        if ( in_array( $post->post_type, wc_get_order_types(), true ) ) {
            do_action( 'woocommerce_before_delete_order', $post_id, $post );
        }
    },
    10,
    2
);

(Because if we modify Abstract_WC_Order_Data_Store_CPT::delete() and trigger these new actions from there, they won't really be equivalent—they would run before pre_delete_post which might mean the order isn't going to be deleted at all.)

I think that would probably work ... but we'd need to make it crystal clear that the hook is fired from both locations. Otherwise, a plugin author might add woocommerce_before_delete_order to make their code work with HPOS orders, without removing existing callbacks hooked into delete_post, which could lead to problems.

Perhaps a way to mitigate this would be to supply a third parameter, which would be the data-store class.

wdyt @jorgeatorres?

Or perhaps we don't add duplicate hooks for the CPT data-store, but rename these two new hooks to something like:

• woocommerce_before_delete_cot_order
• woocommerce_before_trash_cot_order

It probably would be helpful, since then only one hook is needed, but for them to be truly equivalent I think we would need to hook into the equivalent WordPress actions, such as (illustrative only) [...]

This looks ok, but then you'd receive a post instead of an order, unless we load the order again at this point, which I'm not sure we should be doing.

they would run before pre_delete_post which might mean the order isn't going to be deleted at all

Yep, I didn't think of pre_delete_post 😅. Though this sort of disconnection between datastore and the underlying post already occurs: if you call $order->delete( true ); it can still happen that the post isn't deleted (via pre_delete_post) but, regardless, the datastore sets the order ID to 0 and even fires the woocommerce_delete_order hook 🤷‍♂️.

Otherwise, a plugin author might add woocommerce_before_delete_order to make their code work with HPOS orders, without removing existing callbacks hooked into delete_post, which could lead to problems.

I'm thinking this is probably going to be the case, so even though I would've preferred that some canonical actions are always called (instead of depending on the datastore), your current implementation is probably what makes the most sense in the spirit of not breaking existing code. If we receive requests to add the same hook to the posts datastore, we might think about doing something more clever (including not firing both delete_post and the new hook for the same order or something like that).

Given the above, I'm going to merge this as-is, but we can always iterate later. Do let me know if you feel otherwise!

Hi @jorgeatorres, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

• Add the release: add testing instructions label

Thanks: sounds like a plan—as you say, we can always iterate further as adoption of HPOS grows and we get more data 👍