RecurringOrderClose doesn't catch all exceptions

+++ b/src/Plugin/AdvancedQueue/JobType/RecurringOrderClose.php
@@ -99,7 +99,7 @@ class RecurringOrderClose extends JobTypeBase implements ContainerFactoryPluginI
-    catch (DeclineException $exception) {
+    catch (PaymentGatewayException $exception) {

Decline exceptions need to be handled specifically, to tell the job to retry.

What your patch should be doing instead is handling the more general PaymentGatewayException as a second catch() block, which then does something else.

heddn had the right fix for this in #2988470: Don't initiate payment of an already paid 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());
+    }

EDIT: Actually, advancedqueue already does this for us. In Processor.php we have:

    try {
      $job_type = $this->jobTypeManager->createInstance($job->getType());
      $result = $job_type->process($job);
    }
    catch (\Exception $e) {
      $job_type = NULL;
      $result = JobResult::failure($e->getMessage());
      watchdog_exception('cron', $e);
    }

So, sounds like we don't need to do anything?

@josebc
You said:

AcceptJs.php is throwing PaymentGatewayException exceptions that are not being catched by DeclineException causing handleDecline not to fire.

What's in that exception? Perhaps it's AcceptJs that is buggy?

> EDIT: Actually, advancedqueue already does this for us. In Processor.php we have:
> So, sounds like we don't need to do anything?

Not quite.

If we let advancedqueue catch this, then the job is treated as failed, and won't be retried, which is fine.

But we are left with:

- a recurring order in state 'needs_payment', rather than the more accurate 'failed'
- a subscription that is in state 'active', rather than move to the unpaidSubscriptionState

What we need is the part of handleDecline() that deals with the final attempt at dunning to clean up when an \Exception is caught.

Here's a patch.

I'm not too keen on the need to save the order outside of the new markOrderAsFailed() method, but I assume that handleDecline() saves only at the end in case the RecurringEvents::PAYMENT_DECLINED has subscribers that change the order.

There is an inconsistency that we should maybe think about:

When dunning fails, we treat the job as having succeeded:

      $result = JobResult::success('Dunning complete, recurring order not paid.');

When there's an \Exception, it seems logical to me to treat the job as having failed:

// code from my patch:
    catch (\Exception $exception) {
      // If something more general goes wrong, we assume it's not possible
      // or desirable to retry.
      $this->markOrderAsFailed($order);

      // Rethrow the exception so that Advanced Queue can log the job's failure.
      throw $exception;
    }

But that means that the same result -- an order that's marked as failed -- can be counted as a successful job, or as a failed job.

Updated patch, with tests.

(Though branch tests are failing, so they're not going to get run here...)

Committed a slightly different patch (Added a $save_order parameter to handleFailedOrder and updated the dependencies in RetryTest).