FacetsHierarchy Taxonomy uses old (taxonomy_term_hierachy) table.

Setting to needs review for testbot.

Is this already committed? I think that's part of #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration? As you can see this completely fails on the Integration tests.

Also, with it now being a proper field, maybe those queries could be converted to entity queries instead so we don't have to rely on the internal DB structure anymore?

Also, with it now being a proper field, maybe those queries could be converted to entity queries instead so we don't have to rely on the internal DB structure anymore?

🎉

+++ b/src/Plugin/facets/hierarchy/Taxonomy.php
@@ -72,9 +72,9 @@ class Taxonomy extends HierarchyPluginBase {
-      $query = $this->database->select('taxonomy_term_hierarchy', 'h');
-      $query->addField('h', 'tid');
-      $query->condition('h.parent', $id);
+      $query = $this->database->select('taxonomy_term__parent', 'h');
+      $query->addField('h', 'entity_id');
+      $query->condition('h.parent_target_id', $id);

@@ -89,18 +89,18 @@ class Taxonomy extends HierarchyPluginBase {
-    $result = $this->database->select('taxonomy_term_hierarchy', 'th')
-      ->fields('th', ['tid', 'parent'])
-      ->condition('th.parent', '0', '>')
+    $result = $this->database->select('taxonomy_term__parent', 'th')
+      ->fields('th', ['entity_id', 'parent_target_id'])
+      ->condition('th.parent_target_id', '0', '>')
       ->condition((new Condition('OR'))
-        ->condition('th.tid', $ids, 'IN')
-        ->condition('th.parent', $ids, 'IN')
+        ->condition('th.entity_id', $ids, 'IN')

@@ -118,9 +118,10 @@ class Taxonomy extends HierarchyPluginBase {
-      $query = $this->database->select('taxonomy_term_hierarchy', 'h');
-      $query->addField('h', 'parent');
-      $query->condition('h.tid', $tid);
+      $query = $this->database->select('taxonomy_term__
+      parent', 'h');
+      $query->addField('h', 'parent_target_id');
+      $query->condition('h.entity_id', $tid);
       $parent[$tid] = $query->execute()->fetchField();

This is the wrong fix in my opinion.

It should be using the term storage handler, which has methods to fetch the hierarchy for you - and therefore will work with non sql entity storage.

The original code shouldn't interact with the tables direct.

IF we switched this to use the term storage handler, the code would work both now *and* after the core issue goes in

If we can indeed replace that code with methods from \Drupal\taxonomy\TermStorageInterface then that is definitely the best solution.

Also means that is not postponed but needs work and is major if not critical, as Drupal core will eventually break this. Maybe/hopefully in 8.5, if not then in 8.6.

It should be using the term storage handler, which has methods to fetch the hierarchy for you - and therefore will work with non sql entity storage.

The original code shouldn't interact with the tables direct.

IF we switched this to use the term storage handler, the code would work both now *and* after the core issue goes in

If I understand this correctly, we should be using \Drupal\taxonomy\TermStorageInterface::loadParents to replace this? Sounds like a good solution. I didn't know about those methods, that really helps, thanks!

Yep those methods are part of the API and will stay, the implementation underneath is up to core to keep consistent

@borisson_ Let me know what I can do to help here. This is soft-blocking #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration from landing in Drupal core 8.5, which in turn is blocking A) REST support for Term entities' parents, B) Migrate support for Term entities, C) more reliable Views integration for Term entities.

I'm going to try to implement the suggestion made in #7 tonight, my personal laptop is broken so I'll probably have some fun first with setting up a dev env on my windows machine first.

So if I can get all of that to work it shouldn't be too hard to actually implement #7. I'd love to see a review when I get the patch up. I'll tag a new release to help with upgrades after we get this in.

Great! I already rolled a patch for #2932067: Future patch to remove taxonomy_term_hierarchy.

I tested unit and kernel tests locally and they are still green. Hopefully the integration tests still pass as well. No interdiff, because it's a new approach.

+++ b/src/Plugin/facets/hierarchy/Taxonomy.php
@@ -72,15 +74,8 @@ class Taxonomy extends HierarchyPluginBase {
   public function getNestedChildIds($id) {
     $children = &drupal_static(__FUNCTION__, []);
     if (!isset($children[$id])) {
-      $query = $this->database->select('taxonomy_term_hierarchy', 'h');
-      $query->addField('h', 'tid');
-      $query->condition('h.parent', $id);

those methods AFAIK all have their own static cache, so you can probably remove that?

I don't think loadTree has the same structure through, that returns some weird object-not-really-entities thing by default. You probably want to get the ID's of those. Maybe you need to recursively call loadChildren() instead?

The tests that failed are now green locally, I also removed static cache, thanks @Berdir!

1. +++ b/src/Plugin/facets/hierarchy/Taxonomy.php
   @@ -72,11 +70,15 @@ class Taxonomy extends HierarchyPluginBase {
   -      $term = Term::load($id)->getVocabularyId();
   -      $children = $this->termStorage->loadTree($term, $id);
   +    $children = $this->termStorage->loadChildren($id);
   +    $subchilds = [];
   +    $children = array_filter(array_values(array_map(function ($it) {
   +      return $it->id();
   +    }, $children)));
   +    foreach ($children as $child) {
   +      $subchilds = array_merge($subchilds, $this->getNestedChildIds($child));
        }
   +    $children[$id] = array_merge($children, $subchilds);
        return isset($children[$id]) ? $children[$id] : [];
   

   You seem to be using the $children variable twice now, once to load the children terms and once as a left-over of the static cache. I think you can just do return array_merge(); on the second-last line.

2. +++ b/src/Plugin/facets/hierarchy/Taxonomy.php
   @@ -105,16 +107,13 @@ class Taxonomy extends HierarchyPluginBase {
        }
   +    $parent[$tid] = reset($parents)->id();
        return isset($parent[$tid]) ? $parent[$tid] : FALSE;
   

   same here, you don't need $parent anymore.

Fixed a coding standards issue. Should now be ready.

Looks pretty good to me visually.

One thing to point out is that those methods are not new in 8.x, they're basically a 1:1 port of helper functions in 7.x and earlier. So it is likely that they weren't used on purpose so far as this now loads the entities when we only need the ids. An entity query might be faster but that will only start to work with the core issue.

I tested this with ab, to make sure that @Berdir's worry wasn't too much. This was with all cache modules disabled. This is with 4 facets, 3 enabled and 1 with a taxonomy tree (with 12 items in it).

Without patch:

Requests per second:    1.05 [#/sec] (mean)
Time per request:       950.749 [ms] (mean)

With patch:

Requests per second:    1.04 [#/sec] (mean)
Time per request:       965.000 [ms] (mean)

So there is a difference but it is small. I think that with a bigger vocabulary there might be a bigger problem, so we should probably test that as well.

However, the increased readability and less complexity in the actual code is worth taking a small performance hit I think.

I'll try to test this again with a bigger vocabulary in the coming days.

However, the increased readability and less complexity in the actual code is worth taking a small performance hit I think.

… and the performance hit should disappear when updating to 8.6 (and hopefully #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration will also still land in 8.5).

Committed and pushed, thanks so much for helping out and for all this information.

🎉