Problem/Motivation

Let's look at the following code:

$term = Term::load(12);
$term->parent->target_id // returns always NULL, even if the term has a parent

The problem is that $term->parent is a special fish, because it is tagged as "hasCustomStorage" so its never loaded.

Not only does this PHP code not work as expected due to custom storage for the parent field, there are three other consequences:

  1. REST support is missing: it's impossible to access a term's parent term via core's Serialization module, core's REST module, contrib's JSON API module, contrib's GraphQL module or contrib's Default Content module (only the latter has a work-around: #2921377: When Drupal 8.6.0 is released, remove TermEntityNormalizer and tag a new release)
  2. Views integration is custom
  3. Migrate D8 source support would need to be custom. See #2935951: Copy migrate source plugin from migrate_drupal_d8 into migrate_drupal

Proposed resolution

Stop using custom field storage for Term's parent field.

Remaining tasks

None.

User interface changes

None.

API changes

Fixes REST API's responses. (As well as JSON API, GraphQL, etc.) The field simply had to be ignored until now by all clients, because there never was a sensible value in there. (As HEAD's \Drupal\Tests\rest\Functional\EntityResource\Term\TermResourceTestBase::getExpectedNormalizedEntity proves, and the newly added TermResourceTestBase::testGetTermWithParent().)

