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.
Comment | File | Size | Author |
---|---|---|---|
#8 | findbyredirect_cache-2962966-8.patch | 935 bytes | peter.keppert |
| |||
#7 | findbyredirect_cache-2962966-7.patch | 1.07 KB | peter.keppert |
#3 | findbyredirect_cache-2962966-3.patch | 870 bytes | peter.keppert |
|
Comments
Comment #2
peter.keppert CreditAttribution: peter.keppert commentedComment #3
peter.keppert CreditAttribution: peter.keppert as a volunteer commentedPatch attached.
Comment #4
peter.keppert CreditAttribution: peter.keppert as a volunteer commentedComment #5
peter.keppert CreditAttribution: peter.keppert as a volunteer commentedComment #6
BerdirWe 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?
Comment #7
peter.keppert CreditAttribution: peter.keppert as a volunteer commentedHi, 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?
Comment #8
peter.keppert CreditAttribution: peter.keppert as a volunteer commentedComment #9
peter.keppert CreditAttribution: peter.keppert as a volunteer commentedComment #10
PasqualleYou can find more discussion about similar cache context issue here:
https://github.com/drupal-graphql/graphql/issues/584
Comment #12
BerdirThanks, committed.