Setup:
search_api with search_api_solr module. I have set up a solr backend and indexed my website. The index contains the rendered HTML output and the bundle of my entities (for facets). This works fine (so far so good).

Then I've created a view to search in this data backend. I've set it to display fields and set the rendered HTML output as the field I want to see.

Now whenever I searched something it tried to access not only solr but also my data backend. As this is huge, I would like to avoid it.
1.) I've found out that the access check is one of the things that loads my entities completely. I disabled that - so far so good :)
2.) I've found that my rendered entities are not part of the responses. Particularly in src/Plugin/views/query/SearchApiQuery.php in function addResults(array $results, ViewExecutable $view) they are not in the output array. Instead of that there is a field with the key "rendered_item|rendered_item". Its tag results from the code above.

If I rewrite the key of this item to be just rendered_item (e.g. by adding: 

      foreach($values as $key => $value) {
        //if($key == "rendered_item|rendered_item") {
        if(strpos($key, "rendered_item") !== FALSE) {
          $values["rendered_item"] = $value;
        }
      }

before
$view->result[] = new ResultRow($values);
in the end) everything works as intended.

So now I don't know why the code in SearchApiQuery.php does this pipe-thingies. Did I miss some configuration or is this a bug?

Thanks in advance!

Complete code in SearchApiQuery.php:

 /**
   * Adds Search API result items to a view's result set.
   *
   * @param \Drupal\search_api\Item\ItemInterface[] $results
   *   The search results.
   * @param \Drupal\views\ViewExecutable $view
   *   The executed view.
   */
  protected function addResults(array $results, ViewExecutable $view) {
    // Views \Drupal\views\Plugin\views\style\StylePluginBase::renderFields()
    // uses a numeric results index to key the rendered results.
    // The ResultRow::index property is the key then used to retrieve these.
    $count = 0;

    // First, unless disabled, check access for all entities in the results.
    if (!$this->options['skip_access']) {
      $account = $this->getAccessAccount();
      foreach ($results as $item_id => $result) {
        if (!$result->checkAccess($account)) {
          unset($results[$item_id]);
        }
      }
    }

    foreach ($results as $item_id => $result) {
      $values = [];
      $values['_item'] = $result;
      $object = $result->getOriginalObject(FALSE);
      if ($object) {
        $values['_object'] = $object;
        $values['_relationship_objects'][NULL] = [$object];
      }
      $values['search_api_id'] = $item_id;
      $values['search_api_datasource'] = $result->getDatasourceId();
      $values['search_api_language'] = $result->getLanguage();
      $values['search_api_relevance'] = $result->getScore();
      $values['search_api_excerpt'] = $result->getExcerpt() ?: '';

      // Gather any properties from the search results.
      foreach ($result->getFields(FALSE) as $field_id => $field) {
        if ($field->getValues()) {
          $path = $field->getCombinedPropertyPath();
          try {
            $property = $field->getDataDefinition();
            // For configurable processor-defined properties, our Views field
            // handlers use a special property path to distinguish multiple
            // fields with the same property path. Therefore, we here also set
            // the values using that special property path so this will work
            // correctly.
            if ($property instanceof ConfigurablePropertyInterface) {
//              dpm($field_id, "this is set here");
              $path .= '|' . $field_id;
            }
          }
          catch (SearchApiException $e) {
            // If we're not able to retrieve the data definition at this point,
            // it doesn't really matter.
          }
          $values[$path] = $field->getValues();
        }
      }

      foreach($values as $key => $value) {
        //if($key == "rendered_item|rendered_item") {
        if(strpos($key, "rendered_item") !== FALSE) {
          $values["rendered_item"] = $value;
        }
      }


      $values['index'] = $count++;

      $view->result[] = new ResultRow($values);
    }
  }