Note that this does NOT change \Drupal\taxonomy\Entity\Term or \Drupal\taxonomy\TermInterface. In earlier iterations of the patch, those were being changed. At the request of Field API maintainer @amateescu, this patch is postponing the addition of a better interface (methods on entity objects with a better DX) to a future issue that will add a TreeInterface (see @amateescu's comment at #201.4). We arrived at consensus for this approach in an IRC discussion, whose log you can find in #221.

Data model changes

None.

CommentFileSizeAuthor
#311 Screenshot-2018-1-15 Multiple parent filters Site-Install.png13.2 KBlarowlan
#311 Screenshot-2018-1-15 Multiple parent filters (Content) Site-Install.png37.5 KBlarowlan
#309 interdiff-2543726-309.txt7.47 KBdamiankloip
#309 2543726-309.patch68.8 KBdamiankloip
#300 Screenshot-2018-1-12 Multiple parent filters (Content) Site-Install.png19.46 KBlarowlan
#299 after.png15.85 KBlarowlan
#299 before.png15.09 KBlarowlan
#296 Screenshot-2018-1-12 Tags drupal 8 5 x.png30.48 KBlarowlan
#296 Screenshot-2018-1-12 Tags drupal 8 5 x(1).png13.16 KBlarowlan
#296 Screenshot-2018-1-12 content drupal 8 5 x.png11.97 KBlarowlan
#296 Screenshot-2018-1-12 content drupal 8 5 x(1).png9.92 KBlarowlan
#290 interdiff-290.txt2.16 KBamateescu
#290 2543726-290.patch65.95 KBamateescu
#288 2543726-288.patch70.36 KBWim Leers
#288 interdiff-280-283.txt2.75 KBWim Leers
#285 interdiff-285.txt7.05 KBamateescu
#285 2543726-285.patch63.17 KBamateescu
#283 2543726-283.patch69.1 KBWim Leers
#283 interdiff-280-283.txt2.75 KBWim Leers
#280 interdiff-2543726-280.txt3.4 KBdamiankloip
#280 2543726-280.patch63.77 KBdamiankloip
#278 interdiff-2543726-278.txt12.03 KBdamiankloip
#278 2543726-278.patch62.14 KBdamiankloip
#274 interdiff-2543726-274.txt9.51 KBdamiankloip
#274 2543726-274.patch54.29 KBdamiankloip
#271 interdiff-2543726-271.txt3.53 KBdamiankloip
#271 2543726-271.patch57.97 KBdamiankloip
#268 interdiff-2543726-268.txt611 bytesdamiankloip
#268 2543726-268.patch57.8 KBdamiankloip
#260 interdiff-2543726-260.txt6.43 KBdamiankloip
#260 2543726-260.patch57.66 KBdamiankloip
#257 interdiff-2543726-257.txt1.12 KBdamiankloip
#257 2543726-257.patch60.67 KBdamiankloip
#254 2543726-254.patch63.58 KBWim Leers
#254 interdiff.txt1.93 KBWim Leers
#249 2543726-249.patch64.04 KBWim Leers
#249 interdiff.txt2.34 KBWim Leers
#245 2543726-245.patch63.52 KBWim Leers
#245 interdiff.txt1.08 KBWim Leers
#238 interdiff-216-237.txt12.8 KBWim Leers
#237 2543726-237.patch63.71 KBWim Leers
#237 interdiff.txt856 bytesWim Leers
#236 2543726-236.patch63 KBWim Leers
#236 interdiff-232-236.txt755 bytesWim Leers
#234 interdiff-2543726-232-234.patch4.07 KBcaseylau
#234 2543726-234.patch63.06 KBcaseylau
#232 interdiff-2543726.txt610 bytesdawehner
#232 2543726-232.patch60.29 KBdawehner
#230 2543726-230.patch62.99 KBWim Leers
#230 interdiff.txt701 bytesWim Leers
#228 interdiff-2543726.txt671 bytesdawehner
#228 2543726-228.patch60.28 KBdawehner
#223 2543726-223.patch62.98 KBWim Leers
#223 interdiff.txt10.4 KBWim Leers
#222 2543726-222.patch65.22 KBWim Leers
#222 interdiff.txt1.08 KBWim Leers
#216 2543726-216.patch65.04 KBWim Leers
#216 interdiff.txt10.18 KBWim Leers
#205 interdiff-2543726-205.txt611 bytesdamiankloip
#205 2543726-205.patch57.81 KBdamiankloip
#203 interdiff-2543726-203.txt3.89 KBdamiankloip
#203 2543726-203.patch57.66 KBdamiankloip
#200 interdiff-2543726-199.txt2 KBdamiankloip
#200 2543726-199.patch58.12 KBdamiankloip
#195 interdiff-2543726-195.txt8.96 KBdamiankloip
#195 2543726-195.patch56.13 KBdamiankloip
#192 interdiff-2543726-192.txt1.98 KBdamiankloip
#192 2543726-192.patch54.25 KBdamiankloip
#190 interdiff-2543726-190.txt1.29 KBdamiankloip
#190 2543726-190.patch54.17 KBdamiankloip
#188 interdiff-2543726-188.txt730 bytesdamiankloip
#188 2543726-188.patch54.32 KBdamiankloip
#186 interdiff-2543726-186.txt14.35 KBdamiankloip
#186 2543726-186.patch54.31 KBdamiankloip
#179 make_term_parent-2543726-179.patch54.69 KBWim Leers
#174 make_term_parent-2543726-174.patch54.8 KBjibran
#174 interdiff-ad8d93.txt8.12 KBjibran
#171 2543726-171.patch55.71 KBWim Leers
#171 interdiff.txt930 bytesWim Leers
#169 2543726-168.patch55.7 KBWim Leers
#169 interdiff.txt8.17 KBWim Leers
#167 Screen Shot 2017-10-12 at 11.19.41.png9.4 KBWim Leers
#163 interdiff-2543726-163.txt941 bytesdamiankloip
#163 2543726-163.patch47.98 KBdamiankloip
#161 interdiff-2543726-161.txt5.36 KBdamiankloip
#161 2543726-161.patch47.98 KBdamiankloip
#159 interdiff-2543726-159.txt733 bytesdamiankloip
#159 2543726-159.patch47.18 KBdamiankloip
#151 interdiff-2543726-151.txt4.08 KBdamiankloip
#151 2543726-151.patch47.12 KBdamiankloip
#146 2543726-146-for-8.3.x-do-not-test.patch46.78 KBidebr
#143 2543726-143.patch46.66 KBtedbow
#143 interdiff-141-143.txt27.56 KBtedbow
#141 drupal-term_parent_entity_reference-2543726-141.patch74.51 KBmgalalm
#139 interdiff-2543726-128-139.txt17.77 KBmgalalm
#139 drupal-term_parent_entity_reference-2543726-139.patch46.63 KBmgalalm
#130 interdiff-25437326-108-130.txt4.75 KBvoleger
#130 drupal-term_parent_entity_reference-25437326-130.patch46.11 KBvoleger
#128 interdiff-25437326-108-128.txt5.74 KBvoleger
#128 drupal-term_parent_entity_reference-25437326-128.patch46.52 KBvoleger
#127 interdiff-25437326-108-125.txt3.37 KBvoleger
#125 drupal-term_parent_entity_reference-25437326-125.patch46.26 KBvoleger
#118 drupal-term_parent_entity_reference-25437326-118.patch45.41 KBvoleger
#115 drupal-term_parent_entity_reference-25437326-115.patch45.4 KBvoleger
#114 drupal-term_parent_entity_reference-25437326-114.patch45.23 KBvoleger
#110 drupal-term_parent_entity_reference-25437326-110.patch45.34 KBvoleger
#108 drupal-term_parent_entity_reference-25437326-108.patch46.89 KBGrimreaper
#100 interdiff_joint_91_93_95.txt2.44 KBWim Leers
#96 interdiff-2543726-95.txt506 bytesdamiankloip
#96 2543726-95.patch46.25 KBdamiankloip
#93 interdiff-2543726-93.txt909 bytesdamiankloip
#93 2543726-93.patch46.24 KBdamiankloip
#91 interdiff-2543726-91.txt2.15 KBdamiankloip
#91 2543726-91.patch46.15 KBdamiankloip
#89 interdiff-2543726-89.txt8.7 KBdamiankloip
#89 2543726-89.patch44.43 KBdamiankloip
#82 interdiff-2543726-82.txt6.02 KBdamiankloip
#82 2543726-82.patch50.22 KBdamiankloip
#80 interdiff-2543726-2543726-80.txt3.67 KBdamiankloip
#80 2543726-80.patch46.26 KBdamiankloip
#78 term_parent-2543726-78-term-do-not-test.patch43.49 KBpcambra
#74 Selection_294.png30.57 KBdagmar
#71 interdiff-2015149-70-71.txt1.59 KBdagmar
#71 dblog-2015149-71.patch43.87 KBdagmar
#70 interdiff-66-70.txt3.08 KBtedbow
#70 2543726-70.patch45.45 KBtedbow
#69 term_parent-2543726-69-term-do-not-test.patch42.41 KBdmouse
#66 interdiff-2543726-63-66.txt6.41 KBdagmar
#66 term_parent-2543726-66.patch42.38 KBdagmar
#63 interdiff-2543726-54-62.txt589 bytesFredCorreia
#63 term_parent-2543726-62.patch42.3 KBFredCorreia
interdiff-2543726-54-62.txt589 bytesFredCorreia
term_parent-2543726-62.patch42.3 KBFredCorreia
#54 interdiff-2543726-45-54.txt3.07 KBdagmar
#54 term_parent-2543726-54.patch42.29 KBdagmar
#45 interdiff.txt5.26 KBclaudiu.cristea
#45 term_parent-2543726-45.patch42.03 KBclaudiu.cristea
#44 2627678-42.patch22.93 KBclaudiu.cristea
#44 interdiff.txt5.26 KBclaudiu.cristea
#41 interdiff.txt7 KBclaudiu.cristea
#41 term_parent-2543726-41.patch41.87 KBclaudiu.cristea
#39 term_parent-2543726-38.patch42.05 KBclaudiu.cristea
#38 interdiff.txt19.05 KBclaudiu.cristea
#38 interdiff.txt19.05 KBclaudiu.cristea
#34 issue-2543726-comment-34.patch43.19 KBdbjpanda
#11 interdiff.txt1.97 KBclaudiu.cristea
#11 2543726-11.patch42.69 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,253 pass(es). View
#9 2543726-9.patch42.72 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,842 pass(es). View
#9 interdiff.txt9.64 KBclaudiu.cristea
#5 2543726-5.patch42.09 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,819 pass(es), 8 fail(s), and 0 exception(s). View
#1 2543726-1.patch2.05 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,625 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

claudiu.cristea’s picture

Status: Active » Needs review
FileSize
2.05 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,625 pass(es). View

Patch.

claudiu.cristea’s picture

Issue summary: View changes

Added beta evaluation.

dawehner’s picture

Can't we lazyload that somehow? I'm curious about that as additional queries to load an entity are probably problematic.

claudiu.cristea’s picture

@dawehner, is there a reason why we are not keeping the hierarchy (parents) in the default storage? I mean like users are referring their roles.

claudiu.cristea’s picture

FileSize
42.09 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,819 pass(es), 8 fail(s), and 0 exception(s). View

@dawehner,

It took me some time but this patch is removing the custom storage in favor of default storage for term hierarchy. I agree that this makes the transformation more complex. Some benefits:

  • We don't need to handle the term hierarchy with custom code and keep it in custom storage. The hierarchy become a normal entity reference storage.
  • Some TermStorageInterface and VocabularyStorageInterface methods can be moved in TermInterface and VocabularyInterface because they are not more dependent on pure SQL operations. See a table bellow.
  • In Views we can rely on native entity reference relationships, filters, arguments without needing to create term specific handlers, maybe except the "depth" related stuff. This I didn't test, so it remains as a future task. I just converted the handlers to use the new table instead the old one.

Methods that can be deprecated from storages and moved into entity classes because they are losing the SQL specific code:

Method to be deprecated in storage class New method in entity class
TermStorageInterface::loadParents() TermInterface::getParents()
TermStorageInterface::loadAllParents() TermInterface::getAncestors()
TermStorageInterface::loadChildren() TermInterface::getChildren()
VocabularyStorageInterface::getToplevelTids() VocabularyInterface:: getTopLevelTids()
dawehner’s picture

  1. +++ b/core/modules/forum/forum.views.inc
    @@ -74,7 +74,7 @@ function forum_views_data() {
    -      'hierarchy table' => 'taxonomy_term_hierarchy',
    +      'hierarchy table' => 'taxonomy_term__parent',
    
    +++ b/core/modules/taxonomy/taxonomy.install
    @@ -0,0 +1,71 @@
    +/**
    + * Convert the custom taxonomy term hierarchy storage to a default storage.
    + */
    +function taxonomy_update_8001(&$sandbox = NULL) {
    +  $database = \Drupal::database();
    

    You also need to update existing views ...

  2. +++ b/core/modules/taxonomy/src/Entity/Term.php
    @@ -67,38 +74,45 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
    +      // Assure that <root> parent is strict 0.
    +      if (empty($item->target_id) && $item->target_id !== 0) {
    +      }
    

    That looks a bit odd :)

  3. +++ b/core/modules/taxonomy/src/TermStorage.php
    @@ -208,11 +129,11 @@ public function loadTree($vid, $parent = 0, $max_depth = NULL, $load_entities =
    -        $query->join('taxonomy_term_hierarchy', 'h', 'h.tid = t.tid');
    +        $query->leftJoin('taxonomy_term__parent', 'p', 't.tid = p.entity_id');
    

    Is there a reason we need a leftJoin?

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
9.64 KB
42.72 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,842 pass(es). View
  1. #6.1: Added.
  2. #6.2: That was forgotten in the patch. Removed :)
  3. #6.3: No. We're consistent with the 2 tables. Each term have at least one record in {taxonomy_term__parent}. Fixed the join.
claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
42.69 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,253 pass(es). View
1.97 KB

Fixed the failure.

claudiu.cristea’s picture

chx’s picture

in preSave

    if (!$this->getParents()->count()) {
      $this->parent->target_id = 0;
    }

Is this valid? For example, what would various widgets do when referring a nonexisting entity?

dawehner’s picture

If I understand

  /**
   * {@inheritdoc}
   */
  public function isEmpty() {
    // Avoid loading the entity by first checking the 'target_id'.
    if ($this->target_id !== NULL) {
      return FALSE;
    }
    if ($this->entity && $this->entity instanceof EntityInterface) {
      return FALSE;
    }
    return TRUE;
  }

correctly then we should set NULL ...

claudiu.cristea’s picture

@chx, @dawehner,

#19 tells us that 0 (strict) is a non-empty value. That is good because we need to represent somehow the <root> parent. NULL would be the empty value and that is not stored in the backend. So, 0 (strict), is stored and we want that in order to represent the <root> pseudo-reference.

Off-topic, it seems that

if ($this->entity && $this->entity instanceof EntityInterface) {
      return FALSE;
}

is never triggered.

claudiu.cristea’s picture

Issue tags: +ER check for RC

Tagging.

amateescu’s picture

Issue tags: -ER check for RC

This falls a lot more into how taxonomy module works rather than entity reference..

g089h515r806’s picture

For taxonomy term:
why not remove the "taxonomy_term_hierarchy" and add parent column to table "taxonomy_term_field_data".

For menu system, the parent is store at one table "menu_tree", but for taxonomy term, it is in a seperate table.
Which benifit we could get by using a seperate table to store parent info for taxonomy term?

chx’s picture

multiple parents

g089h515r806’s picture

1, The Drupal Core taxonomy UI does not support "multiple parents" feature
2, I have build 100+ Drupal websites, never use "multiple parents" feature
3, If is there any third party module that support "multiple parents" feature? I have never used one of them

If "multiple parents" feature is never used, we can remove this feature from Drupal core in the future.

gcardinal’s picture

I dont agreed it is a good idea to remove this feature from Drupal - we are using it in a project with RESTful import right now.

'parent' => array(
'target_id' => 
),
chx’s picture

> The Drupal Core taxonomy UI does not support "multiple parents" feature
It does.

jibran’s picture

Version: 8.0.x-dev » 8.2.x-dev
Category: Bug report » Task
Issue tags: +Needs reroll

We need a change notice for this change as well.

modules can explicitly load the parent references using the API.

Then this is not a bug imo this is a task because this is an incomplete feature.

+++ b/core/modules/taxonomy/taxonomy.install
@@ -0,0 +1,86 @@
+    $factory = \Drupal::configFactory();
+    // Update views with a low level API.
+    foreach ($factory->listAll('views.view.') as $id) {
+      $view = $factory->getEditable($id);
+      $changed = FALSE;
+      foreach ($view->get('display') as $display_id => &$display) {
+        $path = "display.$display_id.display_options.relationships.parent.table";
+        if (!empty($table = $view->get($path)) && $table == 'taxonomy_term_hierarchy') {
+          $view->set($path, 'taxonomy_term__parent');
+          $changed = TRUE;
+        }
+      }
+      if ($changed) {
+        $view->save(TRUE);
+      }

This can go to post update function now.

dbjpanda’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
43.19 KB

Rerolled the patch on 8.2.x

dbjpanda’s picture

Can any one please point me what is my mistake in rerolling the patch.

andypost’s picture

I compared patches 11 vs 34 and there's few differences, maybe that caused test failures.

+++ b/core/modules/taxonomy/src/TermStorageInterface.php
@@ -20,6 +20,8 @@
+   * @deprecated in Drupal 8.0.x, will be removed before Drupal 8.1.0.

@@ -28,6 +30,8 @@ public function deleteTermHierarchy($tids);
+   * @deprecated in Drupal 8.0.x, will be removed before Drupal 8.1.0.

@@ -39,6 +43,9 @@ public function updateTermHierarchy(EntityInterface $term);
+   * @deprecated in Drupal 8.0.x, will be removed before Drupal 8.1.0. Use
+   *   \Drupal\taxonomy\Entity\Term::load($tid)->getParents() instead.

@@ -50,6 +57,9 @@ public function loadParents($tid);
+   * @deprecated in Drupal 8.0.x, will be removed before Drupal 8.1.0. Use
+   *   \Drupal\taxonomy\Entity\Term::load($tid)->getAncestors() instead.

@@ -63,6 +73,9 @@ public function loadAllParents($tid);
+   * @deprecated in Drupal 8.0.x, will be removed before Drupal 8.1.0. Use
+   *   \Drupal\taxonomy\Entity\Term::load($tid)->getChildren($vid) instead.

+++ b/core/modules/taxonomy/src/VocabularyStorageInterface.php
@@ -22,6 +22,9 @@
+   * @deprecated in Drupal 8.0.x, will be removed before Drupal 8.1.0. Use
+   *   \Drupal\taxonomy\Entity\Vocabulary::getTopLevelTids($vids) instead.

in 8.2.x removed 9.0.0
also first 2 ones needs to point what to use instead

claudiu.cristea’s picture

Status: Needs work » Needs review
Issue tags: +DevDaysMilan
FileSize
19.05 KB
19.05 KB
  • Rerolled for 8.2.x.
  • Applied #37
  • Fixed all BC breaks (#11 was created before API freeze).
claudiu.cristea’s picture

FileSize
42.05 KB

Ouch! Forgot the main patch.

claudiu.cristea’s picture

Fixed the update path thanks to @alexpott, here at Drupal DevDays Milan :)

chx’s picture

Status: Needs review » Needs work

I am sorry and I know I should not but I will butt in: the getParents method returning a field item list is a lousy API. It should return either the target IDs or the entities but not the field item list.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
5.26 KB
22.93 KB

@chx, yes, that perfectly make sense. Initially that was only a getter for $term->get('parent') but in order to be aligned with getAncestors() and getChildren() is more logical to be an array of entities keyed by entity id.

claudiu.cristea’s picture

FileSize
42.03 KB
5.26 KB

Sorry, The last patch was totally wrong, from other issue.

chx’s picture

So #43 was already pushing the new boundaries. I do not know what to say or how to say these days, really. Am I even supposed to give architectural advice any more? I really do not know. So let's make this very clear: this is just an opinion. You can take or leave it. Unlike #43 I have no problems if this goes in as is. With that said, I feel it might be a bit better if the new methods would live in a TermTreeStorage class and service. As far as I can see there's nothing that stops getParents, getAncestors and getChildren from living elsewhere. They are only calling public methods on a Term like id() and get() and the only protected property they are manipulating is added by and for getAncestors. Moving them out would, of course, open doors to a more efficient way to store a tree. Also I am thinking, although I really badly I don't know about these things that if you can separate a class into separate classes then you should. Single responsibility and all that.

andypost’s picture

  1. +++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
    @@ -128,4 +128,25 @@ public function defaultValuesFormSubmit(array $element, array &$form, FormStateI
    +   * @todo Add this to EntityReferenceFieldItemListInterface in Drupal 9.0.x.
    

    Looks that needs follow-up and link here

  2. +++ b/core/modules/taxonomy/src/Entity/Term.php
    @@ -234,4 +242,51 @@ public function getVocabularyId() {
    +  public function getParents() {
    +    $parents = $ids = [];
    +    // Cannot use $this->get('parent')->referencedEntities() here because that
    +    // strips out the '0' reference.
    

    why not use at and array_unshift() root?

  3. +++ b/core/modules/taxonomy/src/Entity/Term.php
    @@ -234,4 +242,51 @@ public function getVocabularyId() {
    +  public function getAncestors() {
    +    if (!isset($this->ancestors)) {
    

    why only ancestors are statically cached? but other method results not?

  4. +++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
    @@ -172,4 +172,23 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
    +   * @todo Add this to \Drupal\taxonomy\VocabularyInterface in Drupal 9.0.x.
    ...
    +  public static function getTopLevelTids($vids) {
    

    this need issue too
    otoh minor versions allows to extend interfaces

  5. +++ b/core/modules/taxonomy/src/TermHierarchyInterface.php
    @@ -0,0 +1,37 @@
    + * @todo Merge this into \Drupal\taxonomy\TermInterface in Drupal 9.0.x.
    ...
    +interface TermHierarchyInterface {
    
    +++ b/core/modules/taxonomy/src/TermInterface.php
    @@ -7,6 +7,8 @@
    + * @todo Merge here \Drupal\taxonomy\TermHierarchyInterface in Drupal 9.0.x.
    ...
     interface TermInterface extends ContentEntityInterface, EntityChangedInterface {
    

    also needs issue and what chx said

  6. +++ b/core/modules/taxonomy/src/TermStorage.php
    @@ -93,100 +69,46 @@ public function resetCache(array $ids = NULL) {
    -  public function deleteTermHierarchy($tids) {
    -    $this->database->delete('taxonomy_term_hierarchy')
    -      ->condition('tid', $tids, 'IN')
    -      ->execute();
    -  }
    +  public function deleteTermHierarchy($tids) { }
    ...
    -  public function updateTermHierarchy(EntityInterface $term) {
    -    $query = $this->database->insert('taxonomy_term_hierarchy')
    -      ->fields(array('tid', 'parent'));
    -
    -    foreach ($term->parent as $parent) {
    -      $query->values(array(
    -        'tid' => $term->id(),
    -        'parent' => (int) $parent->target_id,
    -      ));
    -    }
    -    $query->execute();
    -  }
    +  public function updateTermHierarchy(EntityInterface $term) { }
    

    this changes violates BC

dagmar’s picture

Status: Needs review » Needs work

It seems there are several improvements to this patch. Moving to Needs work.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joachim’s picture

This seems to me like a fairly clear use case for a calculated entity reference field, which AFAIK isn't yet possible in core. I think I saw an issue for this, but not managing to find it right now.

claudiu.cristea’s picture

Why would this be a calculated entity reference? It's a classical entity reference field in the most common way.

joachim’s picture

Yeah, sorry, ignore last comment. Not sure what I was thinking there!

mikeryan’s picture

Will the current patch fix the fact that an entity query on 'parent' doesn't work (#2599450: Drupal\Core\Entity\Query\QueryException: 'parent' not found in Drupal\Core\Entity\Query\Sql\Tables->ensureEntityTable)? If so, that could be closed as a dupe of this. If not, that's another follow-up...

In the meantime, is there a way through the API to query on parent (or, in my case, the lack of one), apart from a database query on taxonomy_term_hierarchy?

mikeryan’s picture

I see this patch does at least one entity query on 'parent' in the tests, so I've closed #2599450: Drupal\Core\Entity\Query\QueryException: 'parent' not found in Drupal\Core\Entity\Query\Sql\Tables->ensureEntityTable as a duplicate.

mikeryan’s picture

Status: Needs review » Needs work

I think this should be "Needs work" until the feedback in #47 and #48 is addressed.

Wim Leers’s picture

larowlan’s picture

Issue tags: +Default content
fgm’s picture

Also, when using this patch, adding a parent term as a view argument causes an incorrect query generation, using taxonomy_term__parent.parent instead of taxonomy_term__parent.parent_target_id, probably because that column was previously taxonomy_term_hierarchy.parent.

Wim Leers’s picture

Issue tags: +Needs tests, +VDC

@fgm Thanks for reporting that! That indicates that needs test coverage too then. And since it affects Views, tagging VDC too.

FredCorreia’s picture

I fixed the issue reported by @fgm in #60 by replacing a missed index value in TermViewsData.

claudiu.cristea’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 63: term_parent-2543726-62.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review
FileSize
42.38 KB
6.41 KB

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 66: term_parent-2543726-66.patch, failed testing.

dmouse’s picture

I use this path in my current project, works weel on 8.2.x branch if anyone needs it.

tedbow’s picture

Status: Needs work » Needs review
FileSize
45.45 KB
3.08 KB

@dagmar thanks for the re-roll!

Fixed \Drupal\Tests\rest\Functional\EntityResource\Term\TermResourceTestBase::getExpectedNormalizedEntity
because parent should have target_id => 0. See \Drupal\taxonomy\Entity\Term::preSave

Also added this to \Drupal\Tests\hal\Functional\EntityResource\Term\TermHalJsonAnonTest::getExpectedNormalizedEntity
I am less sure about this fix because \Drupal\Tests\hal\Functional\EntityResource\HalEntityNormalizationTrait says that

// In the HAL normalization, empty fields are omitted.

So it is. but then our actually entity has it. thoughts?
dagmar’s picture

Is there any reason to disable these tests?

tedbow’s picture

@dagmar no sorry I meant to undo that before I made the patch.
I just disable them locally so I can easily just run the 1 method.

dagmar’s picture

Status: Needs review » Needs work

From #46:

I feel it might be a bit better if the new methods would live in a TermTreeStorage class and service. As far as I can see there's nothing that stops getParents, getAncestors and getChildren from living elsewhere. They are only calling public methods on a Term like id() and get() and the only protected property they are manipulating is added by and for getAncestors. Moving them out would, of course, open doors to a more efficient way to store a tree.

.

From #48:

Items 2, 3 and 6.

From #61:

@fgm Thanks for reporting that! That indicates that needs test coverage too then.

(This is for the views integration).

dagmar’s picture

Issue summary: View changes
FileSize
30.57 KB

While trying to make a patch for test coverage from #61 I saw this:

The error displayed by the view is:

SQLSTATE[42S22]: Column not found: 1054 Unknown column 'taxonomy_term__parent.parent' in 'where clause': SELECT taxonomy_term_field_data.tid AS tid, taxonomy_term_field_data_taxonomy_term__parent.tid AS taxonomy_term_field_data_taxonomy_term__parent_tid FROM {taxonomy_term_field_data} taxonomy_term_field_data LEFT JOIN {taxonomy_term__parent} taxonomy_term__parent ON taxonomy_term_field_data.tid = taxonomy_term__parent.entity_id LEFT JOIN {taxonomy_term_field_data} taxonomy_term_field_data_taxonomy_term__parent ON taxonomy_term__parent.parent_target_id = taxonomy_term_field_data_taxonomy_term__parent.tid WHERE (taxonomy_term_field_data.vid IN (:db_condition_placeholder_0)) AND (taxonomy_term__parent.parent = :db_condition_placeholder_1) LIMIT 11 OFFSET 0; Array ( [:db_condition_placeholder_0] => letters [:db_condition_placeholder_1] => A )

Grayside’s picture

Using this patch on a project and ran into a problem that seems possibly related to its use. Sorry for the size of this comment given it may not be relevant.

On migrating taxonomy terms, if a term is stubbed it will fail to be properly created when the migration process revisits the stubbed term. This is true whether the stub is via parent "reference" or via any other kind of reference (node -> term).

This results in an error message such as:

Fatal error: Call to a member function hasTranslation() on null in core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php on line 52

which I have seen in other data model/data import issue queues such as Multiversion so it is probably an issue more general than this.

Specifically, it appears that the ChangedItem code fails to find an "original" entity (despite there being a stubbed one in the database). The entity the field loads as the current one being updated is a new entity, but not marked as new, leading to the complexity of being an entity with an apparently non-existent tid that is also marked as not new. The intersection of empty original property with not-new status causes the error.

I considered opening a new issue, but wanted to put it up for consideration here in case this issue is the cause. Unfortunately my use case requires this patch, so reproducing the problem without it would be very time consuming.

hchonov’s picture

@Grayside, the problem you are describing is not related to the patches here but to the issue #2841311: Initialized fields of an entity clone have a reference to the original entity object instead to the cloned entity object where exactly the same problem with the ChangedItem is described.

Wim Leers’s picture

@hchnonov++

pcambra’s picture

Adding a patch compatible with the latest 8.2.x version

damiankloip’s picture

I am looking into the views issues outlined in #74

damiankloip’s picture

Status: Needs work » Needs review
FileSize
46.26 KB
3.67 KB

Let's see how this does, this fixes the problem for me - but still needs explicit test coverage for the fix I found.

It seems to be a generic problem with views data generated for multi-value based fields. The code is assuming that if it only have one property/column it should just use the field name, which in this case was 'parent' but this field is multi-value, so it has a dedicated table, which means the actual schema column name should be used. I think it's correct to always use this as regular base fields will have the same result as the actual $field_name we were using before. So we basically just remove the $multiple logic. So it kind of worked by chance before, because field names and storage names were always the same. As a side note, configurable fields are not affected as they are handled slightly differently in views_views_data()

We also shouldn't need to keep the previous views data definition for the column in \Drupal\taxonomy\TermViewsData::getViewsData() now, it should be generated for us. This is why we have one working version and one broken version, as outlined in #71.

damiankloip’s picture

This should fix some(/most?) of the test failures. Not sure we actually need more tests here, this should be caught by the EntityViewsDataTest, but the expectations were tailored to return what was currently being returned, not what we expect to be returned. Also removed a bit more of the table data from TermViewsData as the generic EntityViewsData should be providing most of this now.

dawehner’s picture

  1. +++ b/core/modules/taxonomy/src/Entity/Term.php
    @@ -234,4 +242,51 @@ public function getVocabularyId() {
    +  }
    ...
    +    if (!isset($this->ancestors)) {
    

    Is there a reason we cache the ancestors but not the parents?

  2. +++ b/core/modules/taxonomy/src/Entity/Term.php
    @@ -234,4 +242,51 @@ public function getVocabularyId() {
    +          if (!empty($parent) && !isset($this->ancestors[$id])) {
    

    This check is a little bit weird, should be maybe check for !== NULL or so?

  3. +++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
    @@ -172,4 +172,24 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
    +  public static function getTopLevelTids($vids) {
    

    can we typehint the array?

  4. +++ b/core/modules/taxonomy/src/TermHierarchyInterface.php
    @@ -0,0 +1,38 @@
    +interface TermHierarchyInterface {
    

    I'm wondering whether we could add a follow up for siblings as well

  5. +++ b/core/modules/taxonomy/src/TermHierarchyInterface.php
    @@ -0,0 +1,38 @@
    +   * @return \Drupal\taxonomy\TermInterface[]
    +   *   The parent taxonomy term entities keyed by term id. If this term has a
    +   *   <root> parent, that item is keyed with 0 and will have NULL as value.
    +   */
    +  public function getParents();
    

    It would be nice to document the order of operations.

  6. +++ b/core/modules/taxonomy/src/TermStorage.php
    @@ -93,100 +69,46 @@ public function resetCache(array $ids = NULL) {
    +  public function deleteTermHierarchy($tids) { }
    ...
    +  public function updateTermHierarchy(EntityInterface $term) { }
    

    Is there no way to reimplement this method on the new storage?

  7. +++ b/core/modules/taxonomy/src/TermStorageInterface.php
    @@ -15,6 +15,10 @@
    +   * @todo Remove this method in Drupal 9.0.x. Now the parent references are
    +   *   automatically cleared when deleting a taxonomy term.
    
    @@ -23,6 +27,10 @@ public function deleteTermHierarchy($tids);
    +   * @todo remove this method Drupal 9.0.x. Now the parent references are
    +   *   automatically updates when when a taxonomy term is added/updated.
    +   *   https://www.drupal.org/node/2785693
    
    @@ -34,6 +42,9 @@ public function updateTermHierarchy(EntityInterface $term);
    +   * @deprecated in Drupal 8.2.x, will be removed in Drupal 9.0.0. Use
    
    @@ -45,6 +56,9 @@ public function loadParents($tid);
    +   * @deprecated in Drupal 8.2.x, will be removed in Drupal 9.0.0. Use
    
    @@ -58,6 +72,9 @@ public function loadAllParents($tid);
    +   * @deprecated in Drupal 8.2.x, will be removed in Drupal 9.0.0. Use
    

    Nitpick: Let's mention 8.3.x

  8. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -366,18 +366,16 @@ protected function mapFieldDefinition($table, $field_name, FieldDefinitionInterf
    +
         // Add all properties to views table data. We need an entry for each
         // column of each field, with the first one given special treatment.
         // @todo Introduce concept of the "main" column for a field, rather than
         //   assuming the first one is the main column. See also what the
         //   mapSingleFieldViewsData() method does with $first.
    -    $multiple = (count($field_column_mapping) > 1);
         $first = TRUE;
         foreach ($field_column_mapping as $field_column_name => $schema_field_name) {
    -      $views_field_name = ($multiple) ? $field_name . '__' . $field_column_name : $field_name;
    -      $table_data[$views_field_name] = $this->mapSingleFieldViewsData($table, $field_name, $field_definition_type, $field_column_name, $field_schema['columns'][$field_column_name]['type'], $first, $field_definition);
    -
    -      $table_data[$views_field_name]['entity field'] = $field_name;
    +      $table_data[$schema_field_name] = $this->mapSingleFieldViewsData($table, $field_name, $field_definition_type, $field_column_name, $field_schema['columns'][$field_column_name]['type'], $first, $field_definition);
    +      $table_data[$schema_field_name]['entity field'] = $field_name;
           $first = FALSE;
    

    This really should better have its own issue ... this reminds me of #2838040: Multi-value multi-column base fields are queried with incorrect column names in Views

claudiu.cristea’s picture

#85.6:

+++ b/core/modules/taxonomy/src/TermStorage.php
@@ -93,100 +69,46 @@ public function resetCache(array $ids = NULL) {
+  public function deleteTermHierarchy($tids) { }
...
+  public function updateTermHierarchy(EntityInterface $term) { }

Is there no way to reimplement this method on the new storage?

@dawehner, There's nothing to implement there after parent is a normal entity reference field. The hierarchy is automatically updated in entity/field API. We have to keep the empty methods just to honour the TermStorageInterface contract.

dawehner’s picture

@dawehner, There's nothing to implement there after parent is a normal entity reference field. The hierarchy is automatically updated in entity/field API. We have to keep the empty methods just to honour the TermStorageInterface contract.

Mh okay fair.

damiankloip’s picture

damiankloip’s picture

Status: Needs work » Needs review
FileSize
44.43 KB
8.7 KB

Removed the stuff from #2846614: Incorrect field name is used in views integration for multi-value base fields. Not sure what we should do about some of the query access related fails. This patch is not using a direct query anymore.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
46.15 KB
2.15 KB

This is not the prettiest, but it does ensure the same query access hooks are run when getParents() is called, just like before... and thus should fix the Drupal\taxonomy\Tests\TaxonomyQueryAlterTest There might be a better way to do this? Not sure. Either way, we need some kind of code we're not completely happy with there to keep BC I think.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
46.24 KB
909 bytes

This should work... (still not saying this is the best fix).

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
46.25 KB
506 bytes

Silly mistake. In theory this should bring it back to 2 fails, that would be fixed by #2846614: Incorrect field name is used in views integration for multi-value base fields

Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
    @@ -128,4 +128,26 @@ public function defaultValuesFormSubmit(array $element, array &$form, FormStateI
    +  public function removeItemsByTargetId($target_id) {
    +    if (isset($this->list)) {
    

    I can't see any indication why $this->list would ever be NULL or not unset? Yes, it can be an empty array, but then the foreach will just loop over it.

  2. +++ b/core/modules/forum/forum.views.inc
    @@ -74,7 +74,7 @@ function forum_views_data() {
           'title' => t('Has taxonomy term'),
           'id' => 'taxonomy_index_tid',
    -      'hierarchy table' => 'taxonomy_term_hierarchy',
    +      'hierarchy table' => 'taxonomy_term__parent',
           'numeric' => TRUE,
    

    I know we say entity tables aren't part of the API, but this technically wasn't really an entity table before and still, just renaming and changing that might make some people very unhappy if they where querying this table.

    I guess not many do, the only reference I could find outside of core is our D8 migration project.

    Not sure what to do, what we could think about is to keep this as a read-only mirror of the new data structure, that we update when terms are saved.

  3. +++ b/core/modules/taxonomy/src/Entity/Term.php
    @@ -50,34 +51,45 @@
      * )
      */
    -class Term extends ContentEntityBase implements TermInterface {
    +class Term extends ContentEntityBase implements TermInterface, TermHierarchyInterface {
    

    On the other side, based on our BC rules, we are allowed to add methods to TermInterface and we've done that on a few other entity types as well in minor updates, for example contact form.

  4. +++ b/core/modules/taxonomy/src/Entity/Term.php
    @@ -86,14 +98,11 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
    +    // Terms with no parents are mandatory children of <root>.
    +    if (!$this->get('parent')->count()) {
    +      $this->parent->target_id = 0;
         }
    

    Previously, no value meant "do not update".

    We partially lose that optimization now, on the other side we have a built-in optimization to not write a dedicated field table when the values didn't change, so this might actually be faster in the end?

    Still, I'm wondering if we really still need that 0 special case now. We also shouldn't have any API's relying on it as it was write-only anyway, and we could have a BC for that easily and drop the item if the value is 0?

  5. +++ b/core/modules/taxonomy/src/Entity/Term.php
    @@ -232,4 +240,64 @@ public function getVocabularyId() {
    +    foreach ($this->get('parent') as $item) {
    +      if ($item->target_id == 0) {
    +        // The <root> parent.
    +        $parents[0] = NULL;
    +        continue;
    +      }
    

    Hm.. continue indicates that it could have parent 0 *and* additional parents? Is that really something we support?

  6. +++ b/core/modules/taxonomy/src/Entity/Term.php
    @@ -232,4 +240,64 @@ public function getVocabularyId() {
    +    // @todo Better way to do this? AND handle the NULL/0 parent?
    +    // Querying the terms again so that the same access checks are run when
    +    // getParents() is called as in Drupal version prior to 8.3.
    +    $loaded_parents = [];
    +
    +    if ($ids) {
    +      $query = \Drupal::entityQuery('taxonomy_term')
    +        ->condition('tid', $ids, 'IN');
    +
    +      $loaded_parents = static::loadMultiple($query->execute());
    

    yeah, this is interesting..

    I suppose we could also switch to using $parent->access('view') on the loaded terms. Not sure what's better.

    There have been lots of discussions already around those tags, see #2073663: Taxonomy load functions filter for access control, #1947270: Add test coverage for the term_access query tag and #2073663: Taxonomy load functions filter for access control.

    Maybe @xjm has some thoughts on this.

  7. +++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
    @@ -172,4 +172,24 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
    +  public static function getTopLevelTids(array $vids) {
    +    $tids = \Drupal::entityQuery('taxonomy_term')
    +      ->condition('vid', $vids, 'IN')
    +      ->condition('parent.target_id', 0)
    +      ->execute();
    +    return array_values($tids);
    

    Ah, so here is one example where we need the explicit 0 to be stored. an isEmpty() might also work, not sure..

  8. +++ b/core/modules/taxonomy/src/TermStorage.php
    @@ -93,100 +69,46 @@ public function resetCache(array $ids = NULL) {
       public function loadAllParents($tid) {
    -    if (!isset($this->parentsAll[$tid])) {
    

    Do we lose the static cache here? Is it common that this is called multiple times per request? The terms themself are statically cached, as are the values, so the only overhead would be the repeated entity queries for access checks.

    And the static cache indicates that the access checking could actually be flawed anyway, as it doesn't check the current user, so if you load terms with another current user, you'd get the wrong ones...

  9. +++ b/core/modules/taxonomy/src/TermStorage.php
    @@ -202,11 +124,11 @@ public function loadTree($vid, $parent = 0, $max_depth = NULL, $load_entities =
    +        $query->addExpression('parent_target_id', 'parent');
    

    why not addField(), this isn't really an expression?

  10. +++ b/core/modules/taxonomy/src/TermStorage.php
    @@ -254,7 +176,9 @@ public function loadTree($vid, $parent = 0, $max_depth = NULL, $load_entities =
                 $term->depth = $depth;
    -            unset($term->parent);
    +            if (!$load_entities) {
    +              unset($term->parent);
    +            }
    

    That load_entities stuff is a mess :-/

    Tried to go through the code and calls a bit, to better understand this and I think it makes sense, as unsetting on the loaded term and then saving would remove the parents.

    Just one interesting side-note, it obviously
    doesn't work with translations as it hardcodes the default_langcode file, so anyplaces that uses this is broken on multilingual sites. Looks like there are only 3 non-test calls, in the term form for the parents and some helper function, which doesn't care about it, but at least the parent selection should...

  11. +++ b/core/modules/taxonomy/src/VocabularyStorage.php
    @@ -21,7 +22,7 @@ public function resetCache(array $ids = NULL) {
       public function getToplevelTids($vids) {
    -    return db_query('SELECT t.tid FROM {taxonomy_term_data} t INNER JOIN {taxonomy_term_hierarchy} th ON th.tid = t.tid WHERE t.vid IN ( :vids[] ) AND th.parent = 0', array(':vids[]' => $vids))->fetchCol();
    +    return Vocabulary::getTopLevelTids($vids);
       }
     
    

    is vocabulary really the right place to move this? Why not just switch to an entity query here and deprecate the method?

Wim Leers’s picture

So #89 has failures in TaxonomyQueryAlterTest.

The joint interdiff of #91 + #93 + #95 is what needs to be reviewed.


Review of #91+#93+#95 in particular:

  • the patch did not remove any entity queries. It only added 3. In \Drupal\taxonomy\Entity\Term::getParents() + \Drupal\taxonomy\Entity\Term::getChildren() + \Drupal\taxonomy\Entity\Vocabulary::getTopLevelTids()
  • Because TermStorage::loadParents() now calls Term::getParents(), it doesn't execute an entity query anymore. Nor does it need, nor did it ever need to: those always could and should have been loaded via $term->parent. However, this indeed breaks BC for TermStorage::loadParents(). So what about this: we keep TermStorage::loadParents() and don't change its logic, but deprecate it. That means no BC break, no crazy work-arounds for tests.
  • But the same cannot be said of Term::getChildren() and Term::getAncestors(). Here I have to agree with #47: it's not clear why this is being moved onto the Term class other than convenience.
  • Which then questions the legitimacy of \Drupal\taxonomy\TermHierarchyInterface.

In short: I think having Term::getParents() might make sense because \Drupal\taxonomy\Entity\Term::baseFieldDefinitions has a parent entity reference field (with unlimited cardinality). I don't understand why this justifies a new method though. I think it makes sense to remove the setCustomStorage(TRUE) for the parent base field so that you can do $term->parent->target_id (if a term has a single parent). But I don't see how that causes the need for all these other method additions and API changes.


Overall patch review:

  1. AFAICT #47 was never addressed.
  2. +++ b/core/modules/taxonomy/src/Entity/Term.php
    @@ -234,4 +242,58 @@ public function getVocabularyId() {
    +    // Cannot use $this->get('parent')->referencedEntities() here because that
    +    // strips out the '0' reference.
    

    Why does it strip out '0' references?

  3. +++ b/core/modules/taxonomy/src/Entity/Term.php
    @@ -234,4 +242,58 @@ public function getVocabularyId() {
    +      if ($item->target_id == 0) {
    

    Let's use strict equality.

  4. +++ b/core/modules/taxonomy/src/Entity/Term.php
    @@ -234,4 +242,58 @@ public function getVocabularyId() {
    +  public function getAncestors() {
    +    if (!isset($this->ancestors)) {
    +      $this->ancestors = [$this->id() => $this];
    +      $search[] = $this->id();
    +
    +      while ($tid = array_shift($search)) {
    +        foreach (static::load($tid)->getParents() as $id => $parent) {
    +          if ($parent && !isset($this->ancestors[$id])) {
    +            $this->ancestors[$id] = $parent;
    +            $search[] = $id;
    +          }
    +        }
    +      }
    +    }
    +    return $this->ancestors;
    +  }
    

    I'd like to see explicit test coverage for this in for vocabularies that have "multiple parents" enabled.

    Because AFAICT the current logic will return A, B, C, D, E in case you have these two lineages:

    D is child of B is child of A
    D is child of C is child of A
    

    in other words: there is no way to restore the lineage.

    I guess that perhaps that's the point?

    Should we then also add \Drupal\taxonomy\TermHierarchyInterface::getLineages()?

  5. +++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
    @@ -172,4 +172,24 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
    +   * Gets top-level term IDs of vocabularies.
    

    Why the term "top-level" and not "root"? Is ther e a precedent?

  6. +++ b/core/modules/taxonomy/src/TermStorageSchema.php
    @@ -22,36 +22,6 @@ protected function getEntitySchema(ContentEntityTypeInterface $entity_type, $res
    -    $schema['taxonomy_term_hierarchy'] = array(
    

    Nice!

  7. +++ b/core/modules/taxonomy/src/Tests/Update/TaxonomyUpdateTest.php
    @@ -0,0 +1,148 @@
    + * Ensure that the taxonomy updates are running as expected.
    ...
    +class TaxonomyUpdateTest extends UpdatePathTestBase {
    

    AFAIK we have one of these tests per update path.

    So this should be renamed to something like TaxonomyTermParentUpdatePathTest or something like that.

  8. +++ b/core/modules/taxonomy/src/Tests/Update/TaxonomyUpdateTest.php
    @@ -0,0 +1,148 @@
    +  /**
    +   * Tests taxonomy term parents update.
    +   *
    +   * @see taxonomy_update_8201()
    +   * @see taxonomy_update_8202()
    +   * @see taxonomy_update_8203()
    +   */
    +  public function testTaxonomyUpdateParents() {
    

    I can confirm that in manual testing this also worked as expected! :)

  9. +++ b/core/modules/taxonomy/taxonomy.install
    @@ -0,0 +1,109 @@
    +/**
    + * Convert the custom taxonomy term hierarchy storage to a default storage.
    + */
    +function taxonomy_update_8201() {
    ...
    +/**
    + * Copy hierarchy from {taxonomy_term_hierarchy} to {taxonomy_term__parent}.
    + */
    +function taxonomy_update_8202(&$sandbox) {
    ...
    +/**
    + * Update views to use {taxonomy_term__parent} in relationships.
    + */
    +function taxonomy_update_8203() {
    

    Impressive!

  10. +++ b/core/modules/taxonomy/src/Entity/Term.php
    @@ -232,4 +240,64 @@ public function getVocabularyId() {
    +  public function getChildren() {
    +    $query = \Drupal::entityQuery('taxonomy_term')
    +      ->condition('parent', $this->id());
    +    return static::loadMultiple($query->execute());
    +  }
    

    This is just returning all children, in no particular order.

  11. +++ b/core/modules/taxonomy/src/TermStorage.php
    @@ -93,100 +69,46 @@ public function resetCache(array $ids = NULL) {
       public function loadChildren($tid, $vid = NULL) {
    -    if (!isset($this->children[$tid])) {
    -      $children = array();
    -      $query = $this->database->select('taxonomy_term_field_data', 't');
    -      $query->join('taxonomy_term_hierarchy', 'h', 'h.tid = t.tid');
    -      $query->addField('t', 'tid');
    -      $query->condition('h.parent', $tid);
    -      if ($vid) {
    -        $query->condition('t.vid', $vid);
    -      }
    -      $query->condition('t.default_langcode', 1);
    -      $query->addTag('taxonomy_term_access');
    -      $query->orderBy('t.weight');
    -      $query->orderBy('t.name');
    -      if ($ids = $query->execute()->fetchCol()) {
    -        $children = $this->loadMultiple($ids);
    -      }
    -      $this->children[$tid] = $children;
    -    }
    -    return $this->children[$tid];
    +    /** @var \Drupal\taxonomy\TermInterface $term */
    +    return (!empty($tid) && $term = $this->load($tid)) ? $term->getChildren() : [];
       }
    

    This used to return children in a particular order, but not anymore. AFAICT that's a BC break.

  12. +++ b/core/modules/taxonomy/src/Tests/TaxonomyQueryAlterTest.php
    @@ -32,7 +32,7 @@ public function testTaxonomyQueryAlter() {
    -    $terms[2]->parent = $terms[1]->id();
    +    $terms[2]->parent->setValue($terms[1]->id());
    

    This change is not actually necessary.

Wim Leers’s picture

The joint interdiff of #91 + #93 + #95 is what needs to be reviewed.

Here is that joint interdiff to make life easier for other reviewers.

Wim Leers’s picture

So what I'm proposing in #99 is:

  1. Make $term->parent work, by removing ->setCustomStorage(TRUE);
  2. remove all changes in TermStorage — it should have zero changes
  3. remove TermHierarchyInterface and thus also remove its changes from Term. Term should only have the necessary changes for point 1.
  4. keep the necessary changes in views

All the things in points 2 and 3 are huge scope creep. They may have made sense in Q3 2015, when D8 was not yet out. But now that D8 is out, this would all cause a huge BC break. With very little gain. If we want to to do those things, they should happen in a separate issue, as a feature request.

dawehner’s picture

At least for me using the actual APIs instead of calling out to the DB directly is a prove that things are actually working as expected. If we still call out to the DB, the DB could be still wrong.

Wim Leers’s picture

I think we should not call the DB for $term->parent, but we should for TermStorage::loadParents(). Because TermStorage::loadParents() calls the DB in HEAD, and changing that would break BC. Plus, changing that is not necessary to solve the problem described in the issue summary.

claudiu.cristea’s picture

@Wim Leers, yes, the patch was designed before 8 was out. I agree, that in this stage, we should restrict the changes to #101. Let's see if I understood well:

  1. Make $term->parent a regular entity reference.
  2. Keep the actual hierarchy mechanism in place (table {taxonomy_term_hierarchy} and storage methods), even deprecated, for BC reasons. That means the hierarchy will be redundant stored, but in sync: first as entity reference field storage and second in {taxonomy_term_hierarchy}.
  3. Let Views use the new storage. What about keeping BC here? What if some custom module extended Views handlers?

EDIT: Term::getParents(), Term::getAncestors() and Term::getChildren() were created to replace the corresponding storage methods because, by making $term->parent a regular entity reference field, these methods become agnostic of the storage being used.

claudiu.cristea’s picture

Wim Leers’s picture

#104: glad to hear you agree :)

Wim Leers’s picture

Title: Make $term->parent behave like any other entity reference field » [PP-1] Make $term->parent behave like any other entity reference field
Status: Needs work » Postponed

Sadly, #2846614: Incorrect field name is used in views integration for multi-value base fields has been blocking this for exactly a month now :( (See #96.) Updating status field to reflect the actual current state.

Grimreaper’s picture

khiminrm’s picture

@Grimreaper, thanks!
I've successfully applied your patch to 8.3.2.

voleger’s picture

Tried to update patch from #108 to fix broken handlers for relationships in views.
Updated taxonomy_update_8203().
Updated TermViewsData.

mikl’s picture

#107: Sorry to be a nag, but how long are we going to wait for #2846614: Incorrect field name is used in views integration for multi-value base fields? Views integration is nice, but this is a significant bug in Drupal’s core entities.

I’m currently forced to choose between having to add custom SQL queries to load this data for a template, or having to go live with a patch that alters the database structure. Either way is likely to break my site on upgrade.

Not getting this fixed for 8.4 would be a tragedy, in my opinion.

Wim Leers’s picture

Priority: Normal » Major

#111: I could not agree more.

khiminrm’s picture

Hi!

Please, could anyone say, will we have possibility to upgrade core in future after applying this patch on the project?

Or a way for fixing this issue could be dramatically changed and we will not have any chance to upgrade core without necessity to make a lot of work for moving back all changes in a database made by the patch?

And maybe someone can give advice how we can properly fix relations by parent in existing views for this moment using patch from https://www.drupal.org/node/2543726#comment-12031743

Thanks!

voleger’s picture

That patch fixes broken handlers.
And I want notice that ./core/modules/forum/tests/src/Functional/ForumTest.php:436 used query where taxonomy_term_hierarchy table is used. Is it correct to update that query using this patch?

voleger’s picture

Fix incorrect fields name for argument and sort handlers

voleger’s picture

If someone can review the latest patch?
I just checked that with this patch can successfully pass test Drupal\taxonomy\Tests\Views\TaxonomyRelationshipTest.
Patch works perfectly for me during the last week.

edwardaa’s picture

works for me!

just one code indention issue:

+  /**
+   * {@inheritdoc}
+   */
+  public function getParents() {
+    $parents = $ids = [];
+    // Cannot use $this->get('parent')->referencedEntities() here because that
+    // strips out the '0' reference.
+    foreach ($this->get('parent') as $item) {
+      if ($item->target_id == 0) {
+        // The <root> parent.
+        $parents[0] = NULL;
+        continue;
+      }
+    $ids[] = $item->target_id;
+    }

$ids[] needs to indent

voleger’s picture

rumblestrut’s picture

Works for me as well. Thanks for the fix!

voleger’s picture

Status: Postponed » Needs review

Let's try to test this one

voleger’s picture

damiankloip’s picture

@volegar there are no interdiffs between patches so we're not sure what you changed each time without manually diffing each patch :/

voleger’s picture

damiankloip’s picture

I am good with modifying the views data for now, but the views issue we were depending on should be mentioned where you have added those changes IMO, we can then update the code when it's actually fixed. I am not sure how this works with BC as this is now just working around the bug from that issue by forcing the field, but still with the incorrect field name ('parent').

voleger’s picture

So as I understand when https://www.drupal.org/node/2846614 will be fixed, this patch after reroll should pass all the tests.

voleger’s picture

Status: Needs work » Postponed
Wim Leers’s picture

See #2882717-12: EntityResourceTestBase: assert that PATCHed and POSTed entities contain the intended values$term->parent always being NULL is causing problems there too. That's adding a new @todo that we'll be able to fix in this issue :)

Wim Leers’s picture

AFAICT #1915056: Use entity reference for taxonomy parents is the root cause of this. It's not that issue's fault though; it the sensible thing for its scope. This is something that only a Serialization/REST API would uncover. So the problem is that Serialization/REST's test coverage was too thin.

dawehner’s picture

Title: [PP-1] Make $term->parent behave like any other entity reference field » Make $term->parent behave like any other entity reference field
Status: Postponed » Needs review

Back to normal mode

jibran’s picture

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

FredCorreia’s picture

Can we add conditions in different taxonomy_update_83** to not make some changes if they have already been done by installing the previous patch (like the 108) ?

Because these changes will return errors and fail the update process. For example changes in database done in taxonomy_update_8302.

mgalalm’s picture

mgalalm’s picture

tedbow’s picture

  1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
    index 0000000000..44a4e83033
    --- /dev/null
    
    --- /dev/null
    +++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php.orig
    

    There are a number of *.orig files that are probably just left over from using the patch command. They need to be removed.

  2. +++ b/core/modules/taxonomy/taxonomy.install
    @@ -0,0 +1,127 @@
    +    foreach ($view->get('display') as $display_id => &$display) {
    

    $display here is never used so definitely doesn't need to be by reference. Also could just use array_keys($view->('display')) in the foreach instead.

  3. +++ b/core/modules/taxonomy/taxonomy.install
    @@ -0,0 +1,127 @@
    +
    +
    +
    

    2 extra blank links. Only need 1 between paragraphs.

  4. +++ b/core/modules/taxonomy/tests/src/Functional/Update/TaxonomyUpdateTest.php
    @@ -0,0 +1,148 @@
    +    $tids = [0]; // The root tid.
    

    Comment should appear before statement. Never after.

tedbow’s picture

Status: Needs work » Needs review
FileSize
27.56 KB
46.66 KB

Fixed items in my review in #142.

Also fixed test failure in #141 which was problems in core/modules/taxonomy/tests/src/Functional/Update/TaxonomyUpdateTest.php
Incorrect namespace, and incorrect directories for requiring files.

Wim Leers’s picture

#130 and earlier said this was blocked on #2846614: Incorrect field name is used in views integration for multi-value base fields. @dawehner removed the [PP-1] prefix in #135. Perhaps that was accidental?

I'm wondering now why #143 is passing without #2846614: Incorrect field name is used in views integration for multi-value base fields?

FredCorreia’s picture

In file taxonomy.install (with last patch #143) :

/**
27	 * Copy hierarchy from {taxonomy_term_hierarchy} to {taxonomy_term__parent}.
28	 */
29	function taxonomy_update_8302(&$sandbox) {
30	  $database = \Drupal::database();
31	
32	  if (!isset($sandbox['current'])) {
33	    // Set batch ops sandbox.
34	    $sandbox['current'] = 0;
35	    $sandbox['max'] = $database->select('taxonomy_term_hierarchy')
36	      ->countQuery()
37	      ->execute()
38	      ->fetchField();
39	  }
40	
41	  // Save the hierarchy.
42	  $select = $database->select('taxonomy_term_hierarchy', 'h');
43	  $select->join('taxonomy_term_data', 'd', 'h.tid = d.tid');
44	  $hierarchy = $select
45	    ->fields('h', ['tid', 'parent'])
46	    ->fields('d', ['vid', 'langcode'])
47	    ->range($sandbox['current'], $sandbox['current'] + 100)
48	    ->execute()
49	    ->fetchAll();

We could test if the table 'taxonomy_term_hierarchy' exists to prevent any process on it and throw errors on some Drupal installation.

With a if ($database->schema()->tableExists('taxonomy_term_hierarchy')) before the line 32

idebr’s picture

Attached patch is rerolled against Drupal 8.3.x for those who wish to test the upgrade path against the current stable release. The 'do-not-test' suffix should tell the testbot to leave it alone.

tedbow’s picture

@FredCorreia re #145 I am not sure in what case taxonomy_term_hierarchy would not exist at that point.

The table will not be dropped until taxonomy_update_8304

re @Wim Leers' question in #144

I'm wondering now why #143 is passing without #2846614: Incorrect field name is used in views integration for multi-value base fields?

Looked into this and I think it is because in

In #96 @damiankloip said

Silly mistake. In theory this should bring it back to 2 fails, that would be fixed by #2846614: Incorrect field name is used in views integration for multi-value base fields

It did the only failing test was \Drupal\Tests\taxonomy\Functional\Views\TaxonomyRelationshipTest::testTaxonomyRelationships

This test failed again in #108

But then in #114 testTaxonomyRelationships() no longer failed.

It think this is because in #96 in \Drupal\taxonomy\TermViewsData::getViewsData() it had

+    $data['taxonomy_term__parent']['parent_target_id']['help'] = $this->t('The parent term of the term. This can produce duplicate entries if you are using a vocabulary that allows multiple parents.');
+    $data['taxonomy_term__parent']['parent_target_id']['argument']['id'] = 'taxonomy';

but then in #114 this had changed to

+    $data['taxonomy_term__parent']['parent']['help'] = $this->t('The parent term of the term. This can produce duplicate entries if you are using a vocabulary that allows multiple parents.');
+    $data['taxonomy_term__parent']['parent']['argument']['id'] = 'taxonomy';
+    $data['taxonomy_term__parent']['parent']['relationship']['field'] = 'parent_target_id';
+    $data['taxonomy_term__parent']['parent']['relationship']['label'] = $this->t('Parent');

Note the key change from 'parent_target_id' to 'parent'

Patches after that kept that change and testTaxonomyRelationships() has not failed since.

I am still try to get back into this issue but maybe someone else can figure out if we should change it back and postpone this issue on #2846614: Incorrect field name is used in views integration for multi-value base fields

Wim Leers’s picture

#147: are you saying that those changes in \Drupal\Tests\taxonomy\Functional\Views\TaxonomyRelationshipTest::testTaxonomyRelationships() would no longer be necessary if #2846614: Incorrect field name is used in views integration for multi-value base fields landed?

I think it'd be great to have @damiankloip comment here again, he knows best how this relates to #2846614.

damiankloip’s picture

I think some of the changes may still be needed here, we will have to reroll and remove the changes to the field naming in TermViewsData once #2846614: Incorrect field name is used in views integration for multi-value base fields has landed. I think the start of these changes were introduced in #110, and a few subsequent patches after that. It's a bit harder to track as not all files have interdiffs.

So based on recent comments in #2846614: Incorrect field name is used in views integration for multi-value base fields - It will get committed in the next few days, so we can rely on that on move forward here. Based on that short time frame, I won't bother marking this postponed again.

Wim Leers’s picture

Assigned: Unassigned » damiankloip
Status: Needs review » Needs work

#2846614: Incorrect field name is used in views integration for multi-value base fields just landed!

Now Damian can reroll this according to his comment in #149 and we can finally push this one to the finish line — it's been blocked on #2846614: Incorrect field name is used in views integration for multi-value base fields since #88 — January 25, 2017, or almost 9 months ago!

damiankloip’s picture

Status: Needs work » Needs review
FileSize
47.12 KB
4.08 KB

This rerolls to use the views data changes I originally made (#96 and before maybe). Let's see how we get on. The changes I've made to \Drupal\Tests\taxonomy\Functional\Views\TaxonomyRelationshipTest::testTaxonomyRelationships should make them pass now. We would need an update handle to reflect the changes needed in views.view.test_taxonomy_term_relationship.yml for example, but #2846614: Incorrect field name is used in views integration for multi-value base fields should have already taken care of that for us.

TL;DR is that we're replacing 'parent' in the views table data with the correctly named column of 'parent_target_id'.

Let's see how the rest of the tests go.

jibran’s picture

Issue tags: +Entity Field API, +Field API

Yay! #2846614: Incorrect field name is used in views integration for multi-value base fields is in.

  1. +++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
    @@ -128,4 +128,26 @@ public function defaultValuesFormSubmit(array $element, array &$form, FormStateI
    +  public function removeItemsByTargetId($target_id) {
    

    We need a subsystem maintainer review for this.

  2. +++ b/core/modules/forum/tests/src/Functional/ForumTest.php
    @@ -433,7 +433,7 @@ public function createForum($type, $parent = 0) {
    +    $parent_tid = db_query("SELECT t.parent_target_id FROM {taxonomy_term__parent} t WHERE t.entity_id = :tid", [':tid' => $tid])->fetchField();
    

    Can this be replaced by EFQ?

  3. +++ b/core/modules/taxonomy/taxonomy.install
    @@ -0,0 +1,125 @@
    +function taxonomy_update_8301() {
    ...
    +function taxonomy_update_8302(&$sandbox) {
    ...
    +function taxonomy_update_8303() {
    ...
    +function taxonomy_update_8304(&$sandbox) {
    
    +++ b/core/modules/taxonomy/tests/src/Functional/Update/TaxonomyUpdateTest.php
    @@ -0,0 +1,149 @@
    +   * @see taxonomy_update_8201()
    +   * @see taxonomy_update_8202()
    +   * @see taxonomy_update_8203()
    

    The schema version needs an update.

claudiu.cristea’s picture

I wonder why I was removed from credits here as I spent a lot of effort on this issue?

damiankloip’s picture

Well, it's not near being committed, so I wouldn't panic just yet. Can't you just check yourself again?

claudiu.cristea’s picture

I'm not in panic.

EDIT: Tried to check but it has no effect.

voleger’s picture

+++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
@@ -128,4 +128,26 @@ public function defaultValuesFormSubmit(array $element, array &$form, FormStateI
+  public function removeItemsByTargetId($target_id) {
+    if (isset($this->list)) {
+      foreach ($this->list as $delta => $item) {
+        if ($item->target_id == $target_id) {
+          $this->removeItem($delta);
+        }
+      }
+    }

We should be careful with removeItem method. It trigger reindex item deltas.
so it should be like :

public function removeItemsByTargetId($target_id) {
  if (isset($this->list)) {
    $bias = 0;
    foreach ($this->list as $delta => $item) {
      if ($item->target_id == $target_id) {
        $this->removeItem($delta - $bias++);
      }
    }
  }
Wim Leers’s picture

#151 Yay! The only failures are in TaxonomyParentUITest (with failure: "The handler for this item is broken or missing." not found), which is not being modified by this patch. But that test does use views.view.test_taxonomy_parent.yml, which is being modified by this patch. That test was added by #2207229: Attempt to use 'Parent Term' relationship results in broken/missing handler. I wonder if this issue solved the real root cause of the symptom that #2207229: Attempt to use 'Parent Term' relationship results in broken/missing handler fixed?

#153: added Needs subsystem maintainer review

@claudiu.cristea: only committers have the power to assign or unassign credit. I think it's merely a bug, perhaps because your oldest comments with patches date to the early days of the issue credit system. I'm sure that when this gets committed, you will get credit :)

damiankloip’s picture

Yeah, #2207229: Attempt to use 'Parent Term' relationship results in broken/missing handler is certainly related but I think it just fixed the 'fix' that we already had in place to fix the field data that we can remove here.

This test just need the same fix as the other test; fixing the field used. This should fix that.

@voleger and @jibran I'm not ignoring you guys, honestly :) I just want this passing again first so we know where we are.

damiankloip’s picture

Status: Needs work » Needs review
damiankloip’s picture

This updates the removeItemsByTargetId() method to use setValue() instead. That seems like a simpler way to go rather than use a $bias variable to tweak the the delta as we go. This seems a bit easier to read? Really good catch on that though!

Updated the update functions too. Renamed to 850_* also added the field rename to the update function too. Not sure the updates from the views issue will tackle this as we're moving from a totally different table, not just broken views data.

RE #153.2: I am not sure we want to be converting that here? It seems to be intentionally not using the entity system. Might be just legacy, not sure. If that were the case, we wouldn't need an EFQ, it would just be the value from the loaded term entity I think?

damiankloip’s picture

Status: Needs work » Needs review
FileSize
47.98 KB
941 bytes

That fails because setValue() is not expecting actual items to be passed in. Maybe this instead... (Remove them from list directly, then rekey once after).

voleger’s picture

I gues using array_reverse() can protect from removing item by incorect delta.

public function removeItemsByTargetId($target_id) {
  if (isset($this->list)) {
    foreach (array_reverse($this->list) as $delta => $item) {
      if ($item->target_id == $target_id) {
        $this->removeItem($delta);
      }
    }
  }
}
Wim Leers’s picture

#153.2: updating a test from using a DB query to an entity query, a test that was written in 2008, that seems out of scope. I'd suggest creating a new modernize ForumTest: use entity queries instead of DB queries issue.

Good to see all other feedback being addressed!

Wim Leers’s picture

  1. I manually tested this patch. Before applying, I ensured I had a node with two tags (foo and bar, term IDs 1 and 2). After applying the patch and running the update path, I added another tag (baz, term ID 3). This uncovered a bug: only terms created after this patch is applied get the necessary entries in the taxonomy_term__parent table!

    Then this is what you get for foo:

    json
    …
      "parent": [],
    …
    
    hal_json
    no mention of parent at all!

    Yet for baz:

    json
    …
      "parent": [
        {
          "target_id": 0
        }
      ],
    …
    
    hal_json
    …
      "parent": [
        {
          "target_id": 0
        }
      ],
    …
    

    Worse, this impacts more than just the REST API responses, it also affects the UI: when you want to specify a parent term, the only terms you get to choose from are the ones with entries in the taxonomy_term__parent table. Therefore, foo and bar are not available as choices:

    So as far as I can tell, the update path is incomplete, and so are the update path tests. Sorry 😔

  2. index 73a1549e0a..27ef029ce2 100644
    --- a/core/modules/hal/tests/src/Functional/EntityResource/Term/TermHalJsonAnonTest.php
    +++ b/core/modules/hal/tests/src/Functional/EntityResource/Term/TermHalJsonAnonTest.php
    @@ -46,6 +46,11 @@ protected function getExpectedNormalizedEntity() {
               'href' => $this->baseUrl . '/rest/type/taxonomy_term/camelids',
             ],
           ],
    +      // Terms with no parents are mandatory children of <root>.
    +      // @see \Drupal\taxonomy\Entity\Term::preSave().
    +      'parent' => [
    +        ['target_id' => 0],
    +      ],
         ];
    

    This should not be necessary, because \Drupal\Tests\rest\Functional\EntityResource\Term\TermResourceTestBase::getExpectedNormalizedEntity() already contains this.

    Which means there's a bug in \Drupal\Tests\hal\Functional\EntityResource\HalEntityNormalizationTrait::applyHalFieldNormalization(), because it's removing this field, even though it should not.

    I stepped through it with a debugger, and the root cause lies here in \Drupal\hal\Normalizer\EntityReferenceItemNormalizer::normalize():

        /** @var $field_item \Drupal\Core\Field\FieldItemInterface */
        $target_entity = $field_item->get('entity')->getValue();
    
        // If this is not a content entity, let the parent implementation handle it,
        // only content entities are supported as embedded resources.
        if (!($target_entity instanceof FieldableEntityInterface)) {
          return parent::normalize($field_item, $format, $context);
        }
    

    $target_entity === NULL, which means the body of the if-statement is executed, which is why the parent field is handled as if it were not an entity reference field: because it's referencing a non-existing entity!

    This means that in theory, a term with a parent that is not <root> (i.e. non-existing term zero), should actually work. So building upon the example for review point 1, I added another term, qux (term ID 4), with baz (term ID 3) as its parent. And yes, sure enough, this is now the output:

    json
    …
      "parent": [
        {
          "target_id": 3,
          "target_type": "taxonomy_term",
          "target_uuid": "72ab7a5c-dd28-4dd5-8afc-33a301a43fb0",
          "url": "/taxonomy/term/3"
        }
      ],
    …
    
    hal_json
    {
      "_links": {
    …
        "http://d8/rest/relation/taxonomy_term/tags/parent": [
          {
            "href": "http://d8/taxonomy/term/3?_format=hal_json"
          }
        ]
      },
    …
      "_embedded": {
        "http://d8/rest/relation/taxonomy_term/tags/parent": [
          {
            "_links": {
              "self": {
                "href": "http://d8/taxonomy/term/3?_format=hal_json"
              },
              "type": {
                "href": "http://d8/rest/type/taxonomy_term/tags"
              }
            },
            "uuid": [
              {
                "value": "72ab7a5c-dd28-4dd5-8afc-33a301a43fb0"
              }
            ]
          }
        ]
      },
    …
    

    which is what I had expected all along.

    We'll probably want to add a testGetTermWithParent() to \Drupal\Tests\rest\Functional\EntityResource\Term\TermResourceTestBase to ensure proper test coverage/prevent this from regressing.

  3. +++ b/core/modules/taxonomy/src/Entity/Term.php
    @@ -52,10 +53,17 @@
    +   * @var array
    

    Nit: this should be \Drupal\taxonomy\TermInterface[]?

  4. +++ b/core/modules/taxonomy/src/Entity/Term.php
    @@ -88,14 +100,11 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
    +    // Terms with no parents are mandatory children of <root>.
    ...
    +      $this->parent->target_id = 0;
    
    @@ -248,4 +256,64 @@ protected function getFieldsToSkipFromTranslationChangesCheck() {
    +      if ($item->target_id == 0) {
    
    +++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
    @@ -173,4 +173,24 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
    +      ->condition('parent.target_id', 0)
    
    +++ b/core/modules/taxonomy/src/TermHierarchyInterface.php
    @@ -0,0 +1,38 @@
    +   *   <root> parent, that item is keyed with 0 and will have NULL as value.
    
    +++ b/core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateTaxonomyTermTest.php
    @@ -92,7 +92,7 @@ public function testTaxonomyTerms() {
    +        $this->assertSame((int) $term->parent->target_id, 0);
    

    Should we create a TermInterface::ROOT_ID = 0 constant?

  5. +++ b/core/modules/taxonomy/src/TermHierarchyInterface.php
    @@ -0,0 +1,38 @@
    +   *   The parent taxonomy term entities keyed by term id. If this term has a
    ...
    +   *   A list of ancestor taxonomy term entities keyed by term id.
    ...
    +   *   A list of children taxonomy term entities keyed by term id.
    

    Nit: s/id/ID/

  6. +++ b/core/modules/taxonomy/taxonomy.install
    @@ -0,0 +1,143 @@
    + * @addtogroup updates-8.3.x
    ...
    + * @} End of "addtogroup updates-8.3.x".
    

    Nit: 8.5.x.

  7. +++ b/core/modules/taxonomy/tests/src/Functional/Update/TaxonomyUpdateTest.php
    @@ -0,0 +1,149 @@
    +   * @see taxonomy_update_8201()
    +   * @see taxonomy_update_8202()
    +   * @see taxonomy_update_8203()
    

    Nit: 8501, 8502, 8503 by now… We've been at this for a while now.

Only point 1 and 2 are real concerns, all others are nits. 🎉 Point 1 is pretty deep, but hopefully is a simple, small oversight. Point 2 likely requires changing \Drupal\hal\Normalizer\EntityReferenceItemNormalizer::normalize(). I'll be looking into that more, and hopefully we can spin that off into its own issue.

voleger’s picture

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
8.17 KB
55.7 KB

Whew 😓, many hours later, I have a solution for point 2 of #168!

I think point 1 is best addressed by @damiankloip.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
930 bytes
55.71 KB

D'oh, some last-minute refactoring and not re-running all tests let a silly tiny thing slip through…

jibran’s picture

+++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
@@ -92,6 +93,31 @@ public function normalize($field_item, $format = NULL, array $context = []) {
+      return $target_entity_type_id === $referencing_entity->getEntityTypeId() && $referencing_entity instanceof FieldableEntityInterface;

Why do we need the first part of the statement? I think all we need is $target_entity_type_id is fieldable.

xjm’s picture

Updated issue crediting.

jibran’s picture

Wim Leers’s picture

@jibran++ 👏

That leaves only #167.1 (which hopefully damiankloip can address) and #172.


#172: but an entity type ID is just a string, you need an actual class or object to be able to check that it's implementing a certain interface. That's why I added this @todo in #171:

+++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
@@ -92,6 +93,31 @@ public function normalize($field_item, $format = NULL, array $context = []) {
+   * @todo Add logic for the case where the target entity type is not the same
+   *   as the referencing entity's entity type. This will require looking up
+   *   that entity type's class via EntityTypeManager. Issue: <to be created>

.

Wim Leers’s picture

Assigned: damiankloip » Wim Leers

Just talked to @damiankloip — he's going to work on fixing the gap in the update path described in #167.1. But I'll first tackle the update path test coverage. IOW: I'll make the patch fail, he'll make the patch pass :)

yched’s picture

Hi there :-)

Just a note about the removeItemsByTargetId() / "removeItem() reindexes deltas" thing mentioned in #157 and following :
digging back in the code around #2392845: Add a trait to standardize handling of computed item lists reminded me that we added a \Drupal\Core\TypedData\Plugin\DataType\ItemList::filter($callback) helper for exactly that kind of cases :-)

Wim Leers’s picture

Thanks, @yched! ❤️👌

Wim Leers’s picture

FileSize
54.69 KB

First, a reroll. This conflicted with #2909183: Add path alias (PathItem) field PATCH test coverage. Rebased, solved a trivial conflict.

Stay tuned for that failing update path test coverage — but I won't be able to finish it today.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Issue tags: -Needs tests, -Needs update path tests

Guess what!? I can no longer reproduce the problems I reported in #167.1! I carefully started with a fresh D8, created terms, then applied the patch, ran the update path and … everything worked just fine!

So, yay, there's nothing to be done in terms of update path, it works perfectly, and it actually is already testing all of this!

The environment I previously did the testing in had lots of cruft, that's probably the reason! So we can start the final rounds of review? :)

In looking at the update path test coverage, I found these nits though:

  1. +++ b/core/modules/taxonomy/tests/src/Functional/Update/TaxonomyUpdateTest.php
    @@ -0,0 +1,150 @@
    +class TaxonomyUpdateTest extends UpdatePathTestBase {
    

    Nit: should be renamed to TermParentUpdatetest, "taxonomy" is too broad.

  2. +++ b/core/modules/taxonomy/tests/src/Functional/Update/TaxonomyUpdateTest.php
    @@ -0,0 +1,150 @@
    +  public function setDatabaseDumpFiles() {
    +    $this->databaseDumpFiles = [
    +      __DIR__ . '/../../../../../system/tests/fixtures/update/drupal-8-rc1.bare.standard.php.gz',
    +    ];
    +  }
    ...
    +    $tids = $this->createTerms();
    ...
    +  protected function createTerms() {
    

    This is actually already trying to set up a site to test things like #167.1. But it's doing it in a way that's unlike every other UpdatePathTestBase test.

    The best practice is to either

    1. have database dump files to import in setDatabaseDumpFiles()
    2. or to have a separate PHP file that is passed in setDatabaseDumpFiles(), which is then executed there (see core/modules/rest/tests/fixtures/update/drupal-8.rest-rest_post_update_resource_granularity.php for an example)
claudiu.cristea’s picture

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
    @@ -128,4 +128,34 @@ public function defaultValuesFormSubmit(array $element, array &$form, FormStateI
    +      foreach ($this->list as $delta => $item) {
    +        if ($item->target_id == $target_id) {
    +          unset($this->list[$delta]);
    +        }
    +      }
    

    Is there a reason this is not an array_filter?

  2. +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
    @@ -92,6 +93,31 @@ public function normalize($field_item, $format = NULL, array $context = []) {
    +  protected static function targetEntityIsContentEntity(EntityReferenceItem $ref) {
    

    Is there a reason we call this method content entity even we deal with fieldable here?

  3. +++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
    @@ -173,4 +174,24 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
    +   *
    +   * @todo Add this to \Drupal\taxonomy\VocabularyInterface in Drupal 9.0.x.
    +   *   https://www.drupal.org/node/2785681
    +   */
    

    It feels like this belongs more on the term storage as it returns terms

  4. +++ b/core/modules/taxonomy/taxonomy.install
    @@ -0,0 +1,134 @@
    +function taxonomy_update_8504(&$sandbox) {
    +  $database = \Drupal::database();
    +  // Drop the legacy table.
    +  $database->schema()->dropTable('taxonomy_term_hierarchy');
    +  return t('Taxonomy term legacy tables dropped.');
    +}
    

    From a BC point of view, couldn't it be the case that someone still accesses the data directly? Maybe in order to not cause exceptions it might be worth dropping the table in D9 or so?

yched’s picture

re #182.1
see #177 - removeItemsByTargetId() should just be :

public function removeItemsByTargetId($target_id) {
  return $this->filter(function ($item) use ($target_id) {
    return $item->target_id != $target_id;
  });
}

ItemList::filter() takes care of the gotchas around the fact that an unset($items[$delta]) rekeys the subsequent items.

Wim Leers’s picture

Assigned: Unassigned » damiankloip
Status: Needs review » Needs work
Wim Leers’s picture

This blocks JSON API and GraphQL. And it of course blocks you from building any app that needs access to the parent term. Adding issue tags.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
54.31 KB
14.35 KB

#177: Nice! Thanks @yched. That's exactly what I needed. Can't believe I haven't seen that method on there before!

#180.1: Agreed - renamed.
#180.2: Yes, it's a bit odd (not really less odd than the pattern we use elsewhere but that's another story). Changed to bring in line with the other update tests. The only thing is the tids have now been hard coded in the test. I think this is why it was a method on the test before - to collect the tids that were inserted diretly into the database. Similar with the view ID - there isn't really a need for it to

#182.1: That is fixed by ^^
#182.2: Hmm. This is a good question. The comments are all pointing to content entities but the previous code was also just checking for fieldable.. So we either change the comments or the interface that's being used? I know the plan was always that hal just embedded content entities.
#182.3: I think we can/should just remove that. VocabularyStorageInterface already has this method, it's just been copied to another place. I removed the other usages and just used the
#182.4: Hmm. It's a good question. We would just have a table of stale data though? So if queries were still using it, it wouldn't do much anyway. I'm fine to keep it in, do we support people just querying stuff outside of the API?

TL;DR is we need to decide what to do in the hal \Drupal\hal\Normalizer\EntityReferenceItemNormalizer::normalize and whether to keep the legacy table or not.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
54.32 KB
730 bytes

Let's try that again with the correct variable.

damiankloip’s picture

Sorry, back on a working install. Actually testing the test now!

Wim Leers’s picture

Actually, this is 8.5-only, because #2846614: Incorrect field name is used in views integration for multi-value base fields was committed to 8.5 only.

Therefore #190 is actually green!

damiankloip’s picture

I think based on https://www.drupal.org/core/d8-bc-policy we should remove the schema. We would have stale data in it pretty quickly, unless we kept them in sync. Seems like people should be using the API here instead. @dawehner do you have any other cases where that is not possible. Seems like a semi generic devils advocate example :P

Hmm, looking into stuff related to @dawehner's other point in #182.2. It seems like #2275659: Separate FieldableEntityInterface out of ContentEntityInterface changed this behaviour (in error IMO), but didn't update the comment anyway :/ so I guess we have to go with the fieldable behaviour now either way.

Wim Leers’s picture

Agreed with #192 regarding https://www.drupal.org/core/d8-bc-policy#schema and the taxonomy_term_hierarchy table quickly becoming stale, therefore returning incorrect results anyway.

#182.2: calling the method "fieldable entity" or "content entity": we've been equating "content entity" with "implements fieldable entity interface" for a long time. See \Drupal\Core\Entity\KeyValueStore\KeyValueEntityStorage::doCreate() and editor_entity_insert() for example. Also, it was a pre-existing problem in this very code even, it was just moved into a new helper method!
Anyway, #192 made the change you asked for. I don't feel strongly about it either way. It's just a protected helper method.

Quoting #186:

TL;DR is we need to decide what to do in the hal \Drupal\hal\Normalizer\EntityReferenceItemNormalizer::normalize and whether to keep the legacy table or not.

These are now done.

Time for final review!

  1. +++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
    @@ -128,4 +128,21 @@ public function defaultValuesFormSubmit(array $element, array &$form, FormStateI
    +   * Removes the items with a specific target id.
    

    Nit: s/id/ID/

  2. +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
    @@ -91,6 +92,31 @@ public function normalize($field_item, $format = NULL, array $context = []) {
    +   *   TRUE when the referenced entity is of a content entity type.
    

    Nit: s/content/fieldable/

  3. +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
    @@ -91,6 +92,31 @@ public function normalize($field_item, $format = NULL, array $context = []) {
    +   *   that entity type's class via EntityTypeManager. Issue: <to be created>
    

    we'll need this issue created now.

  4. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Term/TermHalJsonAnonTest.php
    @@ -45,6 +52,32 @@ protected function getExpectedNormalizedEntity() {
    +          $parent_term_id === 0
    ...
    +          $parent_term_id === 0
    
    +++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
    @@ -113,7 +119,18 @@ protected function getExpectedNormalizedEntity() {
    +        $parent_term_id === 0
    ...
    +            'target_id' => 0,
    

    Nit: These can now be updated to TermInterface::ROOT_ID.

  5. +++ b/core/modules/system/tests/fixtures/update/drupal-8.views-taxonomy-parent-2543726.php
    @@ -0,0 +1,62 @@
    +  // This reads as: tid with index 1 has tids with index 2 and 3 as parents.
    

    That doesn't sound right. I think this was meant to read:

    Term with tid 1 has terms with tids 2 and 3 as parents
    
  6. +++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
    @@ -4,6 +4,7 @@
    +use Drupal\taxonomy\TermInterface;
    

    Nit: Only change in this file, can be reverted.

damiankloip’s picture

Cool - thanks Wim! I'll address this feedback shortly.

damiankloip’s picture

ok, I addressed all that feedback. Some good catches in there! One thing; instead of creating a new issue (that could potentially block this as its missing functionality?) I went ahead and implemented the class checking for different entity types in targetEntityIsFieldable and removed the todo.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

instead of creating a new issue (that could potentially block this as its missing functionality?) I went ahead and implemented the class checking for different entity types

Even better!

I think this is finally ready :) 🎉

amateescu’s picture

Assigned: damiankloip » amateescu

I'd like to take look at this patch.. I'll try this weekend so I don't keep it blocked much longer.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
@@ -128,4 +128,21 @@ public function defaultValuesFormSubmit(array $element, array &$form, FormStateI
+  /**
+   * Removes the items with a specific target ID.
+   *
+   * @param int $target_id
+   *   The target id of the item to be removed.
+   *
+   * @return $this
+   *
+   * @todo Add this to EntityReferenceFieldItemListInterface in Drupal 9.0.x.
+   *   https://www.drupal.org/node/2785673
+   */
+  public function removeItemsByTargetId($target_id) {
+    return $this->filter(function ($item) use ($target_id) {
+      return $item->target_id != $target_id;
+    });
+  }
+
 }

It would be nice to have its own test for this, \Drupal\KernelTests\Core\Entity\EntityReferenceFieldTest would be the place for that.

damiankloip’s picture

Yeah, that's a good idea. I'll add that.

damiankloip’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
58.12 KB
2 KB

Here is a test for that. I am just about to go out, so I haven't tested this yet! I'll fix it later if it's failing.

amateescu’s picture

Assigned: amateescu » Unassigned
Status: Needs review » Needs work

Ok, here's my review. I'm a bit sad to say that the patch still needs quite some work if we want to keep it aligned with the goals of the Workflow Initiative.

However, the basic functionality needed for the API-First Initiative could be implemented *a lot* easier by making the 'parent' field a computed one, use the trait from #2392845: Add a trait to standardize handling of computed item lists and simply implement the computeValue() of that trait to ensure that $term->parent returns the correct value instead of NULL. I think this approach could be tried in a new issue so we don't ruin the scope of this one.

  1. +++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
    @@ -128,4 +128,21 @@ public function defaultValuesFormSubmit(array $element, array &$form, FormStateI
    +   * @param int $target_id
    

    $target_id can also be a string :)

  2. +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
    @@ -91,6 +103,39 @@ public function normalize($field_item, $format = NULL, array $context = []) {
    +   * @param \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem $ref
    ...
    +  protected function targetEntityIsFieldable(EntityReferenceItem $ref) {
    ...
    +    $referencing_entity = $ref->getEntity();
    +    $target_entity_type_id = $ref->getFieldDefinition()->getSetting('target_type');
    

    $ref doesn't really describe this argument, it's a field item so let's call it like it is: $item :)

  3. +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
    @@ -91,6 +103,39 @@ public function normalize($field_item, $format = NULL, array $context = []) {
    +    return in_array(FieldableEntityInterface::class, class_implements($target_entity_type_class), TRUE);
    

    We can use is_a() here:

    return is_a($target_entity_type_class, FieldableEntityInterface::class, TRUE);

  4. +++ b/core/modules/taxonomy/src/TermHierarchyInterface.php
    @@ -0,0 +1,39 @@
    +interface TermHierarchyInterface {
    ...
    +  public function getParents();
    ...
    +  public function getAncestors();
    ...
    +  public function getChildren();
    

    One of the goals of the Workflow Initiative is to add a revision_parent field that will be used to generate revision trees, and (at my request) it will be implemented as a generic hierarchy framework on top of entity reference fields, which is basically what we had in D7 with the Tree module.

    That generic hierarchy framework will apply to every tree-like structure in Drupal (taxonomy terms, menu links, books), and the basic API that I have in mind for it is to have the methods that interact with the tree at the field item list level, not at the entity level.

    For example, getting the ancestors would be a getAncestors() call on the 'parent' field: $term->get('parent')->getAncestors().

    Anyway, where I'm getting with this long review point is that I would prefer to not introduce this TermHierarchyInterface here because it kind of goes against the plan outlined above.

  5. +++ b/core/modules/taxonomy/src/Entity/Term.php
    @@ -88,14 +100,11 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
    +    // Terms with no parents are mandatory children of <root>.
    +    if (!$this->get('parent')->count()) {
    +      $this->parent->target_id = static::ROOT_ID;
         }
    
    +++ b/core/modules/taxonomy/src/TermInterface.php
    @@ -7,9 +7,17 @@
    +  /**
    +   * Root ID for terms with no parent.
    +   */
    +  const ROOT_ID = 0;
    
    +++ b/core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateTaxonomyTermTest.php
    @@ -92,7 +93,7 @@ public function testTaxonomyTerms() {
    -        $this->assertNull($term->parent->target_id);
    +        $this->assertSame((int) $term->parent->target_id, TermInterface::ROOT_ID);
    

    In the Tree module, roots are stored with a NULL value for target_id, and I would like to keep that concept here for the same reason mentioned above.

  6. +++ b/core/modules/taxonomy/taxonomy.install
    @@ -0,0 +1,134 @@
    +  // Update the entity type because a new interface was added to Term entity.
    +  $manager->updateEntityType($manager->getEntityType('taxonomy_term'));
    

    If we remove the interface, this entity type update should not be needed anymore. Although I wonder why is it needed in the first place.

  7. +++ b/core/modules/taxonomy/taxonomy.install
    @@ -0,0 +1,134 @@
    +function taxonomy_update_8502(&$sandbox) {
    

    This whole update function looks like it could use a simple INSERT FROM SELECT query, which is very fast and doesn't need any batching.

  8. +++ b/core/modules/taxonomy/taxonomy.install
    @@ -0,0 +1,134 @@
    +  return t('Taxonomy term hierarchy NOT FULLY converted.');
    

    This is a strange message to return from an update function. It will be displayed in the final screen after running the updates and it looks like there was some error during the conversion process, especially because there's no definite 'success' message returned by the next two update functions.

  9. +++ b/core/modules/taxonomy/taxonomy.install
    @@ -0,0 +1,134 @@
    + * Drop the legacy table {taxonomy_term_hierarchy}.
    + */
    +function taxonomy_update_8504(&$sandbox) {
    +  $database = \Drupal::database();
    +  // Drop the legacy table.
    +  $database->schema()->dropTable('taxonomy_term_hierarchy');
    +  return t('Taxonomy term legacy tables dropped.');
    +}
    

    Is there any reason why the table can not be dropped in taxonomy_update_8502()?

  10. +++ b/core/modules/taxonomy/tests/src/Functional/TaxonomyQueryAlterTest.php
    @@ -32,7 +32,7 @@ public function testTaxonomyQueryAlter() {
    -    $terms[2]->parent = $terms[1]->id();
    +    $terms[2]->parent->setValue($terms[1]->id());
    

    Does the previous syntax not work anymore? I'm reasonably sure that it should still work, otherwise this is a BC break..

  11. +++ b/core/modules/taxonomy/tests/src/Functional/TaxonomyQueryAlterTest.php
    @@ -50,12 +50,12 @@ public function testTaxonomyQueryAlter() {
    -    $this->assertQueryTagTestResult(2, 1, 'TermStorage::loadParents()');
    +    $this->assertQueryTagTestResult(3, 1, 'TermStorage::loadParents()');
    ...
    -    $this->assertQueryTagTestResult(2, 1, 'TermStorage::loadChildren()');
    +    $this->assertQueryTagTestResult(3, 1, 'TermStorage::loadChildren()');
    

    Are these changes fixing a bug, or why do we need to change the assertions?

amateescu’s picture

damiankloip’s picture

Status: Needs work » Needs review
FileSize
57.66 KB
3.89 KB

Thanks for looking amateescu! Hopefully we can align this with what everyone needs...

Isn't #2921048: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix just directly conflicting with this issue? I get it's simplifying things slightly, it's not really fixing the underlying issue though. It's just pushing it further down the road. We could have fixed this at the field level or normalizer level in the mean time but they would all just be temporary solutions, pretty much.

I have implemented a few of the smaller points from #201 1,2,3,6. Will have to look at the update based feedback. I didn't write those originally. Seems like there might be a small amount of logic for the insert data.

201.4: Hmm, then we would have to wait for this idea you have that doesn't actually exist yet?! :) Not sure we want to do that. It seems ok to add this additionally but then internally depend on your generic hierarchy work internally when it exists (this would essentially be a wrapper around it then)? In D9 what we have here can be removed in favour of your field level API stuff only.

201.5: using NULL as a way to signify the root term seems like an odd choice, is there a reason for that? Generally NULL as a first class citizen is kind of weird. Plus 0 denotes the top level root now anyway.

201.11: Please see https://www.drupal.org/node/2543726#comment-11918427 (Comment #99). That summarises stuff me and Wim talked about quite well IIRC.

Needs review for now to test the patch.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
57.81 KB
611 bytes
amateescu’s picture

Isn't #2921048: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix just directly conflicting with this issue?

Not at all :) I'll answer in detail below, I just wanted to be clear that there's no conflict between this issue and the other one.

I get it's simplifying things slightly, it's not really fixing the underlying issue though. It's just pushing it further down the road. We could have fixed this at the field level or normalizer level in the mean time but they would all just be temporary solutions, pretty much.

I guess that depends on what you consider the "underlying issue" is.

In HEAD, the 'parent' field is a field that uses a custom storage and there's nothing conceptually wrong with that. The actual bug is that, just like computed fields, custom storage fields need to behave like regular fields and return the correct value(s) when requested via API calls (i.e. $term->parent->target_id).

#2921048: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix is about fixing the real bug in 8.4.x (the fix being ~5 lines of code) at the field level, while this issue is actually about making the 'parent' field not use a custom storage anymore, which is a task that can be fixed at any point in the future, and the final patch here will just need to remove the custom field item list class added in #2921048: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix because it won't be needed anymore.

jibran’s picture

#2921048: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix is about fixing the real bug in 8.4.x

So can someone explain now what is the usecase of this conversation now and please update the issue summary as well?

Wim Leers’s picture

#206: Thanks for that explanation, that helps a lot! ❤️ Also, thank you very much for doing a review, and such an in-depth one at that! 👌

Review posted at #2921048-12: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix — I found one significant problem, which you probably should be able to address. If you can, then I think you're right that #2921048 could land first, and then this issue could land later, to improve the way term parents are stored. Thanks to the test coverage that was developed in this issue, we can know for a fact that there won't be a BC break: if the REST test coverage in both this issue's patch and #2921048's are identical, then we can be absolutely certain! But right now, the test coverage doesn't match, and that's precisely the significant problem I found :) But again, I'm pretty confident you can address that!

I have a call with @damiankloip in ~1.5 hours, I will discuss it with them, to make sure I didn't overlook something.

EDIT: https://twitter.com/wimleers/status/927532217370333184 :)

damiankloip’s picture

In my mind, I would prefer to go down this patch route, as we have a whole fix here. This also formalises the way the parent field works with views, as it's just another ER field. I am totally open to the computed field work in your patch too though - it looks nice and simple. I just kept away from adding a computed before in quest of the 'proper' fix. If that's the best way to keep everyone happy, I am ok with that too.

If the main reason is a clash of interfaces, I think we could solve that. I mentioned briefly above; if the TermHierarchyInterface was a wrapper around any new work, when it exists. We could replace the internal code with the new hierarchy system. Then remove this in D9, leaving just the generic solution.

And yes, the summary needs an update :)

Wim Leers’s picture

Title: Make $term->parent behave like any other entity reference field » Make $term->parent behave like any other entity reference field, to fix REST support and de-customize its Views integration
Issue summary: View changes
Issue tags: -Needs issue summary update
Related issues: +#2921377: When Drupal 8.6.0 is released, remove TermEntityNormalizer and tag a new release

I had a call with Damian. I think there are 3 things to discuss/clear up:

  1. I guess that depends on what you consider the "underlying issue" is.

    You're absolutely right — we've been saying for ages that this is about API-First, but it's about much more: the scope is much broader than just API-First. The scope in the current patch includes fixing Views integration (rather than lots of custom stuff, use the same well-tested mechanisms/code paths as entity reference fields), which is also why this was blocked on #2846614: Incorrect field name is used in views integration for multi-value base fields for a very long time. This badly needs an issue summary update, as @jibran indicated in #207.

    So, to answer your question: this issue's scope is about making $term->parent behave like any other entity reference field, with two goals:

    1. fix REST support
    2. de-customize its Views integration

    Updated issue title + summary.

  2. For the REST half, I agree that #2921048: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix is a viable solution (as I already said in #208).

    For the Views half, we need the storage changes that this patch is making, where it's moving us away from custom storage. @amateescu, do you agree that this de-customizing of storage is something that is A) desired, B) would be implemented the same in the future you describe in #201.4?

  3. I understand that from your perspective (and desire) to introduce a TreeInterface, the introduction of TermHierarchyInterface is A) an unnecessary intermediate step, B) an annoyance. But … we have plenty of BC layers. Any reason why we can't just deprecate TermHierarchyInterface and keep a BC layer around? That BC layer would only exist for Term entities. In fact, even in the current patch, it's effectively @deprecated: there's a @todo to merge it with TermInterface in D9. That would then just change/be removed once your Tree plan works out.

    Damian has been working on this since January. We're finally super close to the finish line. I very very much like the https://www.drupal.org/project/tree idea (I remember discussing it with you in Leuven in 2015 or so), but I don't think this moves you further away from that necessarily. I think it only means one more BC layer.

    Furthermore, I think there's an opportunity to have even fewer BC worries. The current patch does this:

    @@ -34,6 +42,9 @@ public function updateTermHierarchy(EntityInterface $term);
        *
        * @return \Drupal\taxonomy\TermInterface[]
        *   An array of term objects which are the parents of the term $tid.
    +   *
    +   * @deprecated in Drupal 8.3.x, will be removed in Drupal 9.0.0. Use
    +   *   \Drupal\taxonomy\Entity\Term::load($tid)->getParents() instead.
        */
       public function loadParents($tid);
     
    @@ -45,6 +56,9 @@ public function loadParents($tid);
        *
        * @return \Drupal\taxonomy\TermInterface[]
        *   An array of term objects which are the ancestors of the term $tid.
    +   *
    +   * @deprecated in Drupal 8.3.x, will be removed in Drupal 9.0.0. Use
    +   *   \Drupal\taxonomy\Entity\Term::load($tid)->getAncestors() instead.
        */
       public function loadAllParents($tid);
     
    @@ -58,6 +72,9 @@ public function loadAllParents($tid);
        *
        * @return \Drupal\taxonomy\TermInterface[]
        *   An array of term objects that are the children of the term $tid.
    +   *
    +   * @deprecated in Drupal 8.3.x, will be removed in Drupal 9.0.0. Use
    +   *   \Drupal\taxonomy\Entity\Term::load($tid)->getChildren($vid) instead.
        */
       public function loadChildren($tid, $vid = NULL);
     

    We could choose to not do that, and keep those APIs as the official ones, and instead mark TermHierarchyInterface and its implementations as @internal. That way, the public API would remain unchanged compared to HEAD, but we'd at least internally be using this improved way of doing things. Then when the Tree vision materializes, there would truly be zero extra BC layers.

Thoughts?

jibran’s picture

Issue tags: +Performance

Thanks @Wim Leers and @damiankloip for updating the IS.

+++ b/core/modules/taxonomy/src/TermStorage.php
@@ -2,35 +2,14 @@
 class TermStorage extends SqlContentEntityStorage implements TermStorageInterface {

I think we should also do some performance testing for the changes in TermStorage class.

amateescu’s picture

@Wim Leers, re #210:

@amateescu, do you agree that this de-customizing of storage is something that is A) desired, B) would be implemented the same in the future you describe in #201.4?

A) Absolutely! and B) Yes :)

This patch in its current form (well, except for the new interface) is pretty much critical for the "great tree structure unification" described in #201.

Like I said in #206, the reason for getting #2921048: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix in first is that 1) it fixes the API-First bug now (in Drupal 8.4.x), not in 4-5 months (Drupal 8.5.x), and 2) the only additional "work" for this patch would be to remove the custom field item list class added by that issue.

@damiankloip, re #209 and @Wim Leers, re #210.3:

If the main reason is a clash of interfaces, I think we could solve that. I mentioned briefly above; if the TermHierarchyInterface was a wrapper around any new work, when it exists. We could replace the internal code with the new hierarchy system. Then remove this in D9, leaving just the generic solution.

I understand that from your perspective (and desire) to introduce a TreeInterface, the introduction of TermHierarchyInterface is A) an unnecessary intermediate step, B) an annoyance.

Actually, it's a bit more than an unnecessary intermediate step and/or a simple annoyance. I tried to explain the problem in #201 but maybe I couldn't get the point across very well so I'll try again.

The TermHierarchyInterface added by this patch forces the Term entity to be part of a single hierarchy, meaning that the methods for getting the parents, ancestors, etc. are at the entity level. The Workflow Initiative wants to introduce the concept that an entity will be part of multiple hierarchies, the second one being a revision hierarchy, for example.

Here's a code explanation which might make things more clear.

API provided by the current patch:

$term->getParents();
$term->getAncestors();

Desired API:

$term->getTree()->getParents();
$term->getTree()->getAncestors();
$term->getRevisionTree()->getParent();
$term->getRevisionTree()->getAncestors();

Where $term->getTree()->... is simply a shortcut for $term->get('parent')->... and $term->getRevisionTree()->... is a shortcut for $term->get('revision_parent')->....

This is why the distinction between the hierarchy being at the entity level vs. field level is very important to the future of the Workflow Initiative.

Wim Leers’s picture

Thanks for expanding your explanation! I only have one remaining question:

The TermHierarchyInterface added by this patch forces the Term entity to be part of a single hierarchy, meaning that the methods for getting the parents, ancestors, etc. are at the entity level.

This is your concern. But:

Desired API:

$term->getTree()->getParents();
$term->getTree()->getAncestors();
$term->getRevisionTree()->getParent();
$term->getRevisionTree()->getAncestors();

Where $term->getTree()->... is simply a shortcut for $term->get('parent')->... […]

Why couldn't $term->getParents() (from the TermHierarchyInterface) then not also be another shortcut for $term->get('parent'), and therefore a deprecated synonym for $term->getTree()->getParents()?

amateescu’s picture

@Wim Leers, you highlighted the answer to that question in the first qoute (my concern).. :)

Wim Leers’s picture

#214: I don't think that highlighted part answers it. getParents() would just become a synonym for the "default tree" (getTree()->getParents()). I don't see what the problem is with that.

Wim Leers’s picture

Per #2921048-13: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix, where @amateescu said:

However, after writing the two paragraphs above, I just realized that there is a flaw in my explanation, which is the multi-parent architecture of taxonomy terms. […] Or for a REST GET request to know whether a term with a parent of Y is also a root term.

Adding explicit test coverage for that, to prove that that is supported in this patch. Good call, @amateescu! 👍

Wim Leers’s picture

Assigned: Unassigned » amateescu

Still looking for your feedback, @amateescu.

Until #200, we thought this was ready. The review you posted in #201 was invaluable! 👍❤️

@damiankloip then addressed everything, except for point 4.

I tried to explain the different POVs and bring them together in #210. But now with #215 and #216, this is AFAICT blocked on you clarifying your feedback/concerns/proposals. Let me know if there's anything I can do to help (perhaps I can review some other issues to help you find the time for this one)?

amateescu’s picture

Assigned: amateescu » Unassigned

getParents() would just become a synonym for the "default tree" (getTree()->getParents()). I don't see what the problem is with that.

The problem, as I see it, is that we would introduce some methods which will be 1) incomplete, compared to list of methods from the TreeInterface and 2) deprecated basically from the start. Also, some review points from #201 were not addressed, the ones about the upgrade path IIRC.

So my proposal is still the same: get #2921048: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix in to fix the REST problem and then work on this patch as a followup when we settle everything with the new generic interface for hierarchies.

dawehner’s picture

Status: Needs review » Needs work

Settings needs work for the review in #201

@amateescu
While I totally agree that a potential tree would be super nice, I don't think we should block this issue from going on forward:

  • Code is never perfect, you want to iterate, and along the process collect experience. For example we might detect a performance problem earlier than later by having it available on terms
  • The potential tree interface will need time. Coding is never the hard part, getting it reviewed, evaluated, simply takes at least till 8.6
  • There is more value than just REST to this fix. Having a proper entity reference helps all over the place.
Wim Leers’s picture

So #201.6, #201.7, #201.8 and #201.9. IOW: the taxonomy.install changes.

@damiankloip did address #201.6. But 7+8+9 indeed still need to be addressed.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
17:54:27 <WimLeers> amateescu: dawehner  posted a comment/review at https://www.drupal.org/project/drupal/issues/2543726#comment-12364494, you probably hadn't seen that one yet :)
17:55:27 <amateescu> WimLeers: I saw it.. but how many times I can say that I disagree with something? :)
17:58:50 <WimLeers> amateescu: so you also disagree with what dawehner wrote?
17:59:05 <WimLeers> amateescu: and yes, I realize that that's what you've had to do, sorry :(
17:59:37 <amateescu> WimLeers: really, it's the same thing that I disagree with, introducing those methods on term interface in that patch
17:59:43 <WimLeers> amateescu: assuming you indeed disagree with dawehner, that means you disagree with dawehner, damiankloip and I.
17:59:45 <WimLeers> amateescu: ok
17:59:50 <dawehner> amateescu: The problem is that every discussion will always have arguments on both sides :)
18:00:21 <WimLeers> amateescu: So why is it not ok to deprecate the methods this patch would introduce once your (AWESOME!) TreeInterface lands?
18:00:43 <amateescu> WimLeers: dawehner: as the current scope of the issue is 1) fix REST, 2) de-customize views integration, those methods are not critical to any of those two points
18:01:10 <WimLeers> amateescu: that's not true?
18:01:23 <amateescu> WimLeers: how come?
18:01:31 <dawehner> WimLeers: amateescu could we move those methods somewhere else "temporarily"?
18:01:34 <WimLeers> amateescu: those methods are a necessary byproduct of making Term->parent not use custom storage IIRC
18:01:44 <dawehner> So at least the entityinterface is not touched?
18:01:59 <dawehner> Something like TermTreeNode or so?
18:02:06 <amateescu> they could be @internal methods on the Term entity
18:02:12 <amateescu> and not on the interface
18:02:32 <dawehner> amateescu: well, then you kinda end up not allowing anything to call them which would be a bit weird?
18:03:02 <amateescu> dawehner: that's exactly the point: they would be only for internal usage for the moment
18:03:12 <WimLeers> They're not on TermInterface, they're on a new TermHierarchyInterface, which we could deprecate
18:03:23 <WimLeers> Why couldn't $term->getParents() (from the TermHierarchyInterface) then not also be another shortcut for $term->get('parent'), and therefore a deprecated synonym for  $term->getTree()->getParents()?
18:04:01 <dawehner> WimLeers: oh right good point
18:04:08 <amateescu> WimLeers: I already answered this a couple of times, because it forces the single-hierarchy thinking about the entity type
18:04:30 <amateescu> WimLeers: when we want to get to a point where any entity type can be a part of multiple hierarchies
18:04:39 <dawehner> amateescu: fair, this is why I wanted to add it as a separate thing
18:04:43 <amateescu> for example a revision hierarchy
18:04:48 <WimLeers> amateescu: but that _is_ today's reality. I see your point though. I struggle with something similar: the lack of API-First thinking
18:05:31 <amateescu> WimLeers: it's the reality in the term storage, I simply object to bringing it to the term entity itself
18:06:09 <WimLeers> Hypothetical idea (not sure if it works): make all those new methods @internal, don't deprecate the existing methods that the current patch is deprecating, and don't introduce a new interface. Then we're effectively keeping the _exact_ same API, we're _just_ fixing the underlying storage
18:06:24 <WimLeers> amateescu: ^^ — would that work for you?
18:06:43 <amateescu> WimLeers: that's all I'm asking for..
18:07:23 <WimLeers> I forgot many of the details about the patch because it's been stuck for so long, I'm not at all sure that that is possible!
18:07:32 <WimLeers> But okay, let's see if that's possible
18:07:37 <amateescu> WimLeers: like I said earlier, moving those methods outside the storage does not help with neither fixing REST nor with views de-customization
18:08:12 <amateescu> it's just "clean up" for the storage itself
18:08:30 <amateescu> which could be done later..
18:13:11 <dawehner> WimLeers: so this means contrib should not call those new methods? I mean it works for me, but people will still do
18:13:22 <WimLeers> dawehner: amateescu I just tried to remove TermHierarchyInterface
18:13:44 <dawehner> amateescu: so you propose to keep the methods on the storage and "just" rewrite them to use the entity reference?
18:13:49 <WimLeers> dawehner: amateescu the problem becomes that TermStorage::loadParents() _needs_ to retrieve the parents somehow from the Term, and it has no way to do so, without TermHierarchyInterface::getParents()
18:13:52 <amateescu> dawehner: yup
18:13:55 <WimLeers> ok
18:14:05 <WimLeers> so use $term->get('parent') instead of $term->getParents() ?
18:14:13 <amateescu> WimLeers: yup :)
18:14:26 ⇐ joachim quit (~joachim@cpc105064-sgyl40-2-0-cust61.18-2.cable.virginm.net) Quit: off to have cake, probably
18:14:28 <WimLeers> dawehner: do you see obstacles to that?
18:14:33 <WimLeers> dawehner: you know this stuff better than I do
18:14:57 <WimLeers> or better yet, amateescu, if you feel like the solution is so obvious, why don't you roll a quick patch? :P
18:16:25 <amateescu> WimLeers: I guess I have to..
18:16:41 <amateescu> WimLeers: the thing is that I'm neck deep in trying to get workspaces to a core-ready state
18:16:44 <WimLeers> amateescu: I'd love to give reviews in exchange :D
18:16:53 <amateescu> and already very much stressed out by that
18:16:56 <WimLeers> amateescu: yep, everybody is neck-deep!
18:17:11 <WimLeers> amateescu: I've been helping timmillwood with the REST/normalization bits of workspaces
18:17:15 <WimLeers> amateescu: happy to help more
18:17:30 <amateescu> sad but true :)
18:17:45 <WimLeers> amateescu: I'm happy to try to roll an initial patch — you can then improve on it. Would that work better?
18:17:47 <amateescu> I know, thanks for that
18:18:02 <dawehner> amateescu: I might try to roll a patch.
18:18:19 <amateescu> WimLeers: not sure, but I will take care of it first thing tomorrow morning
18:18:28 <amateescu> whatever "it" will be at that time :)
18:18:49 <amateescu> either review something from you or daniel, or roll it mylself
18:18:55 <amateescu> *myself
18:19:26 <amateescu> I have to go for dinner and spending some time with the little one this evening :)
18:19:26 ⇐ reaper013 quit (52e0b0a3@gateway/web/freenode/ip.82.224.176.163) Quit: Page closed
18:20:00 <WimLeers> amateescu++
18:20:02 <WimLeers> amateescu: <3
18:20:04 <WimLeers> amateescu: hf!
18:20:18 <amateescu> WimLeers++
18:20:20 <amateescu> dawehner++
18:20:27 <amateescu> thanks for understanding my POV
18:21:24 <dawehner> amateescu: thank you for insisting on it
18:21:24 <dawehner> amateescu: have a nice evening!

Trying this.

Wim Leers’s picture

First, straight rebase, plus keep tests green, because #2800873: Add XML GET REST test coverage, work around XML encoder quirks automatically changes the normalization of every entity type, which means we need to update our expectations. (That is being fixed in #2928702: Make EntityResourceTestBase's field_rest_test_multivalue test field less invasive: omit it from normalizations. Once that lands, we'll be able to revert this interdiff.)

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
FileSize
10.4 KB
62.98 KB

I followed a simple strategy:

  1. move Term::(getParents|getAncestors|getChildren) to TermStorage
    1. make it protected
    2. make all of them receive a TermInterface $term parameter
    3. replace $this with $term
    4. mark it @internal
    5. add a @todo which will be resolved by @amateescu's future TreeInterface issue
  2. remove TermHierarchyInterface

This means the APIs remain 100% identical to today, and the logic remains 100% identical to @damiankloip's patch.

dawehner’s picture

Just some super quick scan.

  1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
    @@ -90,6 +91,66 @@ protected function createEntity() {
    +    $parent_term_ids = [];
    +    for ($i = 0; $i < $this->entity->get('parent')->count(); $i++) {
    +      $parent_term_ids[$i] = (int) $this->entity->get('parent')[$i]->target_id;
    +    }
    

    Couldn't we replace that with loadAllParents ?

  2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
    @@ -230,4 +291,64 @@ public function testPatchPath() {
    +    Term::create(['vid' => Vocabulary::load('camelids')->id()])
    ...
    +    Term::create(['vid' => Vocabulary::load('camelids')->id()])
    

    Vocabularies are config entities, the ID is well defined upfront :)

  3. +++ b/core/modules/taxonomy/src/TermStorage.php
    @@ -93,100 +77,126 @@ public function resetCache(array $ids = NULL) {
    +      foreach ($term->getParents() as $id => $parent) {
    

    This looks like it shouldn't work

Wim Leers’s picture

  1. Maybe. I didn't try. I kept changes as small as possible. Feel free to simplify this patch further, now that we're not adding new APIs
  2. lol :P That should certainly be simplified. That's not new in #223 though.
  3. Also not new in #223.
dawehner’s picture

Also not new in #223.

Well, by not changing it you'll cause a fatal

dawehner’s picture

Status: Needs work » Needs review
FileSize
60.28 KB
671 bytes

Let's see what this fix results into :)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
701 bytes
62.99 KB

#228 was indeed an oversight, here's another.

dawehner’s picture

This should fix the remaining failure.

caseylau’s picture

These line caused the patch can't be applied by composer-patches

So I apply the patch, then use "git diff" to recreate the patch, No functional change.
I'm sorry, this is our own composer-patch bug, not this issue ignore this patch

caseylau’s picture

Oh, interdiff-2543726-232-234.patch should be rename to interdiff-2543726-232-234.txt,ignore it

Wim Leers’s picture

The last green patch is #216, from November 10.

#2800873: Add XML GET REST test coverage, work around XML encoder quirks landed on November 24, after #216, and added XML test coverage. This explains the remaining new failures: they'd also have occurred for #216.

This small change to XmlEntityNormalizationQuirksTrait makes the TermResourceTestBase::testGetTermWithParent() tests pass when using the XML format.

Wim Leers’s picture

FileSize
12.8 KB

I created a mess in #223. I did follow a simple strategy, but I made many small mistakes, which resulted in these ~15 comments that gradually got the patch closer to green. Sorry about that :(

Here's an interdiff showing all the differences between #216 and #237. That should make it much easier for @damiankloip to review this when he gets back next week.

The interdiff shows two kinds of changes:

  1. (most changes) implementing the consensus strategy we found in #221
  2. (few changes) updates to keep tests passing now that #2800873: Add XML GET REST test coverage, work around XML encoder quirks landed
Wim Leers’s picture

Assigned: Unassigned » damiankloip

#201 points 7+8+9 still need to be addressed. Assigning to @damiankloip for that.

amateescu’s picture

The interdiff from #238 looks very good! I have to re-read the whole patch again in depth but here's a minor point for now:

  1. +++ b/core/modules/taxonomy/src/TermStorage.php
    @@ -351,7 +363,7 @@ public function getNodeTerms(array $nids, array $vocabs = [], $langcode = NULL)
    -    unset($vars['parents'], $vars['parentsAll'], $vars['children'], $vars['treeChildren'], $vars['treeParents'], $vars['treeTerms'], $vars['trees']);
    +    unset($vars['treeChildren'], $vars['treeParents'], $vars['treeTerms'], $vars['trees']);
    

    parents, parentsAll and children are gone, but we have a new ancestors member that we need to unset.

  2. +++ b/core/modules/taxonomy/src/TermStorage.php
    @@ -361,9 +373,6 @@ public function __sleep() {
         // Initialize static caches.
    -    $this->parents = [];
    -    $this->parentsAll = [];
    -    $this->children = [];
    

    And initialize here :)

Wim Leers’s picture

#241: excellent nits!

caseylau’s picture

@Wim Leers

  1. 2543726-237.patch can't be applied to drupal8.4.x.
  2. Drupal8.5.x with 2543726-237.patch and commerce module installed, enable hal and rest, The whole site crash.
    ArgumentCountError: Too few arguments to function Drupal\hal\Normalizer\EntityReferenceItemNormalizer::__construct(), 2 passed in /var/www/d85/core/lib/Drupal/Component/DependencyInjection/Container.php on line 266 and exactly 3 expected in Drupal\hal\Normalizer\EntityReferenceItemNormalizer->__construct() (line 55 of /var/www/d85/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php) #0 /var/www/d85/core/lib/Drupal/Component/DependencyInjection/Container.php(266): Drupal\hal\Normalizer\EntityReferenceItemNormalizer->__construct(Object(Drupal\hal\LinkManager\LinkManager), Object(Drupal\serialization\EntityResolver\ChainEntityResolver)) #1 /var/www/d85/core/lib/Drupal/Component/DependencyInjection/Container.php(171): Drupal\Component\DependencyInjection\Container->createService(Array, 'serializer.norm...') #2 /var/www/d85/core/lib/Drupal/Component/DependencyInjection/Container.php(480): Drupal\Component\DependencyInjection\Container->get('serializer.norm...', 1) #3 /var/www/d85/core/lib/Drupal/Component/DependencyInjection/Container.php(508): Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array) #4 /var/www/d85/core/lib/Drupal/Component/DependencyInjection/Container.php(230): Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array) #5 /var/www/d85/core/lib/Drupal/Component/DependencyInjection/Container.php(171): Drupal\Component\DependencyInjection\Container->createService(Array, 'serializer') #6 /var/www/d85/core/lib/Drupal/Component/DependencyInjection/Container.php(480): Drupal\Component\DependencyInjection\Container->get('serializer', 1) #7 /var/www/d85/core/lib/Drupal/Component/DependencyInjection/Container.php(230): Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array) #8 /var/www/d85/core/lib/Drupal/Component/DependencyInjection/Container.php(171): Drupal\Component\DependencyInjection\Container->createService(Array, 'rest.resource_r...') #9 /var/www/d85/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(105): Drupal\Component\DependencyInjection\Container->get('rest.resource_r...') #10 /var/www/d85/vendor/symfony/http-kernel/HttpKernel.php(193): Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.response', Object(Symfony\Component\HttpKernel\Event\FilterResponseEvent)) #11 /var/www/d85/vendor/symfony/http-kernel/HttpKernel.php(260): Symfony\Component\HttpKernel\HttpKernel->filterResponse(Object(Symfony\Component\HttpFoundation\Response), Object(Symfony\Component\HttpFoundation\Request), 1) #12 /var/www/d85/vendor/symfony/http-kernel/HttpKernel.php(79): Symfony\Component\HttpKernel\HttpKernel->handleException(Object(Symfony\Component\HttpKernel\Exception\NotFoundHttpException), Object(Symfony\Component\HttpFoundation\Request), 1) #13 /var/www/d85/core/lib/Drupal/Core/StackMiddleware/Session.php(57): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #14 /var/www/d85/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(47): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #15 /var/www/d85/core/modules/page_cache/src/StackMiddleware/PageCache.php(184): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #16 /var/www/d85/core/modules/page_cache/src/StackMiddleware/PageCache.php(121): Drupal\page_cache\StackMiddleware\PageCache->fetch(Object(Symfony\Component\HttpFoundation\Request), 1, true) #17 /var/www/d85/core/modules/page_cache/src/StackMiddleware/PageCache.php(75): Drupal\page_cache\StackMiddleware\PageCache->lookup(Object(Symfony\Component\HttpFoundation\Request), 1, true) #18 /var/www/d85/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(47): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #19 /var/www/d85/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(50): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #20 /var/www/d85/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #21 /var/www/d85/core/lib/Drupal/Core/DrupalKernel.php(657): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #22 /var/www/d85/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request)) #23 {main}.
  3. Drupal8.4.x with 2543726-236.patch get the same above error
Wim Leers’s picture

#243:

  1. This patch is not going to be committed to 8.4.x because it's too big of a change.
  2. Sounds like Drupal Commerce is decorating EntityReferenceItemNormalizer, in an incorrect way. Note that https://www.drupal.org/core/d8-bc-policy#constructors explicitly says that constructors A) aren't APIs, B) can change in minors.

    I investigated the commerce module, and they have zero normalizers. So this seems … impossible.

    It looks like you didn't rebuild the container after applying the patch?

  3. Again, it looks like you didn't rebuild the container after applying the patch?
Wim Leers’s picture

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
@@ -230,4 +291,64 @@ public function testPatchPath() {
+    $expected += [
+      'field_rest_test_multivalue' => [
+        0 => [
+          'value' => 'One',
+        ],
+        1 => [
+          'value' => 'Two',
+        ],
+      ]
+    ];

We can remove this thanks to #2928702: Make EntityResourceTestBase's field_rest_test_multivalue test field less invasive: omit it from normalizations having been committed. (This reverts the addition that #222 made to keep tests passing.)

caseylau’s picture

@Wim Leers

  1. Install drupal8.5.x and apply the #237 patch
  2. drush en hal rest -y
  3. composer require drupal/commerce:2.x-dev
  4. drush en commerce commerce_cart commerce_promotion commerce_checkout commerce_log commerce_order commerce_payment commerce_payment_example commerce_price commerce_product commerce_store commerce_tax -y
  5. drush cr

Then #243 error occurs.

The "drush cr" has rebuild the container.

Uninstall one of hal and rest, The error will gone away.

Wim Leers’s picture

@caseylau:

I followed the steps in #246 (albeit without using drush). I reproduced it.

The root cause is not Commerce, nor this patch. It's the entity_reference_revisions module's \Drupal\entity_reference_revisions\EntityReferenceRevisionsServiceProvider. That will need to be updated.
(And if that code used the service decorator pattern, it wouldn't even need to be updated. Look at the html_response.attachments_processor.big_pipe service for an example of that.)
Created #2931496: Add serializer for json/xml for this.

@caseylau's feedback is great, but is definitely not a blocker: he's just being very proactive (impressively so actually!), by testing a patch for the next minor release with his entire set of modules. In doing so, we found a problem in the entity_reference_revisions module that can easily be fixed, even before 8.5 is released.

Berdir’s picture

> And if that code used the service decorator pattern

We are not decorating an existig service, we're adding our own that extends one from core. So no, decorator would not help here, but I guess it would be possible to re-use the arguments that the core service for that class is using instead of adding them hardcoded, then this wouldn't happen.

Technically, __construct() is excluded from BC so it we are allowed to make that change. However, whenever we can, we usually add such arguments with = NULL and a fallback in the constructor, so that we do not break child classes.

Wim Leers’s picture

We are not decorating an existig service, we're adding our own that extends one from core. So no, decorator would not help here, but I guess it would be possible to re-use the arguments that the core service for that class is using instead of adding them hardcoded, then this wouldn't happen.

True in some cases, but not here. In this case, it's subclassing only to call parent::METHOD() in two places. It could easily be a pure decorator.
Darn, it's decorating protected function constructValue(), which of course is not possible to decorate because it's not public.

Of course, normalizers never were APIs at all (we're trying to fix that at #2920536: Force all serializer encoders + normalizers services to be private), but evidently some contrib modules started to use them as if they were APIs anyway.

So, changing it to an optional argument. Once again, not being clear about what our APIs are comes back to haunt us :(

Berdir’s picture

> Darn, it's decorating protected function constructValue(), which of course is not possible to decorate because it's not public.

Yes, but that's not even relevant. A decorator is about decorating an existing service, we define a new one, because this is for a different field type. Because you also overlooked the most important change in the subclass. The fact that we change "protected $supportedInterfaceOrClass".

As I wrote, even if it would be an API, the constructor is technically excluded. But it can already be quite painful to do minor updates, so lets try to be as nice as possible to contrib and custom modules :)

And \Drupal\hal\Normalizer\EntityReferenceItemNormalizer is actually a very important class, because there are many sub-field types of that, so it is only logical that those also need to support normalization and don't want to duplicate 120 lines of code. I'm seeing 4 subclasses in my development installation: file_entity, webform, entity_reference_revisions, dynamic_entity_reference. I also add this paragraph as a comment into the issue you referenced. Re-reading that issue, that is only about the services. That's actually a completely different aspect of API/Code-reuseability than what we are talking about here.

Wim Leers’s picture

Because you also overlooked the most important change in the subclass. The fact that we change "protected $supportedInterfaceOrClass".

You're right, I got the terminology wrong :) Decorating the existing service would work, but would effectively remove the existing service, i.e. no more normalizer for entity reference fields.
What I should have said, is inject the existing service, so that you can reuse it. But that would have failed due to the protected function constructValue(), as I said in #249.

Anyway! #249 addressed your feedback :)

Berdir’s picture

I would recommend to just add the fallback directly in the constructor, that's easier to remove than having to use an already deprecated method, see for example: \Drupal\Core\Entity\ContentEntityForm::__construct(). Then we can just remove the = NULL and the ?: ... in the 9.x branch, without having to update any calls.

Yeah, just making sure we understand each other :) Wouldn't "what I should have said, is inject the existing service, so that you can reuse it." then actually make that service an API again, which is kind of exactly what you want to prevent in the other issue? Yes it would still work to inject that into the service, but it still seems to be the opposite of what you suggested before, conceptually. :)

IMHO, especially that class is specifically *designed* to be subclassed and extended, with that protected constructValue() method that all 4 of the subclasses that I mentioned are overriding.

Wim Leers’s picture

I would recommend to just add the fallback directly in the constructor,

You're right once again. I did #249 the way I did it to prevent unnecessary construction of a service used only in an edge case, but once it's actually injected, it doesn't make a difference anyway. So +1.

Wouldn't "what I should have said, is inject the existing service, so that you can reuse it." then actually make that service an API again, which is kind of exactly what you want to prevent in the other issue?

No, because:

  1. today that contrib service's class is extending a core service's class that isn't an API and it's doing it anyway
  2. then injecting that core service that isn't an API into the contrib service still wouldn't make it an API

In either case, it's the decision of the contrib module that they choose to bear the burden of staying in sync with upstream changes of something that isn't an API.

The problem is that we never clearly communicated what are our APIs — which I talked about in https://wimleers.com/talk/bc-burden-benefit — which is why we are forced to deal with this BC stuff in this issue at all. In practice, everything that isn't explicitly marked as @internal could be seen as an API, and so effectively for ~90% of code we're forced to maintain BC despite it not being an API…

IMHO, especially that class is specifically *designed* to be subclassed and extended, with that protected constructValue() method that all 4 of the subclasses that I mentioned are overriding.

It's FieldItemNormalizer that's designed to be subclassed, not EntityReferenceItemNormalizer. Also: ::constructValue() was added in 2013, in #1880424: Handle entity references on import. Only much, much later did we start seeing the consequences (once again, see my talk). Back then, we were still creating interfaces for everything, turning everything into an API. It's also long before we decided on semver and keeping BC all the way through D9. It's before we had @internal even, I think.

Wim Leers’s picture

FileSize
1.93 KB
63.58 KB

This discussion is making this issue very hard to follow I think :P

I agree with you we shouldn't break BC. I addressed that in #249 (and refined it in the attached patch per your suggestion).

Fact is that normalizers should not be APIs; I think core's normalizers never were meant to be APIs, but maybe they were, but reality is that they are seen and used as such, and therefore we must maintain BC. This is part of a much bigger problem in Drupal: we don't explicitly mark APIs as APIs and everything else as internal implementation detail, and hence developers see almost everything as APIs. In no small part because that was pretty much true in the past. Fixing that is definitely out of scope for this issue. #2912642: Add @internal to automated tests classes is an example of a good step forward to fix that, and #2873395: [meta] Add @internal to core classes, methods, properties is the plan issue for that. Maybe we should move this discussion there? Or at least link to it from there?

damiankloip’s picture

This is part of a much bigger problem in Drupal: we don't explicitly mark APIs as APIs and everything else as internal implementation detail, and hence developers see almost everything as APIs

Nail on head! Yes. I have always said this, especially about normalizers. I have had this come up a few times, people have been using normalizers or encoders stand alone, I tell them the serializer instance is the API. Alas, we didn't have internal officially back then etc etc etc... :)

That diff in #254 looks ok to me - in the name of BC.

Wim Leers’s picture

Status: Needs review » Needs work

Great!

@damiankloip: it's now up to you to do the remaining things listed in #240:

#201 points 7+8+9 still need to be addressed. Assigning to @damiankloip for that.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
60.67 KB
1.12 KB

It's great to have some consensus and progress on this now.

RE #201: 8 & 9 I have done in this patch. I think I mentioned briefly before in an earlier comment, but I'm not sure this is entirely possible, as we need to create delta values for each entity field values:

  foreach ($hierarchy as $row) {
    if ($row->tid !== $tid) {
      $delta = 0;
      $tid = $row->tid;
    }

    $insert->values([
      'bundle' => $row->vid,
      'entity_id' => $row->tid,
      'revision_id' => $row->tid,
      'langcode' => $row->langcode,
      'delta' => $delta,
      'parent_target_id' => $row->parent,
    ]);

    $delta++;
    $sandbox['current']++;
  }

So I don't think we can nicely achieve the same thing with a single SQL query?

amateescu’s picture

Status: Needs review » Needs work

Thanks, @damiankloip, that looks a bit better :) As promised in #241, I've reviewed the entire patch again in depth, and here's what I found:

  1. +++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
    @@ -128,4 +128,21 @@ public function defaultValuesFormSubmit(array $element, array &$form, FormStateI
    +   *   The target id of the item to be removed.
    

    The target ID ... :)

  2. +++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
    @@ -128,4 +128,21 @@ public function defaultValuesFormSubmit(array $element, array &$form, FormStateI
    +  public function removeItemsByTargetId($target_id) {
    

    This won't play well with the Dynamic Entity Reference module from contrib, because it also has to know the target type for the IDs that should be removed.

    Currently, EntityReferenceFieldItemListInterface contains only one 'additional' method, referencedEntities() which can be easily implemented by DER, but the new method that we're adding here will be impossible for them to implement without a second argument. Since we're talking about only 3 lines of code, would it make more sense to simply inline that code where we currently need it and open a separate issue for adding this new method so it can be discussed and reviewed properly?

  3. +++ b/core/modules/taxonomy/src/TermInterface.php
    @@ -10,6 +10,11 @@
    +  const ROOT_ID = 0;
    

    This is the last architectural concern that I have with this patch.

    The fact that the root ID is 0 is simply an implementation detail of the current hierarchical storage for taxonomy term parents, and IMO it is not something that we want to expose to the outside world (i.e. via REST).

    Consider a scenario where someone adds a hierarchical field to user entities, for example to represent a family tree. In that hypothetical scenario, we won't be able to say that root = 0 because 0 is an actual entity in the system. So how do we explain that REST calls need to know about the special ID 0 for taxonomy terms, and a different special ID for users.

    At the very least, we should move this constant to the storage class, and my suggestion is to expose a proper constant (e.g. <root>) as the official API for REST calls that interact with taxonomy parents. This way, if we manage to re-architect and unify all the separate hierarchical implementations in core, we won't need to break the API or deal with ugly BC layers for REST. Let's be API-forward-thinking for a chance, not only API-first-focused-only-on-individual-issues :)

  4. +++ b/core/modules/taxonomy/taxonomy.install
    @@ -0,0 +1,127 @@
    +  // Update the entity type because a new interface was added to Term entity.
    +  $manager->updateEntityType($manager->getEntityType('taxonomy_term'));
    

    Since we're not adding the new interface anymore, I guess we can try to remove this again?

  5. +++ b/core/modules/taxonomy/tests/src/Functional/Update/TaxonomyParentUpdateTest.php
    @@ -0,0 +1,82 @@
    + * @group taxonomy
    

    We should also add a @group Update here.

  6. +++ b/core/modules/taxonomy/tests/src/Functional/Update/TaxonomyParentUpdateTest.php
    @@ -0,0 +1,82 @@
    +   * @see taxonomy_update_8504()
    

    This update function does not exist anymore.

  7. +++ b/core/modules/taxonomy/tests/src/Functional/Update/TaxonomyParentUpdateTest.php
    @@ -0,0 +1,82 @@
    +    $this->assertSame(count($term->parent), 2);
    ...
    +    $this->assertSame(count($term->parent), 2);
    ...
    +    $this->assertSame(count($term->parent), 1);
    

    We can use assertCount() here.

  8. +++ b/core/modules/taxonomy/tests/src/Functional/Update/TaxonomyParentUpdateTest.php
    @@ -0,0 +1,82 @@
    +    // Test if the view has been converted to use {taxonomy_term__parent} table
    

    ... to use *the* {taxonomy_term__parent} table ...

  9. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityReferenceFieldTest.php
    @@ -451,4 +451,49 @@ public function testEntityReferenceFieldDependencies() {
    +    $this->assertCount(2, $values);
    

    This assertion is not really useful since we're checking the entire array structure below.

Also, #241 still needs to be addressed.

jibran’s picture

#258.2 😍

damiankloip’s picture

Thanks @amateescu - great review!

RE #2: That's a good point, I have just inlined it. As we don't have the additional method now, I've removed the corresponding test method. We already have test coverage testing children get deleted (I think TermKernelTest has most of that).

RE #3: I would be keen to see what Wim's view is on this one. It's a really good point about a 0 ID having a different meaning between entity types. From my point of view a string constant would be strange, I would prefer to just use NULL in these cases personally. Having something like '<root>' would still need special handling for consumers I think?

The rest of the points I think I've addressed in this patch.

Wim Leers’s picture

+++ b/core/modules/taxonomy/taxonomy.install
@@ -14,8 +14,6 @@ function taxonomy_update_8501() {
-  // Update the entity type because a new interface was added to Term entity.
-  $manager->updateEntityType($manager->getEntityType('taxonomy_term'));

Looks like this should not have been removed.

amateescu’s picture

That's really weird, we should try to investigate why we need to update the entity type.

@Wim, this is still waiting for your feedback on #258.3 / #260.

SocialNicheGuru’s picture

just FYI .. this change breaks modules like Taxonomy Manager that rely on the the taxonomy_term_hierarchy table that is removed.

Wim Leers’s picture

@amateescu Oh, thanks, I didn't realize that!

  • #258.3: Excellent point about a hierarchy of users, and a User entity with ID zero existing!

    At the very least, we should move this constant to the storage class

    The danger with the value being different in-storage vs in-memory is that they need to be consistently translated, always. It's easier to have a single representation: less that can go wrong. What about NULL?

    my suggestion is to expose a proper constant (e.g. <root>) as the official API for REST calls that interact with taxonomy parents

    I like it! I like the idea conceptually, but I do think that <root> versus numeric IDs in all other cases is gonna be a bit strange. But my alternate proposal of using NULL supports that just fine. Many languages have the concept of a "nullable type". In this case, it's then a nullable integer (in case of numeric IDs) and a nullable string (in case of string IDs). "string or int" is more
    confusing. Plus, in theory, the string <root> could conflict with an existing string ID, whereas NULL definitely can't.

    This would actually be a trivial change for the test coverage:

    'target_id' => TermInterface::ROOT_ID,
    

    Today, that constant maps to 0, but now it'd change to NULL.

    And actually, for the "link" concept that the HAL normalization has, the link already is a NULL link in the current patch:

    +++ b/core/modules/hal/tests/src/Functional/EntityResource/Term/TermHalJsonAnonTest.php
    @@ -37,6 +39,114 @@ protected function getExpectedNormalizedEntity() {
    +      case [TermInterface::ROOT_ID, 2]:
    +        $expected_parent_normalization_links = [
    +          NULL,
    +          [
    +            'href' => $this->baseUrl . '/taxonomy/term/2?_format=hal_json',
    +          ],
    +        ];
    

    So it's just applying the idea that this patch is already doing more consistently.

    Let's be API-forward-thinking for a chance, not only API-first-focused-only-on-individual-issues :)

    +10000

  • #260: Oh … hah :P I should've read your comment first. I ended up agreeing with you obviously.

@amateescu: so what do you think about root ID = NULL?

damiankloip’s picture

Status: Needs work » Needs review
FileSize
57.8 KB
611 bytes

Adding back the entity type update code for now. I would like to know the reason this is still needed too. We've tried to remove it 3 times now though, so this issue maybe needs it either way :P

Wim Leers’s picture

+++ b/core/modules/taxonomy/taxonomy.install
@@ -14,6 +14,8 @@ function taxonomy_update_8501() {
+  // Update the entity type because a new interface was added to Term entity.

We'll need to change this comment now though.

+++ b/core/modules/taxonomy/src/Entity/Term.php
@@ -156,8 +159,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
-      ->setCardinality(BaseFieldDefinition::CARDINALITY_UNLIMITED)
-      ->setCustomStorage(TRUE);
+      ->setCardinality(BaseFieldDefinition::CARDINALITY_UNLIMITED);

Isn't this the reason we still need that line that #268 brought back?

damiankloip’s picture

Status: Needs work » Needs review
FileSize
57.97 KB
3.53 KB

Rerolled.

Wim Leers’s picture

Status: Needs review » Needs work

The #271 interdiff didn't make any sense. I manually diffed the #268 and #271 patches — they're identical, only context changes. IOW: straight rebase to make the patch apply cleanly to HEAD.

Back to NW for #258.3/#260/#267: we can't keep const ROOT_ID = 0;

damiankloip’s picture

Yes, sorry -that was just a diff of the fixed hunk from the reroll.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
54.29 KB
9.51 KB

Let's try this to start with. Not sure if/how many issues we will hit trying to have no values stored...

Wim Leers’s picture

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
@@ -334,15 +314,9 @@ public function testGetTermWithParent(array $parent_term_ids) {
   public function providerTestGetTermWithParent() {
     return [
-      'root parent: [0] (= no parent)' => [
-        [TermInterface::ROOT_ID]
-      ],
-      'non-root parent: [2]' => [
+      'one parent: [2]' => [
         [2]
       ],
-      'multiple parents: [0,2] (root + non-root parent)' => [
-        [TermInterface::ROOT_ID, 2]
-      ],
       'multiple parents: [3,2] (both non-root parents)' => [
         [3, 2]
       ],

Why these changes in #274? This is removing lots of test coverage. Or is this just to check that at least things still work when using non-root parents, and you'll restore this test coverage in a next patch?

damiankloip’s picture

I'm not sure, there is no parent anymore, so no parent data will exist if we going with this NULL idea. What do we test in that case? setting a NULL value does in fact lead to no value being set? That seems like something that is already tested with fields in general.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
62.14 KB
12.03 KB

Me and Wim had a call and decided trying to change the actual storage from 0 for root, to NULL (nothing persisted at the field level) is a much broader change than this issue should tackle. It opens a can of worms with how the UI handles 0 as the root value, amongst other things.

This is based on the patch from #271, It does:

- Removes ROOT_ID constant from TermInterface. I think @amateescu should be happier if we just don't have this? :)
- Adds term specific behaviour to entity reference normalization, so we will have NULL returned instead of 0 for the REST API.
- Adjust test coverage to use NULL for root parent instead of 0 (We'll see how testbot gets on).
- New unit test for EntityReferenceFieldItemNormalizer for the new term trait/behaviour.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
63.77 KB
3.4 KB
Wim Leers’s picture

  1. +++ b/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizerTrait.php
    @@ -0,0 +1,30 @@
    +  protected function normalizeRootReferenceValue(&$values, EntityReferenceItem $field_item) {
    

    Missing docblock.

  2. +++ b/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizerTrait.php
    @@ -0,0 +1,30 @@
    +    // @todo Generalize for all tree-structured entity types.
    

    This would refer to @amateescu's TreeInterface once it exists.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.75 KB
69.1 KB

Sooo … the failures are in Comment + Node tests for the hal_json format, specifically for updating fields.

This is related to #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior, which landed two days ago, because that patch removed the hal_json overrides for those tests of protected static $patchProtectedFieldNames. See comments 74 and 75 on that issue.

I did a very deep dive on debugging this. Turns out that those lines should not have been removed, and the changes in EntityReferenceItemNormalizer here happen to fix that. Specifically, \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::getModifiedEntityForPatchTesting() (which was added in that issue) sets a value of 99998 for entity reference fields. Turns out HAL's entity reference field normalizer is broken in HEAD, because it doesn't deal correctly with references pointing to non-existent entities. That bug in HEAD caused the revision_uid field for Node to end up in the hal_json serialization, but it shouldn't have been: that field should always have been removed in favor of a link in HAL's _links section.

This patch fixes that, and in doing so, is bringing back the discrepancy in PATCH field error checking, because this once again ensures that all reference fields in HAL normalizations live in _links.

Bringing back those deleted lines (reverting the #2824851-74: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior + #2824851-75: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior interdiffs) make tests pass, and per this deep debugging session, that's actually the correct behavior.

MAN this was hard to debug!

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

@amateescu's latest round of feedback from #258 has been addressed.

I don't want to hold this up for the tiny nit in #282.1. This should really land in 8.5, because it has been functionally ready for a long time now. 8.5 should not be yet another release in which you cannot access term parents via REST/JSON API/GraphQL.

I only fixed REST test nitpicks in #283 and helped out with tiny bits here and there. The real work has been done by others, primarily @damiankloip.

Let's get this in!

amateescu’s picture

Me and Wim had a call and decided trying to change the actual storage from 0 for root, to NULL (nothing persisted at the field level) is a much broader change than this issue should tackle. It opens a can of worms with how the UI handles 0 as the root value, amongst other things.

I've been thinking a lot about this in the last few days and I'm happy to say that I arrived at the same conclusion independently from your discussion with Wim :) Allowing the entity reference field storage to store NULL values for roots goes very much into the tree field territory, and should not be handled in here. I'm glad that it was so easy to expose the proper API for REST (root as NULL) while keeping the current term storage intact (root as 0).

#284 gives the feeling that we're in rush mode with this issue, so here's a patch which addresses my final remarks and a bunch of coding standards fixes.

Adding back the entity type update code for now. I would like to know the reason this is still needed too. We've tried to remove it 3 times now though, so this issue maybe needs it either way :P

I also debugged this and I found out why the entity type needs to be updated. Rewrote the comment above that line to say what's really going on.

Wim Leers’s picture

#284 gives the feeling that we're in rush mode with this issue, so here's a patch which addresses my final remarks and a bunch of coding standards fixes.

We are, so I very much appreciate your help in #285! ❤️ Those changes all look excellent!

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.75 KB
70.36 KB

Looks like #285 was rolled relative to #280 instead of #283.

Re-applying #283's interdiff.

Berdir’s picture

+++ b/core/modules/taxonomy/taxonomy.install
@@ -0,0 +1,129 @@
+
+  // Update the entity type because the 'taxonomy_term_hierarchy' table is no
+  // longer part of its shared tables schema.
+  $definition_update_manager->updateEntityType($definition_update_manager->getEntityType('taxonomy_term'));
...
+
+  // Save the hierarchy.
+  $select = $database->select('taxonomy_term_hierarchy', 'h');
+  $select->join('taxonomy_term_data', 'd', 'h.tid = d.tid');
+  $hierarchy = $select
+    ->fields('h', ['tid', 'parent'])

I had a quick look at what happens here exactly, because based on that comment, I would expect that it then *removes* that no longer existing table, but that doesn't happen here because otherwise 8502 would fail hard.

Discussed with @amateescu, what we think is happening is that it sees that something changed but because we don't explicitly check for custom additional tables in \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::requiresEntityDataMigration(), so we say removing a custom table is not a breaking change and we happily move on in \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::onEntityTypeUpdate().

Then we get to the next check, if there is data, we just re-generate the indexes. And we save the new generated entity schema, where now the table doesn't exist anymore, but it still exists as an actual table in the database.

But one thing where it gets interesting when there are no terms, because then it will go into the if where it removes *all* tables and re-creates them, so now the table is gone.

So if I understand this correctly, running this update on a site with no terms would fail? I thought we also have empty update tests? Maybe I'm missing something...

Anyway, we should possibly....

a) open an issue to figure out if \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::requiresEntityDataMigration() *should* care about custom additional tables somehow

b) Possibly move the entity type update into another update that runs *after* 8502, conceptually that makes more sense to me, first we migrate the data into the new table, then we tell the update manager that the old one now no longer exists?

amateescu’s picture

@Wim Leers, right, I forgot rebase my changes onto the latest patch :/

I looked a bit more into it after discussing with @Berdir and the reason why our custom table is not dropped when there are no existing terms is because \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::onEntityTypeDelete() removes only the *known* entity tables with this code:

    // Delete entity tables.
    foreach ($this->getEntitySchemaTables() as $table_name) {
      if ($schema_handler->tableExists($table_name)) {
        $schema_handler->dropTable($table_name);
      }
    }
  protected function getEntitySchemaTables() {
    return array_filter([
      'base_table' => $this->storage->getBaseTable(),
      'revision_table' => $this->storage->getRevisionTable(),
      'data_table' => $this->storage->getDataTable(),
      'revision_data_table' => $this->storage->getRevisionDataTable(),
    ]);
  }

In case we change that logic in the future, let's be safe here and move the entity type update right above the place where we delete the old table manually. This essentially implements #289.b), but without adding a new update function for it.

There's also a small change which I missed in #285, resetting the new $ancestors property in \Drupal\taxonomy\TermStorage::resetCache(), not only in \Drupal\taxonomy\TermStorage::__wakeup().

damiankloip’s picture

Thanks Andrei! This is great. The reasoning does make sense when you think about it... :)

+1 on your recent changes.

larowlan’s picture

The issue summary references #201.4 as TBD but it seems that has been resolved, so tagging for IS update.

Wim Leers’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

You're right. Updated IS.

Wim Leers’s picture

Also: this has been reviewed and RTBC'd by the Field API maintainer. I think that means we can remove the Needs subsystem maintainer review issue tag.

larowlan’s picture

I had some concerns about the scope here.

The issue is fixing the parent field to make it an ER field and fixing rest/serialization support, which feels like its doing two things.

I discussed it with @WimLeers and @amateescu on slack/irc.

Wim pointed out that

if we commit the "make normal field" patch first, then we'd be shipping in a state where the normalization behaves in a way that we aren't testing, and not entirely how we need it to (because it exercises hitherto unexercised edge cases) … which means we'd breaking the REST API if we'd fix it later

If it were months out from alpha, we could punt on both being in, but if we were to split it now, and commit the first bit (data model changes) but miss the second bit (rest changes), we'd ship 8.5.x with a broken REST api that would be an API break if we changed it later.

So in that regards, we have to do both at once.

larowlan’s picture

Did some manual testing as follows

  • Two sites, one running HEAD, one with this patch
  • Added N terms with hierarchy
  • Verified that reordering terms worked
  • Verified that multiple parents works
  • Added content tagged with term that had multiple parents
  • Added a view with exposed filter showing term hierarchy
  • Verified that filter worked and display as expected
  • Verified that deleting parent term deleted children (if they had only one parent)

Screenshots from testing attached
Going to have a crack at upgrade path test locally.

jibran’s picture

Issue tags: +Needs change record

I think we need a change record here.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
15.09 KB
15.85 KB

Tested the update path.

Before and after shots of my hierarchy - worked.

But the views update didn't work.

Here's my view as yaml, before and after. No change. I had two taxonomy fields on my article.

Also, the code expects everything to be .parent. But as the yaml shows, if I add two filters of type parent, then the second one gets parent_1

And doesn't get updated.

larowlan’s picture

After the update hook, my filters disappeared

damiankloip’s picture

Issue summary: View changes

@larowlan, thanks for testing this out thoroughly. I think I see the issue - I guess (and this is just a assumption for you to confirm or deny at this stage :) ) you did:

- Fresh install on 8.5.x branch
- Create views
- Run updates
-> View has broken handlers

?

If so, I think the issue is that we split the underlying views fix out into #2846614: Incorrect field name is used in views integration for multi-value base fields, which contains the update hooks for the views data. They were originally intended to be run together until we split them up (to reduce the scope of this issue). So your site install based on 8.5.x before this patch would not run the views update handler in the above issue. We then change the final pieces of the views data here, and your view doesn't get updated because the only new update handlers to run are the ones in this issue.

If this is not the case, we will obv. need to dig further.

In a nutshell, I think this is working as expected if the update path contains both, i.e. 8.4 > 8.5.

EDIT: Disregard this, mostly. We do have the other updates in this patch! The other patch is multi value fixes...

Wim Leers’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

Back to RTBC per #301 then.

@damiankloip: it'd be great if you could verify your guess/assumption though, because @larowlan is in Australia and hence is basically awake when we're asleep and vice versa. Then we'll know for certain that either your guess is correct, or this patch needs changes, which you can then still work on today, rather than on Monday, at which point it'll likely be too late.

Also, does this mean that if this patch doesn't land in 8.5, that 8.5 will be in an inconsistent state?

voleger’s picture

+++ b/core/modules/taxonomy/taxonomy.install
@@ -0,0 +1,132 @@
+      $base_path = "display.$display_id.display_options.relationships.parent";

I guess this code don't include the next possible configs for update :

"display.$display_id.display_options.relationships.parent_1"
"display.$display_id.display_options.relationships.parent_3"
"display.$display_id.display_options.relationships.parent_{$n}"
damiankloip’s picture

Status: Reviewed & tested by the community » Needs work

Yes indeed - we need to add more smarts to that update hook logic.

heddn’s picture

Title: Make $term->parent behave like any other entity reference field, to fix REST support and de-customize its Views integration » Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration
Issue summary: View changes
Related issues: +#2935951: Copy migrate source plugin from migrate_drupal_d8 into migrate_drupal

Linking #2935951: Copy migrate source plugin from migrate_drupal_d8 into migrate_drupal as another reason for this change. REST wants to export things. Migrate's d8 source does too.

Wim Leers’s picture

Issue tags: +migrate

#305, nice! Adding tag — hope it's the right one, feel free to correct!

larowlan’s picture

Related issues:

@damiankloip yes I did start with 8.5.x head. Should I try again with 8.4.x as the start point?

What about the parent_1 issue?

Wim Leers’s picture

@larowlan: @damiankloip confirmed (but not in many words) the "parent_1" issue in #304. He's working on it.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
68.8 KB
7.47 KB

@larowlan, I thought that was originally the problem, but I jumped to a conclusion without refreshing my memory on exactly what we covered in the other issue. Subsequently, my memory let me down slightly :D So we definitely need a fix for the static use of 'parent' in the update path ( @voleger, you are also correct in #303).

This patch should fix that. This works for me, with your test view from #299. I have also expanded the update path test to cover this case; The test view used contains 2 filters now, and the update path asserts that both filters get updated correctly.

@larowlan, would be grateful if you could help confirm this again! cheers.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
  1. +++ b/core/modules/taxonomy/taxonomy.install
    @@ -90,43 +90,39 @@ function taxonomy_update_8502(&$sandbox) {
         foreach (array_keys($view->get('display')) as $display_id) {
    -      $base_path = "display.$display_id.display_options.relationships.parent";
    

    So from a single loop and hardcoded handler type in $base_path

  2. +++ b/core/modules/taxonomy/taxonomy.install
    @@ -90,43 +90,39 @@ function taxonomy_update_8502(&$sandbox) {
    +      foreach (['relationships', 'filters', 'arguments'] as $handler_type) {
    +        $base_path = "display.$display_id.display_options.$handler_type";
    

    … to the former loop becoming the outer loop and this one becoming the inner loop, and $base_path including a parametrized handler type.

  3. +++ b/core/modules/taxonomy/taxonomy.install
    --- a/core/modules/taxonomy/tests/src/Functional/Update/TaxonomyParentUpdateTest.php
    +++ b/core/modules/taxonomy/tests/src/Functional/Update/TaxonomyParentUpdateTest.php
    

    Plus expanded test coverage.

Back to RTBC!

larowlan’s picture

+++ b/core/modules/taxonomy/taxonomy.install
@@ -90,43 +90,39 @@ function taxonomy_update_8502(&$sandbox) {
+          if (($table && ($table === 'taxonomy_term_hierarchy')) && ($field && ($field === 'parent'))) {
+            $view->set($table_path, 'taxonomy_term__parent');
+            $view->set($field_path, 'parent_target_id');

nice work!

I manually tested this again, this time going from 8.4.x HEAD to this patch, and it works as designed.

We need a change record here.

But also, I'm thinking about the impact on contrib and custom modules here.

We've got a fair few changes in this patch that are dealing with using the new table instead of the old hierarchy table.

So I looked into our BC policy on that

While the database query API and schema API itself are stable, the schema of any particular table is not. Writing queries directly against core tables is not recommended and may result in code that breaks between minor versions.

So I think that our change notice should reference that policy with a direct link and this text.

NR for the change record

larowlan’s picture

Issue tags: +8.5.0 release notes

And we definitely need to highlight this in the release notes

Wim Leers’s picture

Will write CR in the CET morning.

Wim Leers’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

Created CR: https://www.drupal.org/node/2936675.

We've got a fair few changes in this patch that are dealing with using the new table instead of the old hierarchy table.

Note that 99% of them are for updating the Views integration!

larowlan’s picture

larowlan’s picture

Adding review credits for those who helped shape the patch.

larowlan’s picture

A survey of contrib modules was conducted with damien mckenna and berdir.

We surveyed 89 common contrib modules for Drupal 8 and found only one, Facets module, was using a direct reference to taxonomy_term_hierarchy table.

They already have an existing bug for that, as using the table direct won't work with non SQL entity storage.

@SocialNicheGuru pointed out that this will also break taxonomy manager module.

This module is only in -dev status, and looks like it has a lot of D7 era code - e.g. http://cgit.drupalcode.org/taxonomy_manager/tree/src/TaxonomyManagerHelp...

@SocialNicheGuru has already opened #2932067: Future patch to remove taxonomy_term_hierarchy for that case.

I've discussed the impact of committing this to Drupal 8.5 with @catch.

We've agreed to commit this to 8.6.x and make a decision about backporting to 8.5.x as soon as possible.

Committed as 8640a9d and pushed to 8.6.x

Will hold off publishing the change record until final decision is made on backport.

Leaving at RTBC for 8.5.x

Wim Leers’s picture

Thanks! And thanks for the impressive research you applied to contrib!

heddn’s picture

Discuss among yourselves, but waiting 8 months to get term parent seems a little long for things like #2935951: Copy migrate source plugin from migrate_drupal_d8 into migrate_drupal. Without this feature, we won't be able to fully export the term entity.

larowlan’s picture

Yes, but we also need to consider the perceived stability of Drupal 8 as a platform.

There are 1000 sites running taxonomy_manager and 6000 running facets.

Breaking them with short notice doesn't help the image of Drupal 8.

Yes, both of those modules are doing the wrong thing by querying the table direct.

Yes, one of them has a patch already.

We just need to be mindful that people have live sites that this will impact on.

Wim Leers’s picture

@larowlan 👏

The main problem is that Drupal has traditionally not been great at marking what is an API and what is an implementation detail, which is why we end up with those perceived stability problems and tricky BC decisions (I did a talk about that: https://wimleers.com/talk/bc-burden-benefit).

  1. I provided a patch for the taxonomy_manager module that makes the module work both with and without this patch/commit!
  2. Facets maintainer @borisson_ said he'd get a patch going tonight.

Therefore I think it's safe to say that by the time Drupal 8.5.0 ships in March, both modules will be updated and ready. IOW: it should be safe to commit this to 8.5 now!

xjm’s picture

Regarding #321: Having fixes for known contrib modules is, unfortunately, not the only thing we need to consider in terms of disruption. If we know about multiple contrib modules that will be broken by this, then there are very likely sites with custom code that also relies on the current behavior that will be broken.

  • larowlan committed 8640a9d on 8.6.x
    Issue #2543726 by damiankloip, Wim Leers, claudiu.cristea, voleger,...
Wim Leers’s picture

Agreed. We have to weigh risks, pros, cons, and so on.

On the other hand, if developers choose to not use the in this case pretty clearly documented APIs, and choose to depend on internal implementation details, then Drupal would only able to add things.

Good luck with making a decision, I know it's tricky to balance all the requirements!

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

larowlan’s picture

Version: 8.6.x-dev » 8.5.x-dev
xjm’s picture

Version: 8.5.x-dev » 8.6.x-dev

@heddn, regarding #319, is this issue actually a hard blocker for that one? @catch and I were unsure how it would be at first glance. (It seems to already have a patch without this.)

heddn’s picture

re #327: No, it is a soft blocker. What I'd imagine we would do is have two patches. One for 8.6 and one for 8.5. Where 8.5 wouldn't have the parent functionality in it. Then, as a work around if someone needed the parent, they'd have to continue using contrib migrate_drupal_d8 or create a follow-up to add a deprecated class to core that adds the term parent via a prepareRow callback. This would be introduced as deprecated and only be a patch for 8.5.

We can get away without having parent for core's initial purposes because we don't need that data to unblock the critical release blocker for migrate_drupal. But it is an incomplete implementation in 8.5, so the change record could get a little confusing. And I don't really like adding a deprecated class, because that just gets messy for BC. TLDR; it is a soft blocker, so we'll do what the community desires.

Wim Leers’s picture

#328: Do I understand you correctly that with just core's Migrate module, you'd lose your term parents, you need the contrib module https://www.drupal.org/project/migrate_drupal_d8 to not suffer any data loss during migrations?

heddn’s picture

re #329: for core's initial use of its own version of the contrib rendering of d8 source plugin that is forked and improved from contrib... we do not need the parent. In 8.5 we'd be OK to unblock the last few stable blockers for migrate without that feature.

If someone needs/requires the parent in the 8.5 timeframe, they would need to open an issue to have the feature backported and we'd have to alter the deriver to spit out the special snowflake term plugin that returns the parent. Or, alternatively, they can use contrib. Or they can wait for 8.6 or upgrade to 8.6 and use core's generic source plugin.

The reason this works in 8.6 is that we use the entity API to gather all the source details. And the entity API for terms only get fixed with this issue.

TLDR; data loss could occur to the uninformed developer using the entity source plugin during the 8.5.x timeframe. Because the uninformed developer wouldn't know that parent isn't available. Alternatively, we could introduce a special snowflake that is @deprecated from its infancy. This special snowflake would only exist in the 8.5 branch. But this special snowflake stuff would all go into a follow-up where we could discuss actual implementation because our goal it to unblock migrate. We just need to know if we need to open that follow-up, and switch out the @TODO to point to it.

Wim Leers’s picture

for core's initial use of its own version of the contrib rendering of d8 source plugin that is forked and improved from contrib... we do not need the parent.

data loss could occur to the uninformed developer using the entity source plugin during the 8.5.x timeframe. Because the uninformed developer wouldn't know that parent isn't available.

So the answer to #329 is "yes, there would be data loss".

Wim Leers’s picture

#2919034: FacetsHierarchy Taxonomy uses old (taxonomy_term_hierachy) table. landed, which means https://www.drupal.org/project/facets will soon ship a release that is compatible with this patch.

For https://www.drupal.org/project/taxonomy_manager, I posted a working patch two days ago at #2932067-9: Future patch to remove taxonomy_term_hierarchy, but haven't gotten any feedback. That issue was first filed on December 20, almost a month ago. Yet since then, only 3 followers: the original reporter (@SocialNicheGuru), @larowlan, and I. Which is understandable, since that module A) has no stable release for D8, B) its D8 codebase is littered with dead D7 code, C) has no active maintainer. Hence waiting to commit this patch to 8.5 until that patch gets committed may be disproportionate. When maintainers do respond to users complaints, they will have a patch awaiting them.

effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -8.5.0 release notes +8.6.0 release notes

I discussed this with @larowlan, @catch, @xjm, and we're all more comfortable with not cherry picking this to 8.5.x. Because even though everything it does is compatible with our policy for what's allowed into a minor release, the reality is that per #320 and #322, there are some known contrib modules and unknown custom modules, that are accessing internal implementation details rather than stable APIs, that will be disrupted by this. That's fine, but it's far more courteous to provide reasonable lead time for those affected by this to discover it and adapt, and I think the time left before 8.5.0 is released isn't sufficiently courteous.

I realize that's disappointing for REST/JsonApi/GraphQL/etc. use cases. If there are decoupled sites in desperate need of reading or writing $term->parent, perhaps some temporary taxonomy hierarchy API could be provided in contrib until 8.6.0 is released?

I discussed #331 with the Migrate maintainers. While a data loss in 8->8 migrations is not ideal, we can all live with it for now, because while getting 6->8 and 7->8 migrations stable is critical, 8->8 migrations are currently a minority use case and therefore of lower priority for now. Also, @heddn suggested that a workaround to fix the 8->8 data loss problem of $term->parent is relatively straightforward and will open an issue in which to do so, which might land in time for 8.5.0, or if not, then perhaps in a patch release soon following. Thank you, Migrate maintainers, for your understanding and flexibility about this.

Most importantly, a big thank you to everyone who's worked on this issue! I'm thrilled that it's in HEAD, so that all future core work (such as #2836165: New experimental module: JSON API) can start benefitting from it, even if it's still a bunch of months away from making it into a tagged release.

xjm’s picture

Issue tags: +Needs followup

Another thing that we discussed is that we'd like to explore some way of warning developers that the table is deprecated in 8.5.x. I would have preferred that that happen before commit, but can we get a followup issue for that?

borisson_’s picture

#2919034 landed, which means https://www.drupal.org/project/facets will soon ship a release that is compatible with this patch.

I tagged a new release right after I committed that issue. Thanks again for helping out on that @larowlan, @Wim Leers @Berdir.

Wim Leers’s picture

If there are decoupled sites in desperate need of reading or writing $term->parent, perhaps some temporary taxonomy hierarchy API could be provided in contrib until 8.6.0 is released

Such a temporary API in contrib requires disproportionately much effort from those who helped fix the root cause of the problem, i.e. those who helped fix this issue.
Those who want this to work should just apply this core patch to Drupal 8.5, just like they already have been doing in 8.4. With the knowledge that as soon as they update to Drupal 8.6, that won't be necessary anymore. That's what e.g. the JSON API module has been recommending (in #2775635: [BUGFIX] Parent is always empty for taxonomy terms).

Another thing that we discussed is that we'd like to explore some way of warning developers that the table is deprecated in 8.5.x. I would have preferred that that happen before commit, but can we get a followup issue for that?

I don't see how we could throw E_USER_DEPRECATED errors for database queries. I think a change record for 8.5 that warns about the transition in 8.6, and an explicit mention in the 8.5 release notes are the best (only?) ways to communicate this.

I tagged a new release right after I committed that issue.

Woah, awesome, thanks @borisson_! :)

dawehner’s picture

I don't see how we could throw E_USER_DEPRECATED errors for database queries. I think a change record for 8.5 that warns about the transition in 8.6, and an explicit mention in the 8.5 release notes are the best (only?) ways to communicate this.

One could come up with an idea which involves the database layer directly, but I'm not sure its worth the performance impact.

Wim Leers’s picture

Right, that is what I implied: we could do parsing of queries, but that seems pretty excessive.

catch’s picture

Those who want this to work should just apply this core patch to Drupal 8.5, just like they already have been doing in 8.4.

Yes I think this is fine, and it's much nicer having a patch applied when you know it's been applied to the next minor branch than when it's stuck at CNW/CNR.

Berdir’s picture

About the messages, I'm also against doing anything there. It's not actually deprecated, it's just not supported by BC and should not be done *if possible*. But the entity/term storage itself continues to and will always do those kind of queries, obviously. And it would be insane to try and detect who can do such a query and who can't :) Oh and views of course ;)

Truth is that sometimes there are scenarios where it can't be avoided, especially for your own entity types but also others.

amateescu’s picture

We also have the option to do #2921048: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix for 8.5.x, which would give us all the benefits of this patch (working REST support, etc.) without the BC-breaking storage changes.

larowlan’s picture

Published the change record

Wim Leers’s picture

Wim Leers’s picture

In other happy news, #2932067: Future patch to remove taxonomy_term_hierarchy was also committed, so Taxonomy Manager is also forward-compatible with Drupal 8.6/this patch!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

larowlan’s picture

Found another module that uses the taxonomy_term_hierarchy table - shs module.

#2942358: The taxonomy_term_hierarchy table will be removed in Drupal core 8.6.0

heddn’s picture