Don't initiate payment of an already paid recurring order

+++ b/src/RecurringOrderManager.php
@@ -100,6 +101,13 @@ class RecurringOrderManager implements RecurringOrderManagerInterface {
+      throw new PaymentGatewayException(sprintf('Payment already processed for order #%s', $order->id()));

Nitpick: I think the standard for core is to use whatever the modern equivalent of format_text() is, so it's easier to read.

+++ b/src/Plugin/AdvancedQueue/JobType/RecurringOrderClose.php
@@ -106,6 +106,11 @@ class RecurringOrderClose extends JobTypeBase implements ContainerFactoryPluginI
+      return JobResult::failure($exception->getMessage());

Do we not want dunning to happen in this case?

Also, though probably too complex for this issue, but the RecurringOrderRenew queue item will cheerfully carry on and create a new recurring order no matter what happens in RecurringOrderClose. Presumably the intention is that even if dunning happens, you want the next order created to cover the new billing period, and if the dunning fails, then the next order will fail too... I'll file a new issue for that; it could be I'm missing something in the code: #2991359: final dunning retry should clean up the recurring order.

+    catch (\Exception $exception) {
+      // Payment gateways can fail for other reasons than decline, let's handle
+      // these by reporting correctly.
+      return JobResult::failure($exception->getMessage());
+    }

Let's handle this in #2984454: RecurringOrderClose doesn't catch all exceptions.

   public function closeOrder(OrderInterface $order) {
+    // Guard against duplicate charging if recurring goes haywire.
+    $payment_storage = $this->entityTypeManager->getStorage('commerce_payment');
+    $payments = $payment_storage->loadByProperties(['order_id' => $order->id()]);
+    if ($payments) {
+      throw new PaymentGatewayException(sprintf('Payment already processed for order #%s', $order->id()));
+    }

This needs to be replaced with an $order->isPaid() check, along with bumping the Commerce requirement to 2.10.

@@ -139,8 +147,8 @@ class RecurringOrderManager implements RecurringOrderManagerInterface {
     $subscriptions = $this->collectSubscriptions($order);
     /** @var \Drupal\commerce_recurring\Entity\SubscriptionInterface $subscription */
     $subscription = reset($subscriptions);
-    if ($subscription->getState()->value != 'active') {
-      // The subscription has ended.
+    if (!$subscription || $subscription->getState()->value != 'active') {
+      // The subscription does not exist or has ended.
       return NULL;
     }

This needs to be removed, it will be resolved in #2984703: RecurringOrderManager::renewOrder() needs to handle the case when no subscriptions exist anymore.

There's no need to throw an error. If the order was paid, we skip creating a payment. We should however ensure that the state is set to completed correctly. In CS terms, we need to make the function reentrant.

Look at this, but we need to land #3011667: Saving the order before its payment in PaymentGateway::onReturn() can cause data loss to Commerce first, and raise the recurring requirement to 2.13.

+++ b/src/RecurringOrderManager.php
@@ -100,6 +101,13 @@ class RecurringOrderManager implements RecurringOrderManagerInterface {
   public function closeOrder(OrderInterface $order) {
+    // Guard against duplicate charging if recurring goes haywire.
+    $payment_storage = $this->entityTypeManager->getStorage('commerce_payment');
+    $payments = $payment_storage->loadByProperties(['order_id' => $order->id()]);
+    if ($payments) {
+      throw new PaymentGatewayException(sprintf('Payment already processed for order #%s', $order->id()));
+    }

Do we have steps to reproduce for this that could be used to create a test?

The exception stuff has landed, but we don't want to throw an exception as it will cancel out a successful order. Here's a simple patch that checks the order state or ->isPaid() to determine if the method should continue.

Nice, but #10 doesn't address @bojanz's point in #8:

If the order was paid, we skip creating a payment. We should however ensure that the state is set to completed correctly.

Added fix for #11 and test.

+++ b/src/RecurringOrderManager.php
@@ -146,21 +158,23 @@ class RecurringOrderManager implements RecurringOrderManagerInterface {
+    if (!$order->isPaid()) {

We shouldn't really need that as the method would return before anyway, but I guess it won't hurt?

#14 I think it's good to have this, in case someone has added a state that isn't draft but can't be mark_paid. They really shouldn't, but you never know ... and just making the logic ultra-explicit is nice when we're taking money from customers.

Looks good to me! Committed to 8.x-1.x, thanks everyone :)

Haven't reviewed the actual logic of the patch... But just one weird thing I noticed is this change to the .info.yml file:

  - commerce:commerce (>=8.x-2.13)

This seems weird as the composer.json requires at least 2.16

The .info.yml change should probably be reverted.

This is what I get when running Commerce dev:

"Commerce Recurring requires this module and version. Currently using Commerce version (Currently using Unresolved dependency [error]
Commerce (Version >=8.x-2.13 required))"

Done!