Allow the display of processor-generated fields/properties in Views

The problem is similar to the one in #2782577-4: Fix extraction of configurable properties in processors: currently, extracting processor-added properties at search time isn't completely supported (or, in the case of Views, not at all). You'll have to manually add all fields contained in the aggregated field to the view and then do Views magic to achieve the desired behavior. (Or use Solr, which can be configured to load all indexed fields directly from Solr.)

In the long run, though, we probably want to support this. (Otherwise, we should at least remove them from the options when adding fields.)

Thanks a lot for the offer, that would be great!

The problem is in \Drupal\search_api\Plugin\views\field\SearchApiFieldTrait::preRender() – which, unfortunately, might easily be the most complex and ugly method in our whole code base. What it does is extract all necessary property values (necessary for that field handler) from the row's search item into the row object itself, so that they can afterwards be displayed.

The first problem is line 316 (current dev – 65743d3): this dismisses properties of datasources different from the current row's datasource, but fails to take datasource-independent properties ($datasource_id === NULL) into account. It shouldn't dismiss any rows for those properties. ($this->isActiveForRow() in the same line already correctly checks for that (although using is_null() in there would be better, just in case someone wants to use "0" as a datasource ID).

Then, in line 361 we extract the property explicitly by retrieving it from the parent property (or the search item itself, in many cases – including ours). There, you'd instead have to check again whether the property is datasource-independent and the parent is empty and, in that case, instead extract it using the appropriate processor – something like this:

$definition = $index->getPropertyDefinitions()[$name];
if ($definition instanceof ProcessorPropertyInterface) {
  $processor = $index->getProcessor($definition->getProcessorId());
  // …
}

The rest of the code for item value extraction can be taken from \Drupal\search_api\Processor\ProcessorPluginBase::extractItemValues() – you have to create a dummy item with the field you are interested in and then call $processor->addFieldValues() on it.

Also, it seems I didn't think about NULL values used as array keys being automatically cast to empty strings, so we have a bunch of wrong datasource IDs (empty string instead of NULL flying around the class). We should do if ($datasource_id === '') { $datasource_id = NULL; } between line 299 and 300 and between line 432 and 433.

That should do it, basically. Refactoring the code, especially to avoid duplication between this and \Drupal\search_api\Processor\ProcessorPluginBase::extractItemValues() would also be great, as would be tests.
But, as said, I know this is pretty complex code, so I'd also understand if you'll have to give up after all.

Great start, thanks a lot for tackling this!

Attached is a revision – the main part is still missing, though. It can just be adapted from the code in ProcessorPluginBase, though.
In any case, I think if we detect a processor-generated property, we need to use completely different code, so that's how I arranged it now.

Feel free to ask back here or ping me on IRC if you don't understand or disagree with some of my changes.

Are you still (interested in) working on this? Otherwise, please un-assign and I'll try to look at it when I have some time.

OK, so when do you think you'll be able to work on it?
This is not only blocking a stable release, but also another release blocker (#2650364: Add a "skip access checks" option to the Views relationship plugin), so definitely on the critical path. (But on the other hand, there's still a lot of other release blockers, so it's not totally urgent. But in the next week or two would be good.)

I don't really understand everything that's happening here, however as per usual I do have some nitpicks. I feel that this should be reviewed by @drunken monkey to make sure we don't miss anything.

1. +++ b/src/Plugin/views/field/SearchApiFieldTrait.php
   @@ -314,7 +320,7 @@ trait SearchApiFieldTrait {
              // Bail for rows with the wrong datasource for this property, or for
              // which this field doesn't even apply (which will usually be the
              // same, though).
   -          if ($datasource_id != $row->search_api_datasource || !$this->isActiveForRow($row)) {
   +          if (!$this->isActiveForRow($row)) {
   

   The comment here doesn't apply fully anymore, so let's rewrite that?

2. +++ b/src/Plugin/views/field/SearchApiFieldTrait.php
   @@ -354,6 +360,37 @@ trait SearchApiFieldTrait {
   +              $definitions = $index->getPropertyDefinitions($datasource_id);
   +              if (!$parent_path && isset($definitions[$name])) {
   +                $definition = $definitions[$name];
   +                if ($definition instanceof ProcessorPropertyInterface) {
   

   We can probably rewrite that if into just one statement to reduce unneeded nesting?

Thanks a lot for your effort here! I know how scary that method is to me, so it's really quite an accomplishment to fix this in a meaningful way!
However, it's still not quite what I had in mind. Please see the attached patch for how I would do this, and please try it out to verify it works correctly!
In any case, thanks again for your work!

Also thanks to Joris for your review!

We can probably rewrite that if into just one statement to reduce unneeded nesting?

The problem with that is that it wouldn't give proper type hinting in PhpStorm anymore. Also, the line gets pretty long then. I agree that the additional nesting is undesirable, but I still think it's the better variant.

LGTM

Lesbians, gays, transgenders and … mutants? Very open-minded of you, admirable!
Committed.

I'm just going to pop this here now. Will open another issue when I've investigated properly. But as far as I can see if you have multiple aggregated_field properties it keeps taking the first one.

Follow-up posted #2856915: Allow the display of more than one processor-generated fields/properties per type in Views