Contrib.social

Entity query alter with cacheable metadata leaks and triggers LogicException

Open on Drupal.org →
Created on 3 May 2019, almost 6 years ago
Updated 28 March 2024, 12 months ago

I have an entity query_alter that was adding a cacheable metadata to a jsonapi response and before it was working and now I get: `LogicException: The controller result claims to be providing relevant cache metadata`, any ideas? I am altering the json query by a passed argument on the url and I want that resource to be be varied by that url argument, but it seems I should not be adding it on the query alterer now, which would be the proper way?

I am/was doing this on 

function microsite_query_entity_query_alter(Drupal\Core\Database\Query\AlterableInterface $query) {
  $request = \Drupal::requestStack()->getCurrentRequest();
  $renderer = \Drupal::service('renderer');
  if ($request->isMethodCacheable() && $renderer->hasRenderContext()) {
    $build = ['#cache' => ['contexts' => ['url.query_args:microsite_site']]];
    $renderer->render($build);
  }
}

This stopped to work after recent upgrades to core/jsonapi.

@wimleers asked

What code path in JSON:API is triggering your code to be executed? JSON:API should have the appropriate “run entity queries in a render context so it can detect bubbled cacheability and associate it with the resulting response” logic in place. If it doesn’t, that’d be a bug.

I answered: Does this answer your question https://pastebin.com/T0RvpbX3? The first path I actually now see that it’s captured and bubbled up, however the others (one per media entity) are not. Please ignore the `microsite` controller. It’s currently a bare decorator for the jsonapi controller service to add the cache context on an overridden `buildWrappedResponse()` (PoC from yesterday), but I rather not have to do something like this.

He said yes and asked to fill in a ticket, so here it is.

In the end I removed the decorator end ended up replacing the whole jsonapi.entity_resource service with the same `buildWrappedResponse()` overridden to add my needed context but not to have to override also the constructor on the decorator.

From Slack: https://drupal.slack.com/archives/C6DJEP1EK/p1556217296004400 (likely to be lost in the backscroll noise but here for reference)

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
JSON API  →

Last updated 8 days ago

Created by

🇦🇷Argentina hanoii 🇦🇷UTC-3

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

  • Needs reroll

    The patch will have to be re-rolled with new suggestions/changes described in the comments in the issue.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment 12 months ago →
    🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
  • First commit to issue fork.
  • Merge request !7220entity-query-alter → (Open) created by immaculatexavier
  • Pipeline finished with Canceled
    12 months ago
    Total: 3843s
    #131195
  • Pipeline finished with Failed
    12 months ago
    Total: 272s
    #131249
  • Status changed to Needs review 12 months ago
  • Comment 12 months ago →
    🇮🇳India immaculatexavier

    Rerolled for the aforementioned requirement for 11.x

  • Status changed to Needs work 12 months ago
  • Comment 12 months ago →
    needs-review-queue-bot

    The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

  • Pipeline finished with Failed
    12 months ago
    Total: 195s
    #131510
  • Pipeline finished with Failed
    12 months ago
    Total: 188s
    #131537
  • Pipeline finished with Failed
    12 months ago
    Total: 501s
    #131543
  • Status changed to Needs review 12 months ago
  • Comment 12 months ago →
    🇮🇳India immaculatexavier
  • Status changed to Needs work 12 months ago
  • Comment 12 months ago →
    🇺🇸United States smustgrave

    Appears to have test failures.

  • First commit to issue fork.
  • Comment 22 days ago →
    🇪🇸Spain vidorado Logroño (La Rioja)

    I've managed to fix the test, but it's not a regression test—it only verifies the specific use case reported by the original poster. However, it should trigger the error, yet I haven't been able to reproduce it by undoing the fix and running the test.

    Could it be that this is no longer an issue?

  • Pipeline finished with Success
    22 days ago
    Total: 449s
    #430138
contrib.social Blog FAQ Discussions
Production build 0.71.5 2024