Implement a Hierarchical Entity Processor

WIP; doesn't work; just checking in "progress".

This would then replace #2574945: Port the "Index hierarchy" processor, I guess? In that case, please merge the issues so that we don't implement the same functionality twice. If this could in the end work the same as the old processor, but support additional functionality, that would of course be great.
However, using the labels instead of the IDs looks to me pretty specific to a) taxonomy term references and b) the Solr backend. It also looks like it would reduce performance quite a bit, at least on the database backend. So maybe this should better live in the Solr module, where the advantages of this approach can really be used?

Also, it might help you to take a look at the D7 hierarchy processor, which already has working code for spotting fields that could be hierarchical.

This builds on the initial work in #2. I added a few todos--mainly, for non-taxonomy fields, is there a generic way of determining a hierarchy?

Patch inadvertently introduced a dependency on taxonomy, thus the failing tests.

Start of a unit test.

I've been thinking about a generic way of determining a hierarchy from a non-taxonomy entity reference field, and while the APIs are there, I'm not sure how to determine if a given reference should be considered a 'parent', which would be needed to establish hierarchy...

In manual testing, this has some oddities that result in infinite recursion.

Yesterday the previous method of using getOriginalType() == 'field_item:enity_reference' wasn't working, but now it does appear to work for entity reference fields. So I think the patch can be simplified to use that approach again, and hopefully that will remove the recursive behavior.

So getOriginalType() is highly finicky. Sometimes it returns field_item:entity_reference, but if the item has any data in that field, it returns list. I suspect this might be a bug somewhere in the data definition logic.

The difference in behavior of the original type was due to some logic in the Utility::extractField() method:

  $field->setOriginalType($data->getDataDefinition()->getDataType());

for some reason the $data here returns list for data type, whereas the $field data definition is properly set to field_item:entity_reference. For testing purposes, I've just removed this line since there is already logic to set the original type in the getter method.

This adds an integration test, and actually gets hierarchy indexing working for taxonomies.

Note, the 'combined' patch incorporates the patches from both #2743697: Only restore the Query index on __wakeup if a container exists and #2743689: Move ContentAccessTest::assertResults() to a trait, and can be re-rolled without those if/once they are committed.

This bit of code is always NULL when the db backend is used during testing. I haven't manually tested with solr yet to see if this is working or not.

          // The output of all the parents is saved in a new, dynamic field. This
          // is the list of all parents, concatenated and separated with a slash
          // as the lucene/solr guidelines suggest:
          // https://wiki.apache.org/solr/HierarchicalFaceting.
          $hierarchyField = $item->getField('search_api_hierarchy:' . $field->getFieldIdentifier());

This is the combined patch again with the working version of the patch in #2743689: Move ContentAccessTest::assertResults() to a trait.

Maybe like this?

