Allow customising payment intent attributes and metadata

Why is it necessary add this additional event to customise the intent when it is created?

#3170903: Allow customising the payment intent allows us to customise the intent.

If it was important to customise the intent before it was confirmed, we could modify #3170903: Allow customising the payment intent to do that, by still using the same event but calling it before confirmation.

That makes sense. This would seem to heavily overlap with #3170903: Allow customising the payment intent and I'm not sure we need both.

I think we should close #3170903: Allow customising the payment intent as a duplicate, and then in this issue deprecate the existing transaction data event.

We should also have some somewhat-related follow-up issues:
- to allow OrderPaymentIntentSubscriber to update the intent properly when the order changes.
- to trigger OrderPaymentIntentSubscriber if the payment changes, as that could also affect the intent.

That way we get:
- a single recommended solution for customising the intent, providing metadata, etc
- no BC issues that might just possibly come from changing the point at which the event info is added to the intent
- we don't have to update the intent so an API call is saved
- a clean new event API for customising the event, rather than carrying forward the legacy TransactionData event

I'm happy to review and RTBC if you'll do the fixing @Krzysztof. Or I can fix and you can review.

1. +++ b/src/Event/PaymentIntentCreateEvent.php
   @@ -0,0 +1,72 @@
   +class PaymentIntentCreateEvent extends Event {
   

   We will also need to respond to situations where the intent needs to be updated. So let's call this simply a 'PaymentIntentEvent'.

2. +++ b/src/Event/PaymentIntentCreateEvent.php
   @@ -0,0 +1,72 @@
   +  protected $transactionData = [];
   

   I would call this $intentAttributes, using Stripe's terminology.

3. +++ b/src/Event/PaymentIntentCreateEvent.php
   @@ -0,0 +1,72 @@
   +  public function __construct(OrderInterface $order) {
   

   In https://www.drupal.org/project/commerce_stripe/issues/3171408 and other issues we found it necessary to pass the payment object to CreatePaymentIntent(), so let's also add this here as an optional constructor argument with associated getter.

4. +++ b/src/Plugin/Commerce/PaymentGateway/Stripe.php
   @@ -455,6 +456,15 @@ class Stripe extends OnsitePaymentGatewayBase implements StripeInterface {
   +      $intent_array = array_merge($intent_array, $transaction_data);
   

   I think this might need to use NestedArray::mergeDeep()

The maintainer @jsacksick has said elsewhere that we should extend commerce's EventBase for forwards-compatability:

+++ 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).

I'm not sure #3121771: Fix TransactionData event, include description and initial metadata can really be called duplicate. The original TransactionDataEvent was dispatched after payment intent creation at Stripe, and it would preserve that behavior. At that stage the payment entity has its state set and remoteId captured, the commerce payment entity has been saved, and events resulting from that dispatched.

The event constant should be:

Working on the points from #6, #7 and #10.

I think we need to also pass $intent_array to the event constructor, right? Otherwise event subscribers cannot inspect the existing intent attributes.

New patch addressing #6, #7, and passing in the attributes array for inspection.

A remaining question is whether the full intent array should be passed in like this and any modifications accepted.

If that's OK, then we don't need to merge arrays at all, because we can just get the entire modified intent attributes array back from the event.

If we are intending that the properties already in the intent array should not be modifiable, and the intent array is not passed in, then there's nothing stopping subscribers from adding those properties to the (empty) intent attributes array anyway - the code after the event dispatch will overwrite the originals, regardless of whether we use array_merge() or NestedArray::mergeDeep(). Right?

What is preferred here?

Alternate patch that will apply over #3259211: Provide more parameters to createPaymentIntent().

Nice.

A remaining question is whether the full intent array should be passed in like this and any modifications accepted.

+1

From #6:
I think we should close #3170903: Allow customising the payment intent as a duplicate, and then in this issue deprecate the existing transaction data event.

Need to do deprecation.

+++ b/src/Plugin/Commerce/PaymentGateway/Stripe.php
@@ -472,6 +473,16 @@ class Stripe extends OnsitePaymentGatewayBase implements StripeInterface {
+    $intent_attributes = $event->getIntentAttributes();
+    $intent_array = NestedArray::mergeDeep($intent_array, $intent_attributes);

I think we can simply do $intent_array = $event->getIntentAttributes() here.

And then in PaymentIntentEvent let's add an addIntentAttributes() method that uses NestedArray::mergeDeep() to merge the added attributes to the ones originally provided to the event. It's just a convenience so the event subscribers don't have to call getIntentAttributes(), merge deep with whatever additional attributes they want, and then call setIntentAttributes().

Needed to use this patch for adding an intent description.
I applied the approach in #17 and added a new addIntentAttributes() method. I also just added a @deprecated tag for #16

The only concern here is if we should dispatch the event in the same place the transaction data event is being dispatched in the createPayment() method and pass the payment object if a project needed to pass info about the drupal payment to stripe.

I guess we can pass an empty intent array to only update attributes added from the event subscriber and that can avoid updating all the intent attributes in createPayment() after payment save. So we can pass new PaymentIntentEvent($order, [], $payment) to replace new TransactionDataEvent($payment).

The only concern here is if we should dispatch the event in the same place the transaction data event is being dispatched in the createPayment() method

No. See #5.

PaymentIntentEvent is missing a use statement for NestedArray.

The comments in StripeEvents also need attention.

PaymentIntentEvent is only dispatched when a payment intent is created. It does not allow modification of transaction data if a payment intent is updated.

TransactionDataEvent did allow that, but is now deprecated, and it is only passed the payment at __construct(), not the intent attributes.

The only place we call PaymentIntent::update() is following the TransactionData event dispatch, to merge in any added metadata, and that is going to be removed.

I think we can remove any mention of updating a payment intent.

Do we also need a @deprecated tag on the TransactionDataEvent class? Subscribers are not required to use an event constant as the array key in getSubscribedEvents().

Patch as per #21-22.

+++ b/src/Event/StripeEvents.php
@@ -7,12 +7,24 @@ namespace Drupal\commerce_stripe\Event;
+  const PAYMENT_INTENT = 'commerce_stripe.payment_intent';

I think we should call this PAYMENT_INTENT_CREATE in cae we also want to create a PAYMENT_INTENT_UPDATE in the future.

This is not a replacement for every conceivable use case for TRANSACTION_DATA event because it is called earlier. But it should handle the 99% use cases rather better than the current approach, so I think its fine as is. We can always add further events in future if demmand is there.

OK and we'd just use the same event class when dispatching an update event.

This patch changes the event constant. Anyone relying on it will need to update their subscriber getSubscribedEvents() method.

It also adds "Will be removed in 2.x" to the deprecation notices, and the docblock clarifies that the event fires before the payment intent is created.

+++ b/src/Event/StripeEvents.php
@@ -7,12 +7,23 @@ namespace Drupal\commerce_stripe\Event;
+   * @deprecated Will be removed in 2.x. Use StripeEvents::PAYMENT_INTENT to

Constant needs changing here too.

RTBC as trivial and can be fixed on commit.

Committed, thanks!

Realized I forgot to commit the event class... Will tag a new release once tests are green.