Add support for meta data and extra transaction data

patch applied cleanly, but was missing some `use` statements for me and a couple other minor things

ya, that previous one isn't gunna work lol

here's the interdiff

sorry for the mess. those patches were for the beta branches. re-roll for -dev branch

This makes sense. However it needs clean up.

1. +++ b/src/Event/PaymentMetadataEvent.php
   @@ -0,0 +1,88 @@
   +  protected $metaData;
   ...
   +  public function __construct(array $transaction_data, array $metadata, PaymentInterface $payment = NULL) {
   ...
   +    $this->metaData = $metadata;
   ...
   +  public function getMetaData() {
   +    return $this->metaData;
   ...
   +  public function setMetaData(array $metaData) {
   +    $this->metaData = $metaData;
   

   Metadata does not need to be camelcase (see getCacheableMetadata)

2. +++ b/src/Event/PaymentMetadataEvent.php
   @@ -0,0 +1,88 @@
   +   * @return \Drupal\commerce_order\Entity\OrderInterface|NULL
   ...
   +  public function getPayment() {
   +    return $this->payment;
   

   Incorrect type hint / return value.

3. +++ b/src/Plugin/Commerce/PaymentGateway/Stripe.php
   @@ -136,6 +164,24 @@ class Stripe extends OnsitePaymentGatewayBase implements StripeInterface {
   +    $extra_transaction_data = [];
   +    $meta_data = [];
   +    $event = new PaymentMetadataEvent($extra_transaction_data, $meta_data, $payment);
   

   These do not need to be instantiated before firing the event - which could just be passed empty array values, because they are immediately overwritten.

4. +++ b/src/Plugin/Commerce/PaymentGateway/Stripe.php
   @@ -136,6 +164,24 @@ class Stripe extends OnsitePaymentGatewayBase implements StripeInterface {
   +    // Get extra transaction data if it exists.
   +    $extra_transaction_data = $event->getTransactionData();
   +    if ($extra_transaction_data) {
   +      // Ensure we can't overwrite the stuff we need.
   +      $extra_transaction_data = array_diff_key($transaction_data, $extra_transaction_data);
   +      $transaction_data = array_merge($transaction_data, $extra_transaction_data);
   

   Not quite following. But this is trying to prevent any existing data in $transaction_data from being overwritten?

   We could do $transaction_data += $extra_transaction_data then, probably. Existing keys will not be overridden.

5. +++ b/src/Plugin/Commerce/PaymentGateway/Stripe.php
   @@ -136,6 +164,24 @@ class Stripe extends OnsitePaymentGatewayBase implements StripeInterface {
   +    // Get metadata if it exists.
   +    $meta_data = $event->getMetaData();
   +    if ($meta_data) {
   +      $transaction_data['metadata'] = $meta_data;
   +    }
   

   Does Stripe throw an error if the metadata is empty? Otherwise we could just simplify this.

Here is an updated patch. It needs some testing, per changes mentioned in #9 (points 9.4 and 9.5.)

I also renamed the event to CollectTransactionDataEvent, because the purpose of the event is to collect additional information. I'm still not sold on that name, however.

+++ b/src/Event/CollectTransactionDataEvent.php
@@ -0,0 +1,105 @@
+class CollectTransactionDataEvent extends Event {

+++ b/src/Event/StripeEvents.php
@@ -0,0 +1,20 @@
+   * @see \Drupal\commerce_stripe\Event\CollectTransactionDataEvent
...
+  const PAYMENT_METADATA = 'commerce_stripe.payment_metadata';

Naming.

TransactionDataEvent. Collects additional transaction details + metadata

Naming. Doing tests, now.

Here is a patch with a test module. This was used to post a transaction which supplied a description and metadata, as shown in the screenshot.

Uploaded the wrong screenshot.

I also tested with an empty metadata, and there were no errors.

Approach seems fine.

+  /**
+   * Get the transaction data.
+   *
+   * @return array
+   *   The transaction data.
+   */
+  public function getTransactionData() {
+    return $this->transactionData;
+  }
+
+  /**
+   * Get the transaction metadata.
+   *
+   * @return array
+   *   The metadata.
+   */
+  public function getMetadata() {
+    return $this->metadata;
+  }
+
+  /**
+   * Get the payment.
+   *
+   * @return \Drupal\commerce_payment\Entity\PaymentInterface
+   *   The payment.
+   */
+  public function getPayment() {
+    return $this->payment;
+  }
+
+  /**
+   * Sets the transaction data data array.
+   *
+   * @param array $transaction_data
+   *   The transaction data.
+   *
+   * @return $this
+   */
+  public function setTransactionData(array $transaction_data) {
+    $this->transactionData = $transaction_data;
+    return $this;
+  }
+
+  /**
+   * Sets the metadata array.
+   *
+   * @param array $metadata
+   *   The metadata.
+   *
+   * @return $this
+   */
+  public function setMetadata(array $metadata) {
+    $this->metadata = $metadata;
+    return $this;
+  }

It's odd that the getters and setters are not together here, like usual.

+
+  public static function getSubscribedEvents() {
+    return [
+      StripeEvents::TRANSACTION_DATA => 'addTransactionData',
+    ];
+  }
+
+  public function addTransactionData(TransactionDataEvent $event) {

Missing docblocks (inheritdoc for the first one, something sensible for the second one).

+  /**
+   * Name of the event fired to add additional transaction data.
+   *
+   * @Event
+   *
+   * @see \Drupal\commerce_stripe\Event\TransactionDataEvent
+   * @see https://stripe.com/docs/api#metadata
+   */

Would be great to document when this data is sent (when creating a payment? refunding a payment? creating a payment method?), and what data could be added as an example. Our test adds the order ID, but isn't that something we usually send anyway? If not, why not?

It's odd that the getters and setters are not together here, like usual.

Bah, yeah. I thought I had fixed that.

Missing docblocks (inheritdoc for the first one, something sensible for the second one).

Thanks. The second one should link to the links in the summary, so people understand the usefulness.

Would be great to document when this data is sent (when creating a payment? refunding a payment? creating a payment method?), and what data could be added as an example. Our test adds the order ID, but isn't that something we usually send anyway? If not, why not?

Okay, we'll need to document it is when a Charge transaction is created. One reason I added commerce_strip_test was an example, we can mention that.

Should we add an issue to always add the order_id and store_id to metadata, since it is supported? I wasn't sure how opinionated we should be out of the gate.

Addresses #18

Our test adds the order ID, but isn't that something we usually send anyway? If not, why not?

All for a follow up issue which passes store_id and order_id as metadata by default.

+++ b/tests/modules/commerce_stripe_test/src/EventSubscriber/TransactionDataSubscriber.php
@@ -0,0 +1,41 @@
+    $metadata['order_id'] = $payment->getOrderId();
...
+    $metadata['store_id'] = $payment->getOrder()->getStoreId();

Let's add order_id and store_id by default in this patch. That way we just add payment_uuid and maybe add some use case comments.

"Add payment_uuid because another service queries payments over JSON API via Stripe data"

This moves order_id and store_id to always exist in the metadata. The example in the test module now shows

    // Add the payment's UUID to the Stripe transaction metadata. For example,
    // another service may query Stripe payment transactions and also load the
    // payment from Drupal Commerce over JSON API.
    $metadata['payment_uuid'] = $payment->uuid();

Thanks, everyone!