Ah, good to see! (Had a similar problem recently, that's how I spotted this very quickly.)

Now that that's out of the way, here is a more detailed review of your patch:

1. +++ b/tests/src/Kernel/Processor/AddHierarchyTest.php
   @@ -0,0 +1,158 @@
   +  public static $modules = ['filter', 'taxonomy'];
   

   We only use the "old" array syntax in this module.

2. +++ b/tests/src/Kernel/Processor/AddHierarchyTest.php
   @@ -0,0 +1,158 @@
   +    $this->createEntityReferenceField('node', 'page', 'term_field', NULL, 'taxonomy_term', 'default', [], FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED);
   

   Very handy, thanks for that!

3. +++ b/tests/src/Unit/Plugin/Processor/AddHierarchyTest.php
   @@ -0,0 +1,105 @@
   +    $field = $this->prophesize(FieldInterface::class);
   

   Never saw the prophesize() method before, I think. What's the difference to getMock()?

   (Also, didn't know or had forgotten the ::class syntax – nice trick, I'll try to remember that.)

Also, I kind of wonder why you're adding two tests? If you already have a Kernel test, then why go to all the trouble with the unit test just for the alterPropertyDefinitions() method? (Which, by the way, doesn't even exist anymore – cf. #2575003: Make adding new properties/fields more a first-class operation of processors.) Just using real datasources instead of this complex construct of mocks would probably be a lot cleaner, too.

However, the biggest issues are with the processor itself (which I unfortunately looked at last):

1. +++ b/src/Plugin/search_api/processor/AddHierarchy.php
   @@ -0,0 +1,142 @@
   +  public function __construct(array $configuration, $plugin_id, array $plugin_definition, TermStorageInterface $term_storage = NULL) {
   +    parent::__construct($configuration, $plugin_id, $plugin_definition);
   +    $this->termStorage = $term_storage;
   +  }
   

   We don't use that method for dependency injection in our plugins, but rely entirely on setters called from create() instead.

2. +++ b/src/Plugin/search_api/processor/AddHierarchy.php
   @@ -0,0 +1,142 @@
   +  public function alterPropertyDefinitions(array &$properties, DatasourceInterface $datasource = NULL) {
   

   As said, this method doesn't exist anymore. (There are other changes necessary due to #2575003: Make adding new properties/fields more a first-class operation of processors, too.)

3. +++ b/src/Plugin/search_api/processor/AddHierarchy.php
   @@ -0,0 +1,142 @@
   +          'locked' => TRUE,
   

   Unless I'm mistaken, this doesn't do anything anymore – see #2574969: Add a Views-like UI for adding fields.

4. +++ b/src/Plugin/search_api/processor/AddHierarchy.php
   @@ -0,0 +1,142 @@
   +    $fields = $datasource->getIndex()->getFields();
   

   The offered properties shouldn't/can't be based on the index's fields. Instead, base them on the datasource's properties. (Also, this will get you all fields, not just those from the current datasource – so if a node contains a term reference field, a property for that will also be added to all other datasources on the index.)

5. +++ b/src/Plugin/search_api/processor/AddHierarchy.php
   @@ -0,0 +1,142 @@
   +        $properties['search_api_hierarchy:' . $field->getFieldIdentifier()] = new DataDefinition($definition);
   

   Property names can't contain special characters like colons.

6. +++ b/src/Plugin/search_api/processor/AddHierarchy.php
   @@ -0,0 +1,142 @@
   +          // The output of all the parents is saved in a new, dynamic field.
   +          // This is the list of all parents, concatenated and separated with a
   +          // slash as the lucene/solr guidelines suggest:
   +          // https://wiki.apache.org/solr/HierarchicalFaceting.
   

   As said in #3, this approach isn't suited for other backends, so I wouldn't want to use that here.

   More importantly, though, it also doesn't seem like you're actually using it? Instead you seem to do what I'd have suggested: just collecting all the IDs and and adding them to the field. (Should check for duplicates, though.)

7. +++ b/src/Plugin/search_api/processor/AddHierarchy.php
   @@ -0,0 +1,142 @@
   +          $hierarchyField = $item->getField('search_api_hierarchy:' . $field->getFieldIdentifier());
   +
   +          if (!is_null($hierarchyField)) {
   +            $hierarchyField->setValues($hierarchyValues);
   +          }
   +          $field->setValues($hierarchyValues);
   

   That field can never exist, since field IDs are now machine names. You'd want to check for fields using the appropriate property path (which, again, can't contain colons either, though) – you can just do that with $this->filterForPropertyPath().

   Also, I don't understand why you also set the original field's values? If you're gonna do that, why even go through all this trouble with the additional property?
   (Actually, I'd have suggested this anways, like we do it in Drupal 7: just let the user choose the fields that should be processed and then change the values for those fields to the set including all ancestors.)

Regarding the detection of hierarchical fields: as said, please see the module's Drupal 7 version. It's not enough to have an entity reference field: you want a reference to an entity type which itself contains a property with a reference to its own entity type. That way, you right away also know how to create the hierarchy: by just following the references in that property. (Whether this is actually a hierarchy or just some reference where using the processor doesn't make sense can just be left for the user to decide.)

Thanks for the very thorough review! Hopefully I don't miss anything here.

Never saw the prophesize() method before, I think. What's the difference to getMock()?

This was introduced late in the Drupal 8 development cycle, so core uses a mix of Prophecy and the native PHPUnit mocks. See https://phpunit.de/manual/current/en/test-doubles.html#test-doubles.prop.... Prophecy is generally a bit easier to use than the mocks in my experience.

Also, I kind of wonder why you're adding two tests?

Since I've removed the properties approach as suggested, there isn't much left to unit test. I added the unit test first and quickly ran into issues for the preprocessIndexItems method, so moved to kernel tests.

+++ b/src/Plugin/search_api/processor/AddHierarchy.php
@@ -0,0 +1,160 @@
+  /**
+   * Retrieves the term storage.
+   *
+   * @return \Drupal\Core\Entity\EntityStorageInterface
+   *   The term storage.
+   */
+  public function getTermStorage() {
+    if (!$this->termStorage) {
+      return \Drupal::getContainer()
+        ->get('entity_type.manager')
+        ->getStorage('taxonomy_term');
+    }
+
+    return $this->termStorage;
+  }

I've removed storing term storage in the latest patch and opted instead to just store the entity type manager, since that will be needed for the non-taxonomy-specific hierarchy work.

Note, this is still the combined patch for the other 2 issues mentioned in #12.

I'm now working on the general entity reference support.

Gah, I forgot to actually remove the unit test =)

This adds initial support for generalized entity reference hierarchies, which isn't quite working yet. It also adds a (currently failing) test. I had to refactor the FieldsProcessorPluginBase::testType() method to allow access to the field item (otherwise the form method would need overriding if that is preferable).

This gets generic entity references working.

This is till a combined patch containing both #2743697: Only restore the Query index on __wakeup if a container exists and #2743689: Move ContentAccessTest::assertResults() to a trait.

I missed the test plugin when refactoring the arguments to FieldsProcessorPluginBase::testType().

The configuration of this processor (schema.yml) can be simplified now that #2575003: Make adding new properties/fields more a first-class operation of processors is in. I'll have a look at this in more detail later today but that jumped out at me immediately.

I think this patch is looking pretty good, I found nothing really. Not sure if we want to add a testcase for this in the ProcessorIntegrationTest as well, but I'll let @drunken monkey decide on that.

1. +++ b/src/Plugin/search_api/processor/AddHierarchy.php
   @@ -0,0 +1,166 @@
   +  /**
   +   * {@inheritdoc}
   +   *
   +   * Override this method to only allow entity reference fields.
   +   */
   

   I don't think this is allowed in the coding standards, but @drunken monkey can confirm that.

2. +++ b/tests/src/Kernel/Processor/AddHierarchyTest.php
   @@ -0,0 +1,268 @@
   + * @coversDefaultClass \Drupal\search_api\Plugin\search_api\processor\AddHierarchy
   

   We haven't used this in our code yet, @drunken monkey, should we open an issue to do that?

Thanks for the revised patch!
However, there's still some way to go. Did you look at the D7 code already? It has a much more flexible approach, which will also work uniformly for taxonomy terms and other kinds of references. (Or re-read the last paragraph from #22 – I admit it's a bit complicated.) Also, I think we can do a better job at detecting entity references (without needing to change \Drupal\search_api\Utility::extractField()) – i.e., via $field->getDataDefinition(). There's bound to be examples of that lying around in the module, e.g., in the Views integration.

Also, using the fields processor plugin base just because of some similarities will make more problems than it solves, as evidenced by the changes you had to make to that base class. Instead, just copy the two methods you actually need.
This is already done in the attached patch, which also contains some other clean-up.

Not sure if we want to add a testcase for this in the ProcessorIntegrationTest as well, but I'll let @drunken monkey decide on that.

Yes, please add such a test case, too! That's pretty easy to write anyways.

I don't think this is allowed in the coding standards, but @drunken monkey can confirm that.

Indeed, it is not. But I removed that method anyways.

We haven't used this in our code yet, @drunken monkey, should we open an issue to do that?

Yes, probably.

ProcessorInterface::preprocessIndexItems signature has changed.

See https://www.drupal.org/node/2230915.

Updated the patch after the changes in #36. The patch should be working again.

- Implemented interface & trait from the old base class
- Removed extra isEntityReferenceField check during indexing
--- For some reason the field was sometimes cached as a wrong type
--- Since field type is checked while setting processor settings, this extra check is redundant

TODO:
- Implement D7 detection of hierarchical fields (comment #22 & #36)
- add testcase in ProcessorIntegrationTest

This adds a test to the processor integration test.

I had to update the schema file since this is no longer extending from a field processor.

I also renamed the configuration key from fields to hierarchy_fields, since otherwise these lines in the processor integration test always caused a config mismatch (since 'fields' is unset), and thus never passed:

    $this->assertTrue($processor, "Successfully enabled the '$processor_id' processor.'");
    if ($processor) {
      $actual_configuration = $processor->getConfiguration();
      unset($actual_configuration['fields'], $actual_configuration['weights']);
      $configuration += $processor->defaultConfiguration();
      $this->assertEqual($configuration, $actual_configuration, "Processor configuration for processor '$processor_id' was set correctly.");
    }

Alternatively to renaming that, we could update the test to only unset those values for field processors?

There are some other failures I'm looking into, and next I will try to make the hierarchy detection more in line with how it was achieved in 7.x.

This should fix the new tests. There are some failing locally, but those are failing on the main branch too.

Thanks a lot for everyone's work here!
I'll take a look at this later, just going through the issues for tagging, at the moment.

This should fix the new tests. There are some failing locally, but those are failing on the main branch too.

Are you using PHP 5? Can you, by any chance, reproduce the fails in #2784849: Tests fail w/ out of memory error.?
Would be really great to finally resolve that issue.

For the entity reference detection:

Also, I think we can do a better job at detecting entity references (without needing to change \Drupal\search_api\Utility::extractField()) – i.e., via $field->getDataDefinition().

Were you thinking something like this patch?

As for mimicking the 7.x approach for extracting hierarchies, I spent quite a bit of time studying that approach and must admit I'm a bit out of exercise with the entity wrapper api from 7.x, and didn't immediately see a clear way to reproduce that here. I think that refactor is the last bit to be done here though.

Are you using PHP 5? Can you, by any chance, reproduce the fails in #2784849: Tests fail w/ out of memory error.?

I'm not using php 5, but it's interesting that the fails I was seeing locally do not appear on the test bot.

While testing the patch with a taxonomy vocabulary, I noticed not all ancestors were being indexed. Only the first parent was selected.
Altered the patch to resolve this by changing the method 'loadParents($id)' to 'loadAllParents($id)'.

Good catch @Rodlangh! I'll update the test case to cover multi-parent terms.

This updates the test to check for terms with multiple parents are indexed properly.

Nice to see progress being made to hierarchical indexing. But I suppose that this latest patch isn't in a stage yet, where by giving parent term id as conditional filter in Views I could filter all its term's child nodes? I guess it requires a custom "Term (and its parents)" field which is not yet created?
Related case: https://www.drupal.org/node/2789213
Thank you for you effort.

Sorry, this took a while and now I didn't include the last two patches in my work. Attached is a suggestion how I would implement the "hierarchical property" detection. The interdiff is based on #47, though – I'll now try to incorporate the additional test assertions from #50.

I'm not using php 5, but it's interesting that the fails I was seeing locally do not appear on the test bot.

That's known to happen. I know I spent a bit of time getting them to run OK on my desktop PC, and on my laptop I discovered in Milan that they still don't pass. It's just a very tricky system.

Done. Thanks again for your great work!

Please test/review and tell me what you think of my approach!

(Also, one note: I think the node reference tests were previously wrong – if the condition is on parent_reference = "vegetable", why should the node itself ("vegetable") be contained in the results? Don't know why the tests passed for you, then, though – one of us seems to have some mistake there.)

Can't include the processor in ProcessorIntegrationTest since there are no potential hierarchy fields. We'd have to add one.

I have a single value term reference field. It is set to index as hierarchy.

The following error occurs when indexing:

Solarium\Exception\HttpException while indexing: Solr HTTP error: OK (400) {"responseHeader":{"status":400,"QTime":9},"error":{"metadata":["error-class","org.apache.solr.common.SolrException","root-error-class","org.apache.solr.common.SolrException"],"msg":"ERROR: [doc=fej1v2-solr-entity:node/25:en] multiple values encountered for non multiValued field its_my_publication_type: [2111, 2108]","code":400}} in Solarium\Core\Query\Result\Result->__construct() (line 106 of /Volumes/webdev/www/amplexor/my-research-portal/vendor/solarium/solarium/library/Solarium/Core/Query/Result/Result.php).

When I change the field to a multiple value field, the error disappears. How should this work?
- should hierarchy facets be set as multiple fields
- if so do we need to tag each level
- or is there a bug that the field cardinality should be set to multiple if it is a hierarchical solr field (even if the drupal field is not multiple)

As per usual, I have nitpicks, this was just a first time looking at the patch in-depth. I do think that we should have an integration test as well as the current Kernel test.

1. +++ b/src/Plugin/search_api/processor/AddHierarchy.php
   @@ -0,0 +1,308 @@
   +    static::setFieldsHelper($container->get('search_api.fields_helper'));
   

   That's odd, do we do this with a static method in other places?

2. +++ b/src/Plugin/search_api/processor/AddHierarchy.php
   @@ -0,0 +1,308 @@
   +        // The property might be an entity data definition itself.
   +        $properties[''] = $definition;
   

   Why do we set that as an empty string? should we set this to something more descriptive?

   $properties['_self'] sounds better imho.

3. +++ b/src/Plugin/search_api/processor/AddHierarchy.php
   @@ -0,0 +1,308 @@
   +            foreach ($property->getPropertyDefinitions() as $name_2 => $property_2) {
   

   I don't really like the $variable_2 naming strategy here.
   I don't think that's very descriptive.

4. +++ b/src/Plugin/search_api/processor/AddHierarchy.php
   @@ -0,0 +1,308 @@
   +          // Might happen, for example, if the property only exists on a certain
   +          // bundle, and this entity has the wrong one.
   

   Do we have a test for that? I think we should have one. (This is the exception)

5. +++ b/tests/src/Kernel/Processor/AddHierarchyTest.php
   @@ -0,0 +1,298 @@
   +    $this->installConfig(['filter']);
   

   Let's use the long syntax here as well.

6. +++ b/tests/src/Kernel/Processor/AddHierarchyTest.php
   @@ -0,0 +1,298 @@
   +#    $this->index->removeDatasource('entity:comment')->save();
   

   If it's needed that this is commented out, let's just remove it.

7. +++ b/tests/src/Kernel/Processor/AddHierarchyTest.php
   @@ -0,0 +1,298 @@
   +    $this->container->get('search_api.index_task_manager')->addItemsAll($this->index);
   +    $index_storage = $this->container->get('entity_type.manager')->getStorage('search_api_index');
   

   Let's reflow these lines so that they are not as long.

   $this->container
     ->get(...)
     ->addItemsAll($this->index);
   

I will try to actually test the patch soon.

@ mpp: I'm pretty sure that's just a bug in the Solr backend – the Search API doesn't care about single-valued vs. multi-valued fields anymore. So, unless you can reproduce it with the database backend, you should report it in the Solr module's issue queue.

@ Joris: As always, thanks a lot for your thorough review!

That's odd, do we do this with a static method in other places?

No, but since we need it in the static supportsIndex() method, I decided to do it this way.
But might make sense to instead just use \Drupal::service() in that method. I'll let you decide.

Why do we set that as an empty string? should we set this to something more descriptive?

A non-empty string would have the potential for a conflict, whereas there's surely no real empty property on the entity. Also, this already perfectly describes that the entity is right on the property itself, with no additional property path – otherwise, we'd have to use a "magic" value and have a special case for that later.

I don't really like the $variable_2 naming strategy here.
I don't think that's very descriptive.

No, it isn't, but do you have a better suggestion? $nested_property and $nested_nested_property? With three different levels, anything else seemed worse to me.

Attached is a patch containing fixes to the other small nitpicks – except for the tests, which I (or someone else) will have to take care of later.

I'll work on the tests.

This adds back the integration test with a taxonomy hierarchy field, and a generic entity reference one. Note that the entity reference field is not being picked up. I also cannot get a generic entity reference (eg, a field on the article bundle, pointing at the article bundle) to show up in manual testing either.

Posting this here for review, in case somebody else has a minute to debug.

The reason the kernel tests still pass is that AddHierarchy::getHierarchyFields() isn't used during indexing, and only called when constructing the configuration form, and from ::supportsIndex().

I'm going to look into this some more.

This change now detects self-referential fields. I did not make a comparison of bundles since it is possible a site might implement a hierarchy field that works across bundles. Good call on the integration test since it pointed out this issue!

Adding related issue in facets, depending on this issue

#65 still results in a multiple values encountered for non multiValued field when the term reference has a cardinality > 1. Opened #2801765 for follow up.

Thanks a lot, looks great!
In the attached patch I just fixed some small style nitpicks and tried to refactor getHierarchyFields() a bit. Is this better, Nick? We could also let findHierarchicalProperties() recurse into the sub-properties by itself, saving a few more lines of code, but I think that would just make things more complicated, not less.
Other than this, I don't really know how to make this code clearer. It's just rather complex.

Speaking of which, I'm not quite sure what this new addition for "self-referential fields" does, and I'm pretty sure it won't work correctly in all cases.
@ jhedstrom: Could you give me an example of which cases this covers that weren't covered before? Why do we need this?
In any case, we can't use the field ID as the second part of the array key, since that part is normally a property name – the field ID often corresponds to the property name, but not always, and it's wrong to assume it will. We should instead probably use the last part of the field's property path. Implemented that also in the attached patch, let's see if it works.

In any case, thanks again for the great work!

And this removes the few cases of camelCase variables.

Fatal error: Cannot redeclare non static Drupal\search_api\Processor\ProcessorPluginBase::$fieldsHelper as static Drupal\search_api\Plugin\search_api\processor\AddHierarchy::$fieldsHelper in /web/modules/contrib/search_api/src/Plugin/search_api/processor/AddHierarchy.php on line 29

Was caused by an old version, you may ignore this.

@ jhedstrom: Could you give me an example of which cases this covers that weren't covered before? Why do we need this?

A self-referential field would be a field on an entity, referencing that same entity type. For instance, a field on the node referencing other nodes. Functionally, it is described in both tests with the parent_reference field. Without the self-referential addition to AddHierarchy::findReferentialFields(), those fields are not available in the UI (see the integration test).

The failures are due to changes in #2782577: Fix extraction of configurable properties in processors. I'll work on this.

I simply made the fields helper property and methods static. The alternative was to call out to the global \Drupal::service() method in the static methods in the hierarchical processor, which seemed messier.

Thanks for looking into the fail and trying to fix this. However, I alway thought the static DI I used there was pretty ugly – moving that to the plugin base class is out of the question. I guess we just have to use \Drupal::service() in the static methods after all – can't really be helped. We could give them optional $fields_helper parameters to use DI at least for the calls from non-static context, but I don't think that would be worth it. I'm open to reconsider that, though.

Anyways, the attached patch should properly fix this now.

Thanks also for your explanation of "self-referential fields". However, I don't really understand why they shouldn't already be covered by the existing code. Are you sure this isn't just a fluke of the tests and can be fixed by changing/fixing them?
For example, if you index nodes and they have a field parent referencing other nodes – wouldn't our code then already correctly identify the self-reference by inspecting parent:parent (or parent:entity:parent)?

Or like this – just make the two static methods non-static and use a temporary object in supportsIndex()?
(Another idea, but requiring an API change, would be to make supportsIndex() non-static after all. This would also allow us meaningful static caching for those hierarchical fields – currently, they are computed (at least) twice per request.)

Are you sure this isn't just a fluke of the tests and can be fixed by changing/fixing them?

I don't think so. I manually tested with and without the self-referential code addition, and without, an entity reference field on a node, pointing to itself, just doesn't appear in the processor UI because it isn't detected via the properties logic.

I just tried out adding a node reference to nodes – and the hierarchy processor picked it up correctly (without the additional special case for self-referential fields). However, I then still couldn't get the tests to run correctly. From what I see in the code, adding the target bundles to the reference field settings should be enough (as added in the attached patch, together with static caching for getHierarchyFields()), but for some reason that just doesn't seem to get picked up.

What's even more irritating is that, when I then tried it again in the UI, it didn't work anymore, the field was not listed by the hierarchy processor. No clue what could cause that, I'm pretty sure I did exactly the same thing as before.

Just posting the patch now, I don't have time today to debug this further. Maybe we'll manage to find the cause yet – otherwise, the work-around I'd prefer is setting the bundles manually in some way. Seems idiotic, but it's unfortunately not the first time doing things the right way was just too complex for us in D8.

Hm, OK, then maybe I forgot removing the work-around before testing it the first time? Seems the only explanation.

Still, from what I can see, all the pieces should be in place to get the property added, it's just that the list of possible bundles gets lost somewhere along its way of five thousand different Core classes.
So, if we need a work-around, it should be adding the bundle(s) manually. Otherwise, the whole rest of the code is pointless, really – it will never find entity references that aren't base fields. (Although that still at least covers the most common case, term parents.)

I think the discrepancy between processing and the UI is that we use different logic for both. The tests test both the UI and the processing, which is why I'd needed to add that additional logic for self-referential fields. I'm on IRC now, where it might be easier to discuss.

+++ b/src/Plugin/search_api/processor/AddHierarchy.php
@@ -0,0 +1,316 @@
+          foreach ($properties as $property) {
+            $property_label = $property->getLabel();
+            $property = $this->getFieldsHelper()->getInnerProperty($property);
+            if ($property instanceof EntityDataDefinitionInterface) {

Put another way, the reason this isn't finding self-referential fields is that when it gets to one, it find the entity its referencing, then looks for properties on the entity that might reference itself.

We could just put a check here to see if the field is already pointing at the entity type, and if not, then dive into the properties (which works for things like nodes referencing hierarchical terms, etc).

OK, I now found the issue, and it seems it really can't be helped, Core doesn't implement this currently in any way.
The problem is this code in \Drupal\Core\Field\TypedData\FieldItemDataDefinition:

  public function getPropertyDefinitions() {
    return $this->fieldDefinition->getFieldStorageDefinition()->getPropertyDefinitions();
  }

Since the property definitions ("target_id" and "entity") are just taken from the field storage definition (which doesn't have any bundles set), there is no chance that they'd include the correct bundles. (Also, even if it would have the correct bundles, it would still fail for fields with multiple possible bundles due to \Drupal\Core\Entity\TypedData\EntityDataDefinition::getPropertyDefinitions(), which only uses the known bundles if it's a single one.)

The problem with the special case for self-references is that they solve the problem in a too specific use case. This problem means that, in general, no manually placed entity reference field will be recognized by this processor. It would work for taxonomy term parents (surely the most common use case, admittedly), but almost nothing else.
Moreover, it would also yield false positives when, e.g., article nodes contain references to page nodes. If page nodes don't contain that reference, you can't possibly use it to build a hierarchy.

With that in mind, here are two suggested solutions for this problem, in patch form:

1. Use our existing FieldsHelper::getNestedProperties() method to retrieve the properties' nested properties (instead of just calling getPropertyDefinitions()). This is a minimal change amd should cover all possible cases – but would also yield false positives, since the configured bundles aren't taken into account at all (all existing bundles are checked). Our "Add fields" UI already has that problem, though, so at least users will be used to that (and it has much less impact here).
2. For entity reference fields, manually extract the target_bundles setting and pass it on to findHierarchicalProperties(). This will avoid false positives and only return the really relevant properties, but uses a bit more code and might also have problems with non-Core entity references (if they also follow this scheme).

Please tell me which solution you'd find better!

I think I prefer the route in .2 - as that reduces the possibility of false positives. However the possibility of it not working with contrib module is scary.

So I think that's why we should use the first option, as to keep more compatibility with contrib entity references.

OK, then let's do that. The attached patch should fix the fail, and also cleans up some related code in the "Add fields" form.

I think this is looking good. +1 for RTBC here.

RTBC++ from me as well.

Great to hear, thanks for reviewing!
Committed.