Trying to get field values with

/** @var \Drupal\search_api\Item\Item $item */
$item->getFields();
// or
$item->getField('url');

throws error

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\rest\ResourceResponse.

This happens when Index has URL value for item.

How to reproduce bug:

  • Create search index and add URI (search_api_url) field and re-index content.
  • Create custom REST resource for GET drush generate rest-resource or drupal gprr.
  • Write custom Search API request for index where URI field exist. Not matter what you choose for parse mode and other stuff. Try to call $item->getFields() or $item->getField('url')->getValues().
  • Enable REST and make request to it. And you will see the exception.

My simple code to copy-paste for test:

<?php

namespace Drupal\MUMODULE\Plugin\rest\resource;

use Drupal\rest\Plugin\ResourceBase;
use Drupal\rest\ResourceResponse;
use Drupal\search_api\Entity\Index;
use Psr\Log\LoggerInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;

/**
 * Represents how to throw bug in REST resource.
 *
 * @RestResource (
 *   id = "search_api_bug_example",
 *   label = @Translation("Search API bug example"),
 *   uri_paths = {
 *     "canonical" = "/api/search-api-bug",
 *   }
 * )
 */
class SearchApiUriBug extends ResourceBase {

  /**
   * {@inheritdoc}
   */
  public function __construct(array $configuration, $plugin_id, $plugin_definition, array $serializer_formats, LoggerInterface $logger) {
    parent::__construct($configuration, $plugin_id, $plugin_definition, $serializer_formats, $logger);
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    return new static(
      $configuration,
      $plugin_id,
      $plugin_definition,
      $container->getParameter('serializer.formats'),
      $container->get('logger.factory')->get('rest')
    );
  }

  /**
   * Responds to GET requests.
   */
  public function get() {
    $index = Index::load('global_index');
    /** @var \Drupal\search_api\Plugin\views\query\SearchApiQuery $query */
    $query = $index->query();
    /** @var \Drupal\search_api\ParseMode\ParseModePluginManager $parse_mode */
    $parse_mode = \Drupal::service('plugin.manager.search_api.parse_mode')
      ->createInstance('direct');
    $query->setParseMode($parse_mode);
    $query->keys('TEST');
    $query->setFulltextFields();
    $query->range(0, 5);
    $search_result = $query->execute();

    if ($search_result->getResultCount()) {
      /** @var \Drupal\search_api\Item\Item $item */
      foreach ($search_result as $item) {
        $item->getFields();
      }
    }
    return new ResourceResponse('You will not see it ;)');
  }

}
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Niklan created an issue. See original summary.

Niklan’s picture

The patch which fix this error.

Niklan’s picture

Issue summary: View changes
borisson_’s picture

Status: Active » Needs review

The fact that we are not getting any errors in tests about this, means that we should write a new regression-test for this as well.

Status: Needs review » Needs work

The last submitted patch, 2: 295881-1.patch, failed testing. View results

drunken monkey’s picture

Component: General code » Framework
Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
2.8 KB
3.59 KB

Phew, that bug seems pretty complicated. The crux is apparently \Drupal\Core\Render\MetadataBubblingUrlGenerator::generateFromRoute() – when $collect_bubbleable_metadata is FALSE, this will bubble metadata to the render context, which causes EarlyRenderingControllerWrapperSubscriber to throw an exception because it thinks metadata is lost. So this seeming no-op change (at least I was confused how that would help) is indeed a workaround for the problem – we more or less explicitly throw away metadata, instead of implicitly, which convinces Core that we mean it and that's OK.

Anyways, a failing test for this would really be helpful. Setting up a complete REST test for this is probably way too much work, but maybe we can set up the same event subscriber and then trigger the exception, somehow? (Really not sure, though. Seems like we'd need to do an internal HTTP request for that.)
In any case, the attached revision should at least fix the existing fail. (The others were in HEAD.)

drunken monkey’s picture

Status: Needs review » Needs work

Setting to NW for regression test.

Andriy Khomych’s picture

Rerolled patch for the 8.x-1.x
However, tests have conflicts during rerolling. I guess it is better to rewrite some parts of them.

drunken monkey’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.81 KB

Thanks for posting!
The attached patch also contains the updated tests.

As this still seems to be relevant, and writing a test would really be a lot of work for a small problem, I think we can skip it in this case. Please confirm that the attached revision still works for you and I’ll commit it.

Status: Needs review » Needs work

The last submitted patch, 9: 2958811-9--url_processor_rest_error.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

drunken monkey’s picture

Test fail seems completely unrelated, but no harm in making that test a bit more robust regardless.

Also, removed one unused use statement.

Already got private feedback that the patch still works as expected, so will commit in a day or two if no-one complains.

Status: Needs review » Needs work

The last submitted patch, 11: 2958811-11--url_processor_rest_error.patch, failed testing. View results

drunken monkey’s picture

drunken monkey’s picture

Status: Needs review » Fixed

Committed.

  • drunken monkey committed c39358c on 8.x-1.x
    Issue #2958811 by drunken monkey, Niklan, Andriy Khomych: Fixed bug in...

Status: Fixed » Closed (fixed)

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