Recently I launched a client site that heavily used RedirectResponse for add to cart -> checkout, checkout redirection, auto-checkout, and others. We experienced some issues, but really hit a bug when finally we kicked on the page cache expiration to be a day rather than zero.

One of the biggest culprits was bad / lacking cache metadata which caused bug reported in #2573807: Fatal error: Call to a member function getTimestamp().

The end result made things happy:

      $url = Url::fromRoute('commerce_checkout.form', ['commerce_order' => $cart->id()])->toString(TRUE);
      $response = new TrustedRedirectResponse($url->getGeneratedUrl());
      $response->addCacheableDependency($url);
      $response->addCacheableDependency($cart);
      $event->setResponse($response);

So any time we throw a redirect we should pass $url->getGeneratedUrl() and also then $response->addCacheableDependency($url);

ALSO, we should document the need for specifying cache contexts on URL parameters. I had to run the following which caused #2877498: Dynamic cache does not respect query parameters

    $url = Url::fromRoute('commerce_checkout.form', ['commerce_order' => $order->id(), 'step' => $step_id])->toString(TRUE);
    $response = new TrustedRedirectResponse($url->getGeneratedUrl());
    $response->addCacheableDependency($url);
    $response->addCacheableDependency($order);
    $response->getCacheableMetadata()->addCacheContexts(['url.query_args:zuoraEc', ''url.query_args:refid']);

So maybe we'll need payment gateways to specify query params which bust cache? We had instances where changing the query param on the checkout form did not cause our redirect. And we can't just kill the entire cache for the checkout form, because performance.

Comments

mglaman created an issue. See original summary.

Wim Leers’s picture

Wim Leers’s picture

Issue tags: +D8 cacheability

The main question here is: why do you want to use cacheable redirects? Is that appropriate here?

Because: class TrustedRedirectResponse extends CacheableSecuredRedirectResponse.

skyredwang’s picture

I ran into this problem in multiple projects. Here is a related core bug: #2744715: Generated URLs should only have cache contexts on GET/HEAD

mglaman’s picture

Here is one reason why; out of habit:

      if (!($response instanceof SecuredRedirectResponse)) {
        try {
          // SecuredRedirectResponse is an abstract class that requires a
          // concrete implementation. Default to LocalRedirectResponse, which
          // considers only redirects to within the same site as safe.
          $safe_response = LocalRedirectResponse::createFromRedirectResponse($response);
          $safe_response->setRequestContext($this->requestContext);
        }
        catch (\InvalidArgumentException $e) {
          // If the above failed, it's because the redirect target wasn't
          // local. Do not follow that redirect. Display an error message
          // instead. We're already catching one exception, so trigger_error()
          // rather than throw another one.
          // We don't throw an exception, because this is a client error rather than a
          // server error.
          $message = 'Redirects to external URLs are not allowed by default, use \Drupal\Core\Routing\TrustedRedirectResponse for it.';
          trigger_error($message, E_USER_ERROR);
          $safe_response = new Response($message, 400);
        }
        $event->setResponse($safe_response);
      }

We have to use TrustedRedirectResponse for external URLs, like off-site payment gateways (PayPal.) But I over used it for local redirects.

mglaman’s picture

Status: Active » Closed (works as designed)

Okay, we're good in Commerce. Our main way of redirecting is the NeedsRedirectException which is a helper around EnforcedResponseException and TrustedRedirectResponse.

    $response = new TrustedRedirectResponse($url, $status_code, $headers);
    $cacheable_metadata = new CacheableMetadata();
    $cacheable_metadata->setCacheMaxAge(0);
    $response->addCacheableDependency($cacheable_metadata);

Cache max-age set to 0. Checkout uses this, payment checkout panes use this.

All this hoopla was on my end not properly implementing redirects and having a frantic week.