Problem/Motivation

Single-step contact/payment/billing is really hard if you need to do anything before Stripe tries to create a payment method and customer.

If, say, you want to identify or create a user matching the order email from a contact information pane on the same step as the payment/billing information pane, that's really difficult, because Stripe has a jquery submit handler on the form, pre-empting other checkout pane submit methods. Stripe::createPaymentMethod() and Stripe::doCreatePaymentMethod() are going to run before any other checkout pane submit method runs.

Proposed resolution

Dispatch a new StripeEvents::PAYMENT_METHOD_CREATE event before a payment method is created.

CommentFileSizeAuthor
#26 commerce_stripe-payment-method-create-event-3191993-26.patch3.84 KBjohnpitcairn
#24 commerce_stripe-payment-method-create-event-3191993-applies-over-3259211-3259349-3171408-3167261-3256552-DO-NOT-TEST.patch3.84 KBjohnpitcairn
#23 commerce_stripe-payment-method-create-event-3191993-23.patch4.37 KBjohnpitcairn
#17 commerce_stripe-payment-method-create-event-3191993-17.patch3.86 KBjohnpitcairn
#11 commerce_stripe-payment-method-create-event-3191993-11.patch4.46 KBjohnpitcairn
#2 commerce_stripe-payment-method-create-event-3191993-2.patch4.33 KBjohnpitcairn

Comments

John Pitcairn created an issue. See original summary.

johnpitcairn’s picture

johnpitcairn commented
Title: Allow customising payment method and customer creation » Add event to allow customising payment method and customer creation
Status: Active » Needs review
StatusFileSize
new commerce_stripe-payment-method-create-event-3191993-2.patch4.33 KB

Setting to needs review for testing, but probably warrants a new test to check that the event is dispatched.

Status: Needs review » Needs work

The last submitted patch, 2: commerce_stripe-payment-method-create-event-3191993-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnpitcairn’s picture

johnpitcairn commented
Assigned: johnpitcairn » Unassigned
jeff veit’s picture

jeff veit commented

Looks like it needs 

use Symfony\Component\EventDispatcher\Event;

and not
use Drupal\Component\EventDispatcher\Event;

I'm going to do some testing to see if I can use this to create a Stripe customer for anonymous users.

johnpitcairn’s picture

johnpitcairn commented

That is possible I think. I found I needed to stash the order id in private tempstore, because the event doesn't get that handed to it.

For anonymous recurring orders, I still needed to create a real (blocked) user for commerce_recurring to do its thing, but it is possible for checkout to remain anonymous.

johnpitcairn’s picture

johnpitcairn commented

The Drupal event is a wrapper to work around backwards compatibility for the deprecated/changed Symfony event constructor method signature.

johnpitcairn’s picture

johnpitcairn commented

Change notice for that: https://www.drupal.org/node/3159012

jeff veit’s picture

jeff veit commented

Just to note then, for future readers that it should be

use Drupal\Component\EventDispatcher\Event;
on D9.1 and later, as in the patch

and

on D8,
use Symfony\Component\EventDispatcher\Event;

So and you may need to patch the patch to fix this unless D8 introduces transitional code with that mapping, or it is added in Contrib,

John - I am successfully using this for anonymous users. It took a while to get right, and I've added the limitation that once you've set the email address, you can't change it for the order, but it turned out that it isn't much code.

I'll clean it up and probably add it as a different, solved, issue, for others to find. I'm reluctant to make it into a module when it's dependent on a patch.

johnpitcairn’s picture

johnpitcairn commented
Assigned: Unassigned » johnpitcairn

The solution for the deprecation will be to use the old dispatcher and argument order until 8.x support is dropped, or until 10.x support is needed.

Re-rolling.

johnpitcairn’s picture

johnpitcairn commented
Status: Needs work » Needs review
StatusFileSize
new commerce_stripe-payment-method-create-event-3191993-11.patch4.46 KB

Updated patch using the deprecated Symfony\Component\EventDispatcher\Event with an appropriate todo.

johnpitcairn’s picture

johnpitcairn commented
Assigned: johnpitcairn » Unassigned

Passes tests, unassigning, please review.

jsacksick’s picture

jsacksick commented 
+++ b/src/Event/PaymentMethodCreateEvent.php
@@ -0,0 +1,93 @@
+ * @todo Extend \Drupal\Component\EventDispatcher\Event when 8.x support ceases.

Commerce introduced a base event class to facilitate the transition. (EventBase, under commerce namespace).

jsacksick’s picture

jsacksick commented
Status: Needs review » Needs work

Note we might not need setPaymentMethod() since objects are always passed by reference, which means getting the payment method and modifying it should be sufficient.

Also, not sure why you'd need to set the payment details, and not just access them?

johnpitcairn’s picture

johnpitcairn commented
Assigned: Unassigned » johnpitcairn