CommentFileSizeAuthor
#19 3031621-19--fix_views_field_extraction.patch17.77 KBdrunken monkey
#19 3031621-19--fix_views_field_extraction--interdiff.txt4.36 KBdrunken monkey
#18 3031621-interdiff-8-15.txt607 bytesDeFr
#15 3031621-15--fix_views_field_extraction.patch14.8 KBDeFr
#8 3031621-8--fix_views_field_extraction.patch14.84 KBdrunken monkey
#8 3031621-8--fix_views_field_extraction--tests_only.patch3.35 KBdrunken monkey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Knurg created an issue. See original summary.

Knurg’s picture

Knurg CreditAttribution: Knurg commented
Issue summary: View changes
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Category: Feature request » Bug report
Status: Active » Postponed (maintainer needs more info)

This code was added in #2911734: Passing processor-generated field values from search results to Views is broken specifically to make this work again after the changes in #2650986: Fix entity loading in SearchApiFieldTrait. The “pipe thingie” allows us to tell multiple fields that use the same property apart – especially for aggregated fields, you might easily have several, and there it’s very important which one you’re looking at. Therefore, we suffix all configurable properties with their field IDs.

As far as I can see, this is still working, and it also has test coverage. Not sure why it wouldn’t work in your case.
If you run this module’s tests locally, do they all pass?
In the end, you’ll probably need to debug this yourself. The problem seems to be in \Drupal\search_api\Plugin\views\field\SearchApiFieldTrait::extractProcessorProperty(), which should use 'rendered_item|rendered_item', too, but apparently doesn’t.

Also see #2898327: Keep SearchApiQuery::addResults() from loading all result items individually – currently, the $result->getLanguage() will, unfortunately, also load the item from the database.

Knurg’s picture

Knurg CreditAttribution: Knurg commented

I did some deeper search. If I add any other fields from the indexed ones every time my entities are queried. I don't know why and how this should make sense, but with the above patch I get nearly immediate responses from my drupal. When I query fields or if I don't have the patch from above it takes some time, because it loads all my entities...

I had a look at extractProcessorProperty - but I could not find how this should be involved here. None of my debugs goes through there - so it is probably not called?

Knurg’s picture

Knurg CreditAttribution: Knurg commented
Issue summary: View changes
Knurg’s picture

Knurg CreditAttribution: Knurg commented
Issue summary: View changes
Knurg’s picture

Knurg CreditAttribution: Knurg commented

I am still having this issue - I can only prevent loading of data from the system directly if I tell him to store the rendered_item in rendered_item :/

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Component: General code » Views integration
Status: Postponed (maintainer needs more info) » Needs review
FileSize
3031621-8--fix_views_field_extraction--tests_only.patch3.35 KB
3031621-8--fix_views_field_extraction.patch14.84 KB

Oh, you’re right! It requires very specific circumstances, but I could now indeed reproduce this.
I tried setting up the view like you did and then just manually setting $values['rendered_item'] and/or $values['rendered_item|rendered_item'] in SearchApiQuery::addResults(). The results:

  • As long as $values['rendered_item'] was set, that value was always used.
  • If that was missing, but $values['rendered_item|rendered_item'] set to some value, that value was used but it still called the processor’s addFieldValues() method! It just threw away the results afterwards to use the pre-set value.

Working from this basis, I was able to modify the tests to actually fail quite spectacularly (see attached tests-only patch). And then, I even managed to actually fix the tests with some modifications. It seems the core problem is that the modified combined property path (with field ID suffix) isn’t consistently used throughout the trait, so a property set there will sometimes be used, and sometimes won’t be.

The patch is just a POC/WIP at the moment, but seems to work fine. Would be great if you could test it (plus maybe others – even better). Reviews are, of course, also welcome, but I know that there are still some rough edges (mainly: undocumented method parameters).

The last submitted patch, 8: 3031621-8--fix_views_field_extraction--tests_only.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 8: 3031621-8--fix_views_field_extraction.patch, failed testing. View results

Knurg’s picture

