Port reference field support for non-empty entity reference fields not pointing to an entity from #2543726

Now really starting this… because this is now also blocking #2945093, per #2945093-53: Comprehensive JSON API integration test coverage phase 3: test JSON API-specific use cases: related/relationship routes, includes and sparse field sets.

Fortunately, there already are some existing test assertions that we'll need to make pass in HEAD. So let's start with those.

1. +++ b/tests/src/Functional/TermTest.php
   @@ -123,9 +123,8 @@ class TermTest extends ResourceTestBase {
          case [0]:
   ...
            $expected_parent_normalization = [
   -          'data' => [],
   +          'data' => NULL,
   

   This one should not be a problem.

2. +++ b/tests/src/Functional/TermTest.php
   @@ -151,7 +150,7 @@ class TermTest extends ResourceTestBase {
          case [0, 2]:
            $expected_parent_normalization = [
              'data' => [
   -            // @todo This is missing the root parent, fix this in https://www.drupal.org/project/jsonapi/issues/2940339
   +            NULL,
                [
                  'id' => Term::load(2)->uuid(),
   

   This one could be trickier. Hopefully not!

This would be simpler if #2940342: Cacheability metadata on an entity fields' properties is lost had already landed, because that's changing quite a bit wrt normalizing fields, this will most certainly conflict.

There are six cases to support:

1. Single-cardinality entity reference field, empty (only possible if optional): 'data': null
2. Single-cardinality entity reference field, non-empty, but has target_id pointing to non-existent entity: 'data': null
3. Single-cardinality entity reference field, non-empty: 'data': {…}
4. Multiple-cardinality entity reference field, empty (only possible if optional): 'data': []
5. Multiple-cardinality entity reference field, non-empty: 'data': [{…}, {…}, {…}]
6. Multiple-cardinality entity reference field, non-empty, but has >=1 reference with target_id pointing to non-existent entity: 'data': [{…}, null, {…}]

What do we support, and what do we have test coverage for?

• HEAD only supports cases 3, 4 and 5.
• #2945093-53: Comprehensive JSON API integration test coverage phase 3: test JSON API-specific use cases: related/relationship routes, includes and sparse field sets is adding test coverage for cases 1 and 4, and is getting stuck on that (case 1 is treated as case 4).
• #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only gave JSON API (disabled) test coverage for cases 2 and 6. #4 re-enables that test coverage.
• Together, that should give us coverage for all edge cases!

More tomorrow.

No. A top-level Term entity's parent field is explicitly set to 0 (zero) to indicate that its parent is the non-existent "root" Term.

Quoting \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::isEmpty(), which we should respect:

  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;
  }

i.e. any value being set on target_id means it's not empty. For Term entities, it's 0. For others, it could be 'foobar'.

Data integrity is up to the entity type's validation constraints to guarantee. JSON API should not try to interpret it in some way.

Two and six are the same thing. In case of e.g. Terms: a term could have both <root> (target_id = 0) and blabla (target_id = 34598) as a its parents. That should then result in

'data': [null,
 {'id': 'some-uuid-here', 'type': 'taxonomy_term--tags'}]

Actually

+++ b/tests/src/Functional/TermTest.php
@@ -123,9 +123,8 @@ class TermTest extends ResourceTestBase {
-          'data' => [],
+          'data' => NULL,

this is wrong!

It should be

'data' => [NULL],

because the JSON API spec says at http://jsonapi.org/format/#document-resource-object-linkage:

Resource linkage MUST be represented as one of the following:

• null for empty to-one relationships.
• an empty array ([]) for empty to-many relationships.
• …

and because the parent always is a to-many relationship:

    $fields['parent'] = BaseFieldDefinition::create('entity_reference')
      ->setLabel(t('Term Parents'))
      ->setDescription(t('The parents of this term.'))
      ->setSetting('target_type', 'taxonomy_term')
      ->setCardinality(BaseFieldDefinition::CARDINALITY_UNLIMITED);

Fixing that expectation. The test will still fail though: HEAD is still broken.

And this makes TermTest::getIndividual() pass!

However, this sadly causes e.g. the PATCH test coverage to fail. Because that doesn't know how to deal with null values.

That being said, I think that the expectations in #13 are technically violations of JSON API… because it breaks linkage.

But this is how Term entities are designed to work! I'm not sure how to continue here. Thoughts?

Clarifying that this needs feedback.

This is a little mind-bending. How can a parent by both a child and not child at the same time

That just meant there can be multiple parents? E.g. chocolate is a child of both food and Belgian specialties.

Long story short: I think we need special casing for targeted resources which do not exist. Regardless of whether or not entity reference field values allow it or not.

That's also what I was leaning towards… thanks for the sanity check!

which would indicate that the term was both a root and a child simultaneously

Indeed, and that's entirely valid/possible!

And in fact, that's exactly my main concern here: if a term has both the root as its parent and a non-root term, then it'd still end up being serialized to 'data': [{…}]. Now it seems the term does NOT have multiple parents, but only a single parent!

So while I agreed with

Long story short: I think we need special casing for targeted resources which do not exist. Regardless of whether or not entity reference field values allow it or not.

… I don't yet know how to do that. The best idea I've had is coming up with a pseudo root term with an all-zero UUID.

In CS terms it's also an invalid graph—one can't have an edge to a node that doesn't exist.

That's not true. It's totally possible to have the root node not exist in memory, and still be able to traverse the tree. It'd just mean special casing that one possible parent. And that's what we did here.

I think we need to drop PATCH support […] On GET, we need to elide reference items from relationships when they point to non-existant resources.

Dropping PATCH support altogether and eliding from GET responses still does not solve the problem: you still won't be able to know whether a term has the root term as a parent. And that's crucial to know.

Additionally, we could also add entity references back to attributes […]

AFAICT that wouldn't work either, because the "related" and "relationships" responses still won't be able to make sense/be consistent with what's in attributes.

I'm not sure where you're going with #24 :) Can you elaborate?

What I'm trying to say is that GETing from a relationship route is not meant to tell you about field values. It's telling you about linkage.

Ah, of course, +1!

Nice ASCII art, although I don't fully grasp it :P Because I don't see how B has target_id === [1, 2]. Doesn't matter though, I can reply to A/B/C/D.

In this A/B/C/D, I think you or I or both of us are mixing up the possible values for target_id (Drupal/database land) with those in the JSON API representation. Only the JSON API representation can contain null!

• (A) Yes, I'm referring to the virtual "root" (<root> in the UI), which is virtual because no Term entity actually exists in the DB that represents it. It's essentially an unmodifiable, unloadable, unviewable Term.
• (B) You absolutely can create this, as you later discovered :)
• (C) I see what you're saying (internal values for an entity reference field may not make sense to represent in JSON API relationships), but I don't think it applies here. I guess I actually don't fully understand what you mean here I think you meant target_id = [1, 0] (Drupal/DB land) maps to relationships = [1, null] (JSON API land)? But if so, that's the same as (D). In your ASCII art, you say there is no node in the graph (hence the NULL?) and are asking whether this then even is a valid graph. The implication being that this cannot occur. I agree, I don't think actually can occur.
• (D) Same as (B).

[{"type": "taxonomy_term--foo", "id": "virtual"}] // When target_id === 0

😍 I DID NOT REALIZE THIS WAS OKAY! I thought the JSON API spec mandated that every resource identifier's id contained a UUID, but apparently that's not the case! In fact, the JSON API spec only mentions UUIDs in the context of client-generated IDs: http://jsonapi.org/format/#document-resource-identifier-objects.
Cool, this is a great way out!

This leaves only one remaining concern, can we differentiate between NULL and target IDs?

First off: THANK YOU THANK YOU THANK YOU THANK YOU THANK YOU THANK YOU for your deep dive on this, your analysis here was super helpful, super insightful, and obviously unblocks further progress! 🎉 👏 Without you, we wouldn't be at this place where that is the only remaining question :)

Now, to your sole remaining concern!

I'm not yet convinced that a null target ID is in fact possible. See the code I quoted in #11: \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::isEmpty():

  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;
  }

IOW: if it's NULL, then it's empty, and hence entity reference fields can only contain non-NULL values. Zero is an example, but generally it'll be non-zero, because it'll reference entities by non-zero numerical ID.

Getting this going again.

The last iteration of this patch is >2.5 months old. Rebased, now retesting, to see how it fares.

#29.5: 🙏 — apparently that changed since #14 from 2.5 months ago!

Only fixing that in this reroll.

WOW, I did not expect that!

I was hoping we'd go back from #28's 92 fails to #14's 8 fails.

• Fixed CS (includes #29.2 + #29.5.
• #29.1: Why?
• #29.3: That's just c/p'd from RelationshipItemNormalizerValue + updated.
• #29.4: Fixed.

Still to do: implement the outcome of the discussion in #17–#27! That includes addressing #29.6.

I believe this implements #17–#27. This is the goal:

diff --git a/tests/src/Functional/TermTest.php b/tests/src/Functional/TermTest.php
index 61bd003..bddb348 100644
--- a/tests/src/Functional/TermTest.php
+++ b/tests/src/Functional/TermTest.php
@@ -126,7 +126,10 @@ class TermTest extends ResourceTestBase {
       case [0]:
         $expected_parent_normalization = [
           'data' => [
-            NULL,
+            [
+              'id' => 'virtual',
+              'type' => 'taxonomy_term--camelids',
+            ],
           ],
           'links' => [
             'related' => $self_url . '/parent',
@@ -153,7 +156,10 @@ class TermTest extends ResourceTestBase {
       case [0, 2]:
         $expected_parent_normalization = [
           'data' => [
-            NULL,
+            [
+              'id' => 'virtual',
+              'type' => 'taxonomy_term--camelids',
+            ],
             [
               'id' => Term::load(2)->uuid(),
               'type' => 'taxonomy_term--camelids',

Because these "null relationships" or "virtual relationships" now need to generate a proper resource identifier object ({'type':'something','id':'virtual'}) rather than null — as this test expectation diff shows, it no longer makes sense to have a separate normalizer value and normalizer. We just need to update RelationshipItem a bit.

It also makes the diffstat significantly smaller! From

7 files changed, 124 insertions, 17 deletions.

to

4 files changed, 56 insertions, 19 deletions.

Let's make a change record/documentation for this and then make a follow-up to add a meta member to these resource identifier objects so we end up with:

I can do that here.

I thought I was going mad, I couldn't get the green patch at #33 to pass locally! Turns out it was only passing on 8.5, not on 8.6. The 8.6 test I queued confirmed that.

This will make it pass on both branches.

Now it should pass on 8.6. The reason that the above patches were only failing on 8.6 is because Drupal 8.6 fixed the parent field. Which is why only on 8.6 the "relationships" routes will work as expected for Term entities.

Also fixed CS violations.

Oops, that -41 patch should not have been uploaded. It was not ready. Please ignore it. This patch will be relative to #40.

The #40 interdiff fixed Drupal\Tests\jsonapi\Functional\TermTest::testGetRelationships(), but now Drupal\Tests\jsonapi\Functional\TermTest::testRelated() is failing.

(That, and #40 has ::testGetRelationships() still failing on 8.5, because #40 didn't think to only do it for 8.6. Even though I mentioned in the comment for #40 that it had 8.6-only behavior. I forgot to add a hunk.)

This fixes both of those. Was a nightmare to solve!

#42: because help would be the link label/key, and picking something super generic like help prevents us from using this for more generic purposes in the future.

WHEW!

Green again at last! Now green on both 8.5 & 8.6!

Now to finally address #34 + #35.

I had accidentally uploaded a patch that implements #34. @gabesullice of course saw this,
and already reviewed it in #42. I gave my rationale in #43, but still looked at the links Gabe provided. One of them said:

Refers to context-sensitive help.

… which of course is what this does. So, did that. Now it's exactly as #34 suggested :)

Also:

1. Better issue title.
2. Updated documentation: https://www.drupal.org/docs/8/modules/json-api/core-concepts#virtual — diff: https://www.drupal.org/node/2803133/revisions/view/10926052/10989299
3. Created change record: https://www.drupal.org/node/2976856

This caused a regression: #2977879: Regression in #2940339: when multiple vocabularies exist, normalization of Terms fails. But the root cause actually lies in Drupal core: #2977882: Term entity type's "parent" field does not specify "target_bundles" setting: a term from *any* vocabulary can be chosen.