Problem/Motivation
I am adding support for redirects to module GraphQL. In the query resolver, I used findMatchingRedirect() function and the following error has been logged:
"LogicException: The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early. Returned object class: Drupal\Core\Cache\CacheableJsonResponse. in Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (line 154 of ...\core\lib\Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber.php)."
After some research, I came across #2630808: Response::getResponse() does not collect cacheability metadata from Url object, where same error was reported and subsequently identified that the issue lies in failure to collect cacheability metadata when calling Url::toString() in findByRedirect(), which is being called by findMatchingRedirect().

Proposed resolution
Collect cacheability metadata in Url::toString() call and add the resulting GeneratedUrl as a cacheable dependency to return value.

CommentFileSizeAuthor
#8 findbyredirect_cache-2962966-8.patch935 bytespeter.keppert
#7 findbyredirect_cache-2962966-7.patch1.07 KBpeter.keppert
#3 findbyredirect_cache-2962966-3.patch870 bytespeter.keppert
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

peter.keppert created an issue. See original summary.

peter.keppert’s picture

peter.keppert CreditAttribution: peter.keppert commented
Issue summary: View changes
peter.keppert’s picture

peter.keppert CreditAttribution: peter.keppert as a volunteer commented
FileSize
findbyredirect_cache-2962966-3.patch870 bytes

Patch attached.

peter.keppert’s picture

peter.keppert CreditAttribution: peter.keppert as a volunteer commented
Status: Active » Needs review
peter.keppert’s picture

peter.keppert CreditAttribution: peter.keppert as a volunteer commented
Assigned: peter.keppert » Unassigned
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Version: 8.x-1.1 » 7.x-1.x-dev
Status: Needs review » Needs work 
+++ b/src/RedirectRepository.php
@@ -113,9 +113,11 @@ class RedirectRepository {
     $query = $uri->getOption('query') ?: [];
-    return $this->findMatchingRedirect($path, $query, $language);
+    $returnValue = $this->findMatchingRedirect($path, $query, $language);
+    return ($returnValue) ? $returnValue->addCacheableDependency($generatedUrl) : $returnValue;
   }

We don't use camelCase in local variables, the () is also not necessary.

I don't quite understand what you are trying to do with your call, though? Why do you need to call this in for a decoupled request like that?

peter.keppert’s picture

peter.keppert CreditAttribution: peter.keppert as a volunteer commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
FileSize
findbyredirect_cache-2962966-7.patch1.07 KB

Hi, thanks, you are correct, styling issues resolved and updated patch attached.

As to the reasons for calling this function. GraphQL currently supports resolving only non-redirected routes and should support also redirects, that's why I need to determine whether path is redirected and if so, what is the final destination, which is then resolved as standard path. Aside from this particular use case, reading Redirect module, I understood findMatchingRedirect() as an API function to be called in case when I have a path and need to determine whether it is redirected and where to (taking chained redirects into account). Hence, I determined that omission to collect cacheable dependencies is an issue which should be addressed. Is my reasoning incorrect?

peter.keppert’s picture

peter.keppert CreditAttribution: peter.keppert as a volunteer commented
FileSize
findbyredirect_cache-2962966-8.patch935 bytes
peter.keppert’s picture

peter.keppert CreditAttribution: peter.keppert as a volunteer commented
Status: Needs work » Needs review
Pasqualle’s picture

Pasqualle
Location 🇭🇺 Budapest
CreditAttribution: Pasqualle as a volunteer commented

You can find more discussion about similar cache context issue here:
https://github.com/drupal-graphql/graphql/issues/584

  • Berdir committed 02ced4a on 8.x-1.x authored by peter.keppert 
    Issue #2962966 by peter.keppert: findByRedirect doesn't handle cacheable...
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Fixed

Thanks, committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.