Automattic / woocommerce-subscriptions-core

Subscriptions core package for WooCommerce
Other
80 stars 29 forks source link

Filter whether to recreate the inital payment order’s cart. #594

Closed peterwilsoncc closed 2 months ago

peterwilsoncc commented 2 months ago

Fixes #591

Description

This adds a filter to WCS_Cart_Initial_Payment::maybe_setup_cart() to allow other extensions to filter whether the initial cart is recreated during the checkout flow.

I've done this for the pre-orders project to introduce support for subscription products. Feel free to request an alternative approach if this filter isn't required.

Risks: The main risk I see is extensions naively implementing the filter and conflicting with Subs/Subs-Core expectations. For example add_filter( 'wcs_recreate_initial_payment_order', '__return_false' )

At present i haven't got anything to protect against that as once someone is writing PHP it's reasonable to expect them to know what they are doing. If the Subs dev approach is different, let me know.

How to test this PR

I'm unsure how to test this without using the pre-orders subs compatibility branch to demonstrate the issue.

  1. Checkout woocommerce-pre-orders#497
  2. Activate WooPayments as a payment method
  3. Activate Subs
  4. Create a Far Future pre-order:
    • Type: Simple Subscription (other details as you wish)
    • Pre-orders: enabled
    • Pre-order matures a month or more from now
    • Pre-order paid upon release
  5. Open a private/incognito browser window and purchase the product.
    • Ensure you use a real email address or mail capture tool such as mailhog
    • During checkout you will be presented with the "pay later" gateway
    • WooPayments will not be available
  6. Back in the admin, go to: WooCommerce > Pre-orders > Actions (tab) > Complete (section)
  7. From the dropdown, select your demo product
  8. Fill out the message field
  9. Submit the form
  10. Check the email of the test user
  11. Click the email ending "your pre-order from Date is now available"
  12. Click the link to pay for the pre-order
  13. On Subs-Core trunk: a. The payment page will be hijacked by Subs and the order recreated b. Rather than being able to pay for the order, the only gateway available will be pay later
  14. On this branch: payment will proceed via the pay-for-order screen

Product impact

james-allan commented 2 months ago

At present i haven't got anything to protect against that as once someone is writing PHP it's reasonable to expect them to know what they are doing. If the Subs dev approach is different, let me know.

I'd probably say we're a little more protective over adding action hooks that have broad consequences. This is mainly because it makes supporting the plugin from a customer support and maintenance POV a little harder as the interface layers are far too large to fully understand and implications further down the chain aren't fully understood. ie I need to know what functionality might break with this filter and Woo Subscriptions is so complex that I can't reasonably do that.

Having said all that, after reading the PR description, I'm not sure of a better approach for that scenario. Subscriptions is a little too heavy handed in hi-jacking the pay-for-order request when it contains a subscription related order and the only way to get around that would be to unhook our function(s).

I think it would be better in our case to just allow this sort of filter so we can have some control around this flow, rather than encourage a wild west approach of hooking other plugins functionality etc.

I will suggest another name though that's a little more targeted. The order being loaded into the cart and the payment processed via the checkout isn't really linked to creating new orders. For instance, we don't recreate renewal orders but they also use this cart set up approach.

So perhaps a name like:

'wcs_setup_cart_for_' . $this->cart_item_key'wcs_setup_cart_for_subscription_initial_payment'

That would give us an established format if we rolled this out to other subscription cart types: like 'wcs_setup_cart_for_subscription_renewal' etc

peterwilsoncc commented 2 months ago

Thanks James, I've rebased against trunk and pushed a rename of the filter in fe0dfe00a6fe3942e1963f0487ae97f2e205fc26.

That would give us an established format if we rolled this out to other subscription cart types:

Makes sense, I see the similar method in the other cart handlers now. I haven't added it anywhere else due to the bold if.

peterwilsoncc commented 2 months ago

@james-allan I've added the version annotation in 54ce60c21f840e5992d3825693dfead5b4908986. I'm unable to merge as I'm not a member of the Automattic organization so I'll leave you to do the honors.