Using the List Value processor to display label for Search API Fields throws an error.

I guess this makes sense, no idea why this happens though, do you think it's possible to provide a test?

@ndrake86: oh, don't worry, you can take all the time you need :) I'm currently focused on #2794745: Use Search API's display plugin to fix facets

1. +++ b/src/Tests/ProcessorIntegrationTest.php
   @@ -534,4 +535,36 @@ class ProcessorIntegrationTest extends WebTestBase {
   +  protected function checkListProcessor() {
   

   This needs a docblock.

   /**
    * Tests the list processor.
    */
   

2. +++ b/src/Tests/ProcessorIntegrationTest.php
   @@ -534,4 +535,36 @@ class ProcessorIntegrationTest extends WebTestBase {
   +    $this->
   

   Not sure what this is supposed to be, but I guess this should be removed?

Can you rename the file to .patch, so the testbot can pick it up?

I changed some docs around. I think the fails here are not specific to this issue but rather related to something else, I just committed something that should fix this.

I'll have a look at the tests tonight.

Core provides interface \Drupal\Core\TypedData\OptionsProviderInterface that should be used

+++ b/src/Plugin/facets/processor/ListItemProcessor.php
@@ -116,13 +117,18 @@ class ListItemProcessor extends ProcessorPluginBase implements BuildProcessorInt
+      if (!$field instanceof FieldInterface) {
+        $function = $field->getSetting('allowed_values_function');
...
+        if (empty($function)) {
+          $allowed_values = $field->getSetting('allowed_values');
+        }
+        else {
+          $allowed_values = ${$function}($field);
+        }

Here should be check for \Drupal\Core\TypedData\OptionsProviderInterface
and it's methods used instead of direct settings access

Didn't find a solution with \Drupal\Core\TypedData\OptionsProviderInterface, but covered many case with this patch.

Setting to needs review for the testbot, not sure if that's the way to go though. At least we should also implement the integration test from #27.

The patch in #34 makes this already very unreadable / complex method even worse, I'm not too happy with that - but that might be fixed in a follow-up.

@andypost: the optionsProvider might be a good thing to do here - getPossibleOptions on that interface does look like what we need.

I debugged that a bit, and here's another bug - instead of $field_identifier (that is index related) you should use $index->getField($field_identifier)->propertyPath

Because mapping to fields should be done through typed data, without property path a lot of errors possible caused by name collisions
For example Node has field *labels* and Node attache Term has field *labels* - facet will try to use node field but should take property path in account

We're looking for a solution based on #27 (as that has tests + the code that @andypost mentioned.

+++ b/src/Plugin/facets/processor/ListItemProcessor.php
@@ -116,13 +117,18 @@ class ListItemProcessor extends ProcessorPluginBase implements BuildProcessorInt
+      if (!$field instanceof FieldInterface) {
+        $function = $field->getSetting('allowed_values_function');
...
+        if (empty($function)) {
+          $allowed_values = $field->getSetting('allowed_values');
+        }
+        else {
+          $allowed_values = ${$function}($field);
+        }

We're looking for a way to do this trough the entity API; in pseudocode

if ($field instanceof OptionsProviderInterface) {
$allowed_values = $field-> getPossibleOptions ();
}

Apart from that, we'd need to fix the test + if possible add additional unit / kernel test coverage. However that extra testcoverage is very low priority.

options_allowed_values should be an easier way to do this, thanks @swentel

This patch also makes some other changes for reducing the complexity of the plugin

+++ b/src/Plugin/facets/processor/ListItemProcessor.php
@@ -108,6 +109,7 @@ class ListItemProcessor extends ProcessorPluginBase implements BuildProcessorInt
+    $field = FALSE;

@@ -115,31 +117,28 @@ class ListItemProcessor extends ProcessorPluginBase implements BuildProcessorInt
-    if ($field) {
-      $function = $field->getSetting('allowed_values_function');
+    if (!$field) {
+      return $results;

I'd better use !empty() here and added interface check for field interface

Thanks for that suggestion @andypost, I did some additional refactoring of the code to make it somewhat easier to read.

No idea how to fix the tests - but manual testing confirms that it works.

Go testbot, go!

I think the integration test fail is related to #2809797: Create test for multiple facets on a page. - so I'll go there to figure that out.

So - just to keep everything updated in the issue queue and not just in my head:

- this patch needs to updated to fix the failing unit tests.
- the issue #47 links to should be the one where we fix the integration test so that this can be fixed.

Looks like #2809797: Create test for multiple facets on a page. is not the reason for the fails here. Diving back in.

The work I did on the plane.

Fixes unit tests.

The List item processor doesnt work for fields that are on an entity reference field on a document. Before this patch , we could get the field settings using the datadefinition method like in this patch https://www.drupal.org/files/issues/facets-Fix_TranslateEntityProcessor-... , but i wonder why we take a longer route like in this patch?

Thanks,
Sukanya

@sukanya.ramakrishnan: I don't think we were explicitly supporting that use-case. I don't mind adding it back in though if it's not hard to do. In any case, adding/supporting that usecase means that we'd need a test for that as well. I think we should probably do that in a seperate issue? Not sure. Maybe we can do that here as well.

Anyway, the datadefinition that we get from search api is not sufficient to get the information needed here.

Ok will create a new issue for this usecase so that this issue doesnt get blocked. Thanks!

regards,
Sukanya

Committed, thanks!

Reverted, tests were failing because of this.

+++ b/src/Plugin/facets/processor/ListItemProcessor.php
@@ -115,34 +118,45 @@ class ListItemProcessor extends ProcessorPluginBase implements BuildProcessorInt
+    if ($field instanceof FieldStorageDefinitionInterface) {
+      if ($field->getType() !== 'entity_reference' && $field->getName() !== 'type') {
+        $allowed_values = options_allowed_values($field);
+        return $this->overWriteDisplayValues($results, $allowed_values);
...
+    // If no values are found for the current field, try to see if this is a
+    // bundle field.
+    $list_bundles = $this->entityTypeBundleInfo->getBundleInfo($entity);
+    if ($list_bundles) {
+      foreach ($list_bundles as $key => $bundle) {
+        $allowed_values[$key] = $bundle['label'];

Why not allow ER field type?
And limit for "type" field name?

before using bundles you need to make sure that entity type support bundles

I can't remember why I did this. I'll update if I remember.

I think we can commit this, looks good. Going to give the testbot some time to look at this first - I made some small changes for readability.

Changing author.

Committed and pushed, thanks for sticking with this.