Commerce introduced a base event class to facilitate the transition. (EventBase, under commerce namespace).

Thanks, I'll use that for this and the transactionData event.

Note we might not need setPaymentMethod() since objects are always passed by reference.

Yes, I'll remove the setter.

Also, not sure why you'd need to set the payment details, and not just access them?

Fair enough. If anyone wants it badly enough it's an easy follow-up. I'll remove the setter for that too.

johnpitcairn’s picture

johnpitcairn commented

Commerce introduced a base event class to facilitate the transition. (EventBase, under commerce namespace).

Using this will require a minimum commerce version of 8.x-2.25. That's very recent, and there have been no security releases since 8.x-2.18. I'm inclined to leave this as-is for a while yet. Thoughts?

johnpitcairn’s picture

johnpitcairn commented
Status: Needs work » Needs review
StatusFileSize
new commerce_stripe-payment-method-create-event-3191993-17.patch3.86 KB

This patch removes the setter methods, but does not extend commerce core EventBase, as per #16, pending discussion.

johnpitcairn’s picture

johnpitcairn commented
Status: Needs review » Needs work

OK I see we are now requiring commerce 2.25. I'll re-roll this accordingly.

johnpitcairn’s picture

johnpitcairn commented
Assigned: johnpitcairn » Unassigned
Status: Needs work » Needs review

Extending EventBase handles switching the Event class according to the core/symfony version in use, but it does not handle switching the event dispatch()parameter order. Right? And the commerce issue handling that is postponed until 8.x support is dropped, with only the base class in place.

I don't see any advantage making this change now when it would require parameter-swapping code for both event dispatches, only to remove that anyway when 8.x support is dropped. There are a ton of patches against Stripe.php as it stands, let's not make life harder.

We may as well continue to use the existing deprecated Symfony\Component\EventDispatcher\Event class and parameter order, with todos to change those when commerce core drops support for 8.x.

jsacksick’s picture

jsacksick commented

I don't see any advantage making this change now when it would require parameter-swapping code for both event dispatches, only to remove that anyway when 8.x support is dropped. There are a ton of patches against Stripe.php as it stands, let's not make life harder.

If you extend the base event class now, you won't have to do it later, as the base class knows which event class to extend, so that is one less thing to do, and less deprecation errors, it doesn't hurt to do it, so not sure what you gain by postponing this change as well.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented

setPaymentDetails was removed; but it should be included because $paymentDetails is an array and therefore not passed by reference in getPaymentDetails (unlike getPaymentMethod), so it's good to provide an API for anyone wanting to change payment details so they are not exposed to the inner workings of the event construction.

We should follow maintainer guidance in #20.

johnpitcairn’s picture

johnpitcairn commented

setPaymentDetails was removed; but it should be included

From @jsacsick in #14:

Also, not sure why you'd need to set the payment details, and not just access them?

Which is why the setPaymentDetails method was removed. Clarification or use-case needed?

I will try to attend to #20 soon. I need to look at the implications of #3256552: Allow customising payment intent attributes and metadata and friends first.

johnpitcairn’s picture

johnpitcairn commented
StatusFileSize
new commerce_stripe-payment-method-create-event-3191993-23.patch4.37 KB

Regarding #20: Correct me if I am wrong - the event should extend Commerce EventBase, which handles aliasing the new Drupal Event class for core < 9.1. We should follow the Commerce example and switch our event dispatch to use the new argument order (event first, name second). I'll open a separate issue about dropping 8.9 support.

This patch

  • Extends EventBase and switches the argument order
  • Adds some description of where/how the event is dispatched in the remote transaction setup (very very early)

It does not reintroduce the setPaymentDetails method, pending clarification from @JonathanShaw.

johnpitcairn’s picture

johnpitcairn commented
StatusFileSize
new commerce_stripe-payment-method-create-event-3191993-applies-over-3259211-3259349-3171408-3167261-3256552-DO-NOT-TEST.patch3.84 KB

And in case anyone else needs it, here's one that applies over:

#3259211: Provide more parameters to createPaymentIntent()
#3259349: Allow for payments without stripe review pane
#3171408: Allow off_session payment intents for recurring payments
#3167261: Lost functionality billing information on anonymous checkout
#3256552: Allow customising payment intent attributes and metadata

In that order.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Status: Needs review » Reviewed & tested by the community

Looks right to me.

johnpitcairn’s picture

johnpitcairn commented
StatusFileSize
new commerce_stripe-payment-method-create-event-3191993-26.patch3.84 KB

As of today all those other issues are committed and included in rc10. So the patch at #24 should be correct.

Renamed and re-uploaded, no changes.

jsacksick’s picture

jsacksick commented
Status: Reviewed & tested by the community » Fixed

Committed, thanks!

  • jsacksick committed 394670ce on 8.x-1.x 
    Issue #3191993 followup: Commit the missing event class.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.