Knurg CreditAttribution: Knurg commented
Component: Views integration » General code
Status: Needs work » Postponed (maintainer needs more info)

Looks great! :)

Keep going!

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Component: General code » Views integration
Status: Postponed (maintainer needs more info) » Needs work

Hm, can’t reproduce that fail locally. Let’s let the test bot try again …
(Also, you accidentally re-set the issue fields.)

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented

Weird, still fails for the test bot, and still passes for me.
Can someone reproduce this fail and either fix it, or at least tell me what the error 500 page says?

DeFr’s picture

DeFr CreditAttribution: DeFr at Axess Open Web Services commented

Adding a quick note here to state that I've tested the patch and it did fix the issue, correctly fetching the rendered item from Solr instead of computing it.

Going to check if I can reproduce those fails.

DeFr’s picture

DeFr CreditAttribution: DeFr at Axess Open Web Services commented
Status: Needs work » Needs review
FileSize
3031621-15--fix_views_field_extraction.patch14.8 KB

Good news: I can reproduce the fail. Even better news, I can easily reproduce it outside of the test env now that I know what's failing, it's the "Rendered item" field, if you add the version that's not indexed. It's failing because you've added LoggerTrait straight into SearchApiFieldTrait, but SearchApiRenderedItem was trying to use both SearchApiTrait and LoggerTrait itself, which now fails composition.

Fixing that is easy enough, attaching a patch that should come back green. RenderedItem was the only views field plugin using both those trait in search_api itself, but I'm a bit worried that someone out there might be doing the same and will face a not so nice break.

DeFr’s picture

DeFr CreditAttribution: DeFr at Axess Open Web Services commented

Forgot to mention the cause for this not failing for you ; I guess you ran the test using PHP 7.3, which introduced a detection for the following case

trait T {
    function doSomething() {
        return;
    }
}

trait S {
    use T;
}

class A {
    use T;
    use S;
}

It works in 7.3+ but throws a fatal error for older versions.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ at Calibrate commented
Status: Needs review » Reviewed & tested by the community

The explanation in #15 #16 makes a lot of sense. An interdiff would've been nice but at first glance the changes are very simple.

DeFr’s picture

DeFr CreditAttribution: DeFr at Axess Open Web Services commented
FileSize
3031621-interdiff-8-15.txt607 bytes

Attaching the missing interdiff, which is quite limited ;-)

Not sure about the RTBC, I've only made the test pass, but all in all this is drunken_monkey patch in #8 which was uploaded as a WIP (that's why I didn't attach the interdiff at first). That being said, it does seems to work fine, so it's tested and you've reviewed it, so I guess strictly speaking say it is RTBC ;-)

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Status: Reviewed & tested by the community » Needs review
FileSize
3031621-19--fix_views_field_extraction--interdiff.txt4.36 KB
3031621-19--fix_views_field_extraction.patch17.77 KB

Awesome, thanks a lot for debugging this!
Yes, I was aware of the problem earlier PHP versions have with multiple property inheritance through traits, this got me lots of times before. I’m still amazed, though, that PhpStorm doesn’t report this problem. Anyways, thanks a lot for finding it!

Also thanks to Joris for reviewing, of course!

Yes, it was still WIP as a bit of test code and some doc comments were still needed. (Also you forgot to remove the import for LoggerTrait when you removed its usage.) Should all be fixed in the attached patch revision. Please test/review and we can get this committed.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ at Calibrate commented
Status: Needs review » Reviewed & tested by the community

Looks like the previous review I did was not strict enough, thanks for picking that up @drunken monkey! Back to rtbc this goes :)

  • drunken monkey committed 342d078 on 8.x-1.x 
    Issue #3031621 by drunken monkey, DeFr, borisson_: Fixed fields returned...
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Status: Reviewed & tested by the community » Fixed

Awesome, great to hear!
Committed.
Thanks a lot again for everyone’s work in here!

Status: Fixed » Closed (fixed)

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