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

This looks nice! 👏

Except for one thing:

+++ b/core/modules/hal/tests/src/Functional/EntityResource/Term/TermHalJsonAnonTest.php
@@ -47,6 +54,29 @@ protected function getExpectedNormalizedEntity() {
+    if ($has_parent) {
+      $normalization['_links'][$this->baseUrl . '/rest/relation/taxonomy_term/camelids/parent'] = [
+        ['href' => $this->baseUrl . '/taxonomy/term/2?_format=hal_json'],
+      ];
+      $normalization['_embedded'][$this->baseUrl . '/rest/relation/taxonomy_term/camelids/parent'] = [
+        [
+          '_links' => [
+            'self' => [
+              'href' => $this->baseUrl . '/taxonomy/term/2?_format=hal_json',
+            ],
+            'type' => [
+              'href' => $this->baseUrl . '/rest/type/taxonomy_term/camelids',
+            ],
+          ],
+          'uuid' => [
+            ['value' => Term::load($this->entity->get('parent')->target_id)->uuid()],
+          ],
+        ],
+      ];
+    }
+

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
@@ -90,6 +90,22 @@ protected function createEntity() {
+    $has_parent = !$this->entity->get('parent')->isEmpty();
+    if ($has_parent) {
+      $parent_id = $this->entity->get('parent')->target_id;
+      $parent = [
+        [
+          'target_id' => $parent_id,
+          'target_type' => 'taxonomy_term',
+          'target_uuid' => Term::load($parent_id)->uuid(),
+          'url' => base_path() . 'taxonomy/term/' . $parent_id,
+        ],
+      ];
+    }
+    else {
+      $parent = [];
+    }

This is wrong, this is only adding _links and _embedded (hal_json format) or a non-empty parent field (json format) for $term->parent if and only if the parent is not <root> (or term zero).

We need those to be always present, otherwise API clients would have to hardcode that logic themselves. We want to be explicit about the fact that a term doesn't have a parent.

#2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration gets this right.

Actually, I think that this patch does the right thing, IMO.

I strongly disagree. This patch causes the normalizations/API responses to communicate information implicitly rather than explicitly, which can lead to misinterpretations, and requires all API clients to build in this understanding.

In #2543726-205: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration the _links and _embedded entries look like this when the parent term is the root term (term zero):

'_links' => [
  'http://d8/rest/relation/taxonomy_term/camelids/parent' => [
    NULL
  ],
],
 '_embedded' => [
  'http://d8/rest/relation/taxonomy_term/camelids/parent' => [
    NULL
  ],
],

The absence of a URL explicitly communicates that there is no parent resource to request.

multi-parent

This is a very good point, and something we're missing test coverage for. Working to add that… stay tuned.

Added that multi-parent test coverage at #2543726-216: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration — now you can inherit it here.

Do you happen to know of any examples of REST API's dealing with a multi-parent hierarchy resource like we have here?

Not immediately, no.

The answer might very well be that we need to keep the magic <root> = 0 assumption that we have […]

I think so, yes.

(Typing on my phone from 🏓 competition.)

Huh? :( I really did not intend to upset you! I did read everything you wrote, and I tried to constructively explain why I disagree. The explicit versus implicit.

EDIT: oh, I see, I think you mean that you ended up kind of agreeing with me, yet my comment still answers part of your comment before you agreed. You’re right. I should’ve edited that away. It’s just how I gradually answer a comment, and if I switched tasks in between I sometimes forget to go back and re-read and revise the entire comment. My apologies!

NOw that we are not going to backport #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration to 8.5x. Can we consider this patch for 8.5.x?

@jibran: yes, indeed, and @amateescu asked the same question at #2543726-341: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration. We'll have to be VERY careful to ensure it behaves exactly the same though. We'd need this to work exactly the same as 8.6.

Marking NW because #11 does not include the same test coverage that #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration committed to 8.6.

Just chatted with @damainkloip about this.

We agreed that it's probably better to not try to land this issue in Drupal 8.5, because:

1. The general recommendation from #2543726 is to just apply the latest patch to Drupal 8.5, site owners will simply be able to drop that patch once they update to Drupal 8.6 (see #2543726-339: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration). The benefit of applying that patch is that it also works for other components in the API-First ecosystem: https://www.drupal.org/project/jsonapi and https://www.drupal.org/project/graphql. If we'd commit a custom temporary normalizer here, then that temporary normalizer would have to be removed if they are using JSON API or GraphQL and want $term->parent to work!
2. There is significant risk in ensuring that the temporary normalizer this issue would introduce behaves exactly the same. If we get even one detail wrong, we'd be having another BC break to worry about in the update path from 8.5 to 8.6! And even if we do that now, how can we ensure that behavior stays perfectly in sync between 8.5 and 8.6 during the lifetime of 8.5? (Admittedly the risk is lower then, but not zero.)
3. In general, the effort vs shelf life ratio doesn't look very attractive. Quite a lot of effort, with quite a bit of risk, with quite a bit of ecosystem downsides, whereas "just apply the core patch in 8.5" doesn't involve extra risk nor effort.

Indeed - this patch does not add a normalizer of any kind.. yet. I think we were thinking that if we add this, we would need to also add a normalizer (or change how the new list class handles the computed items for no parent) to return values for the parent = 0 case. The patch would currently just return an empty array instead of a value of 0. Which is different to the new behaviour in #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration.

Otherwise, we shoot ourselves in the foot a little bit and make an additional API BC break for ourselves, if we're not careful.

Thoughts? :)