Make Json response cacheable

The IS lacks a bit of context/description. Taken from #3066540: Is it possible to use a cacheable response? → , the idea seems to not make this cacheable for Drupal itself, but allow a custom module to alter the cache metadata in order to have the response cached by the client – is that correct? Caching search results within Drupal is mostly pointless, as there are only few scenarios where you can expect to reach a reasonable cache hit rate.

Anyways, on the patch itself, I’m unfortunately no expert in the Drupal cache system, but most of the code does seem sensible. A few comments, though:

1. +++ b/src/Controller/AutocompleteController.php
   @@ -167,7 +172,9 @@ class AutocompleteController extends ControllerBase implements ContainerInjectio
   -            $label = $this->renderer->render($build);
   +            $label = $this->renderer->executeInRenderContext(new RenderContext(), function () use ($build) {
   +              return $this->renderer->render($build);
   +            });
   

   Wouldn’t this discard any caching information coming from that build array? As said above, I’m no expert (or even intermediate, I guess), but it does seem like we should extract the metadata from the render context afterwards?

2. +++ b/src/Controller/AutocompleteController.php
   @@ -177,15 +184,19 @@ class AutocompleteController extends ControllerBase implements ContainerInjectio
   +            $url = $suggestion->getUrl();
   +            if ($entity = $url->getOption('entity')) {
   +              $cacheable_metadata->addCacheableDependency($entity);
   +            }
                // Generate an HTML-free version of the label to use as the value.
                // Setting the label as the value here is necessary for proper
                // accessibility via screen readers (which will otherwise read the
                // URL).
   -            $url = $suggestion->getUrl()->toString();
   +            $url = $suggestion->getUrl()->toString(TRUE);
   

   $suggestion->getUrl()->toString(TRUE) already returns a cacheable object, no need to look for an entity manually (I think).

3. +++ b/src/Controller/AutocompleteController.php
   @@ -206,7 +217,12 @@ class AutocompleteController extends ControllerBase implements ContainerInjectio
   +    $this->moduleHandler()->alter('search_api_autocomplete_response', $response);
   

   This hook still needs to be documented in search_api_autocomplete.api.php.

Most of this is addressed in the attached patch revision. However, this still needs a hook documentation and, more importantly, tests. Also, it seems like it even breaks current tests? (Still setting to NR for the test bot, but will switch to NW right afterwards.)

I am getting the leaked metadata errors with the current patch. I think its due to the view been rendered within a block. Autocomplete builds the view, and theres a check in core to see if the view has a URL in `core/modules/views/src/Form/ViewsExposedForm.php`. If theres no URL its rendering one which is triggering the leaked metadata error.

Moving `$search->createQuery` inside `executeInRenderContext` resolves this.

@lstirk: Seems like what you describe might have already been fixed in 🐛 LogicException: Bubbling failed Fixed ? Please try with the latest dev version of the module.

Hi,

I've updated the patch to work with version 8.x-1.9 and modified the hook as follows:

<?php
    $this->moduleHandler()
      ->alter('search_api_autocomplete_response', $response, $search);
?>

The implementation should look like this:

<?php

use Drupal\Core\Cache\CacheableJsonResponse;
use Drupal\search_api_autocomplete\SearchInterface;

/**
 * Implements hook_search_api_autocomplete_response_alter().
 *
 * @see \Drupal\search_api_autocomplete\Controller\AutocompleteController
 */
function my_custom_module_search_api_autocomplete_response_alter(CacheableJsonResponse &$response, SearchInterface $search) {
  if ($search instanceof SearchInterface
    && $search->getIndexId() === 'my_custom_index') {
    $config = \Drupal::configFactory()->get('system.performance');
    $response->getCacheableMetadata()
      ->setCacheMaxAge($config->get('cache.page.max_age'));
    $response->getCacheableMetadata()
      ->addCacheContexts(['url', 'url.query_args']);
    $response->getCacheableMetadata()
      ->addCacheTags(['my_custom_tag']);
  }
}

?>

drunken monkey → closed merge request !5

Thanks, makes sense.
However, we are now using issue forks and MRs for development, so I created one and applied your patch.
Also, when posting patches an interdiff would have been very helpful (especially when you first rebase and then make further changes).

Still NW for tests and hook documentation.