Add support for per-bundle view modes in the rendered entity processor

Good work, thanks!
But it wouldn't be a Search API issue if the first patch was OK.
The attached one should be RTBC from my standpoint, though.

Committed.
Sorry for the long delay, and thanks again for your work!

Ah, damn! Actually wanted to post a re-rolled patch, because it didn't apply anymore, but then I got ahead of myself and pushed. And, of course, I made a mistake during rebasing.
Follow-up patch attached, hopefully fixes things again.

Since the test bot has been "running" for a day now, I just committed it. Let's hope for the best.

Hey,

my indexing now is going haywire. In the end it comes down to EntityViewBuilder->isViewModeCacheable($view_mode) expecting a string, but getting an array.
This in turn seems to come from preprocessIndexItems on line 213 handing over $this->configuration['view_mode'][$item->getDatasourceId()] which hands over the config set up with this issue (?), namely, instead of just "entity:node" -> default, this is "entity:node" -> array (page, article, ...)

Am I the first one the notice this? This seems to be pretty big broken issue, I find it hard to believe nobody noticed?

I'm not sure if I'm supposed to reopen this, or open a new issue.

I rewrote that part as below. I'm _really_ sure that this is not the right way to do this...

 // Annoyingly, this doc comment is needed for PHPStorm. See
    // http://youtrack.jetbrains.com/issue/WI-23586
    /** @var \Drupal\search_api\Item\ItemInterface $item */
    foreach ($items as $item) {
      $bundles = $item->getOriginalObject()->getDataDefinition()->getBundles();
      if (is_array($bundles) && !empty($bundles)) {
        $bundle = array_pop($bundles);
        if (empty($this->configuration['view_mode'][$item->getDatasourceId()][$bundle])) {
          continue;
        }
        else {
          $view_mode = $this->configuration['view_mode'][$item->getDatasourceId()][$bundle];
        }
      }
      else {
        if (empty($this->configuration['view_mode'][$item->getDatasourceId()])) {
          continue;
        }
        else {
          $view_mode = $this->configuration['view_mode'][$item->getDatasourceId()];
        }
      }
      if (!($field = $item->getField('rendered_item'))) {
        continue;
      }
      $build = $item->getDatasource()->viewItem($item->getOriginalObject(), $view_mode);
      $field->addValue($this->getRenderer()->renderPlain($build));
    }

Rerolling more polished patch.

Am I really the only one running into this?

This is fixing two things:

1.) As far as I can tell, the code until now simply does not respect the choice made in the configuration form for the view mode of the rendered item.

2.) build() returns a SafeString object, which can be casted to String, but initially is an object, which makes Unicode::truncateBytes() fail hard later on.

+      $build = $item->getDatasource()->viewItem($item->getOriginalObject(), $view_mode);
+      $content = $this->getRenderer()->renderPlain($build);
+      $field->addValue((string)$content);
     }
 
     // Restore the original user.

nit space when type casting.

1) Harder to test since views has a lot of issues with search-api currently.
2) Fixed.

Denchev a good first step would be to provide a failing test that tests the other selections to show what needs fixing at all.
Also the code is adding more complexity and we need tests for what is fixed already.

2) i don't see how you fixed anything with that interdiff?

+++ b/src/Plugin/search_api/processor/RenderedItem.php
@@ -210,8 +210,23 @@ class RenderedItem extends ProcessorPluginBase {
+      if (is_array($this->configuration['view_mode'][$item->getDatasourceId()])) {
...
+          $view_mode = $this->configuration['view_mode'][$item->getDatasourceId()][$bundle];
...
+        $view_mode = $this->configuration['view_mode'][$item->getDatasourceId()];

It seems to me like this value is not maintained consistently then? Let's fix writing the view mode config with a stable structure to simplify code and follow the schema. IMHO we should use a default bundle name resulting in one if less.

Also pointing at the general issue about incomplete views integration:
#2387017: Finish Views integration

Hey Miro,

I essentially wrote that patch. The bundle view thingie is optional right now. For example: we wrote some custom entities, which do not allow bundles and mix those with regular nodes (which have bundles).

So at preprocessIndexItem() time, their might or might not be a bundle attached.

The second issue is more straight forward. renderPlain() returns a SafeString object. Later on Unicode::truncateBytes is called on the return value and will fail. I decided to strip that out into a new issue.

Here is the separate issue: #2557723: RenderedItem processor returns SafeString object instead of string.

Separate Patch + Test change for RenderedItem processor.

Thank you for all the updates.
I still highly recommend to not have configuration that has two different formats. Once a string, once an array. This always leads to trouble.

I would propose that the string
'entity:node' => 'full',
should be uniformly written as

'entity:node' => [
  'default' => 'full',
],

Then the extended configuration has the same data structure.

'entity:node' => [
  'page' => 'full',
  'article' => 'teaser',
],

Code to access is then simple, because you can just init $bundle = 'default' and always access the settings the same way.

Well, you are right. With the new bundle selection in place, there will always be a view_mode present, so this will always be an array, so we do not have to check for that.
Except: when updating search_api now and already having an older config stored, that does not yet respect bundle.
Somebody else is welcome to write an update hook for that.

Attached is a reroll of that patch.

Forgot to copy over the test changes as well.

Thanks for the patch! (Although I'd say this should have gone into its own issue, not be discussed here.)

First off:

+++ b/src/Plugin/search_api/processor/RenderedItem.php
@@ -210,7 +210,16 @@ class RenderedItem extends ProcessorPluginBase {
+      $bundle = $item->getOriginalObject()->getValue()->bundle();

This will only work for content entities. The correct way to get the item's bundle is the datasource's getItemBundle() method. Fixed in the attached patch.

However, I also don't think that falling back to 'default' is really the way to go if there is no view mode configured. First off, this will produce errors for bundles that can't be viewed. Also, I don't think we specify anywhere that there's a special handling for a 'default' view mode, so even for those bundles that can be viewed, this is likely to produce an error.
While it's certainly not optimal either, I guess ignoring those datasources/bundles for which we don't have a view mode configured makes the most sense here (as was the previous practice – and as you, for some reason, still do if there are no settings for the datasource at all).

Somebody else is welcome to write an update hook for that.

We're currently still in Alpha, so no update hooks are necessary, yet.

I am not sure what I am supposed to do now :)

I am not sure what I am supposed to do now :)

Using my patch as the base, please change the code to skip the item if there is no view mode configured for its datasource/bundle (instead of using 'default'). Probably also log a warning, so the user is made aware of this (but only once per call to the method, I'd say).

+++ b/src/Plugin/search_api/processor/RenderedItem.php
@@ -225,6 +229,10 @@ class RenderedItem extends ProcessorPluginBase {
+      \Drupal::logger('search_api')->warning('Items without a view mode: ' . $empty_view_modes);

I see now there are X items without a view mode. But that doesn't help me in fixing them. If anything, then you should list the names of the items that need fixing so i have a reference where to start. Instead of counting you could create an array of wrongly configured references and then implode with the message.

Note that with logging, the user is not made (immediately) aware of the fact except when working through the logs. Unsure, but i think we also should output a message.

Thanks, looks already better!

Just a few stlye improvements, a more helpful log message and not counting items that cannot be viewed for the warning.
Revised patch attached, please test/review! Then, I think, we can finally commit here.

I see now there are X items without a view mode. But that doesn't help me in fixing them. If anything, then you should list the names of the items that need fixing so i have a reference where to start. Instead of counting you could create an array of wrongly configured references and then implode with the message.

I think my revised error message is good enough there. The user can then just review the configuration for that processor, and that's really all there is to it. This will usually occur when a new entity type or bundle is added without the "Processors" form being re-saved.
Hm, on the other hand, the user will probably also want to make sure that these items will then be correctly indexed.
Maybe we should even reject items completely from indexing if no view mode is set for them? This will more immediately notify the user, at least when indexing manually – and in other cases, there is really no other way than logging anyways; and it would ensure it's easy to properly index after fixing the configuration.

Note that with logging, the user is not made (immediately) aware of the fact except when working through the logs. Unsure, but i think we also should output a message.

I agree that this is not optimal, but on the other hand there are really a lot of problems for which people should review the logs. And for cron runs, or when an item is immediately indexed after being saved, there is really no other way to get the message reliably to someone to whom it is relevant (except in the latter case, if the editor is also a site admin).

I like the idea of stopping the indexing when the configuration is not satisfying. Dunno what all the implications would be. A reindex still seem to be needed, otherwise it would try to reindex them with every cycle and the system doesn't settle down?

Anyway it's one small detail. I would expect there are other similar problems with other configurations that can lead to a broken situation and needs reindexing after fixing.
To discuss some general "make end user better understand what's going on" issue i created #2566169: Provide access to log on index view

Rest looks pretty fine.

OK, then let's commit this.
Thanks again for your work on this, everyone!