Problem/Motivation

As explained in Proposing an alternative to application/vnd.drupal.ld+json, the new plan is to use HAL as the primary format for REST. First we'll need to decide how to serialize an entity to hal+json.

We will definitely add a link for each entity reference. Some have stated a preference for additionally including the entity reference field with the other fields in the resource's state.

Proposed resolution

Add a set of Normalizers and an Encoder, creating the following JSON structure:

  {
    "_links": {
        "self": {
            "href": "http://d8.l/entity/entity_test/3"
        },
        "type": {
            "href": "http://d8.l/rest/type/entity_test/entity_test"
        },
        "http://d8.l/rest/relation/entity_test/entity_test/field_test_entity_reference": [
            {
                "href": "http://d8.l/entity/entity_test/1",
                "lang": "de"
            },
            {
                "href": "http://d8.l/entity/entity_test/2",
                "lang": "en"
            }
        ]
    },
    "_embedded": {
        "http://d8.l/rest/relation/entity_test/entity_test/field_test_entity_reference": [
            {
                "_links": {
                    "self": {
                        "href": "http://d8.l/entity/entity_test/1"
                    },
                    "type": {
                        "href": "http://d8.l/rest/type/entity_test/entity_test"
                    }
                },
                "uuid": [
                    {
                        "value": "083028f3-f851-4ba9-a763-996bc09e9338"
                    }
                ],
                "lang": "de"
            },
            {
                "_links": {
                    "self": {
                        "href": "http://d8.l/entity/entity_test/2"
                    },
                    "type": {
                        "href": "http://d8.l/rest/type/entity_test/entity_test"
                    }
                },
                "uuid": [
                    {
                        "value": "daf2fd6e-d9e3-4566-ac06-6fb2379cc374"
                    }
                ],
                "lang": "en"
            }
        ]
    },
    "uuid": [
        {
            "value": "c4d98886-533a-4270-8e00-b490a8c44580"
        }
    ],
    "langcode": [
        {
            "value": "de"
        }
    ],
    "name": [
        {
            "value": "uP7sBcAa",
            "lang": "de"
        },
        {
            "value": "rM0oP6vM",
            "lang": "en"
        }
    ],
    "field_test_text": [
        {
            "value": "jUedNbKA",
            "format": "full_html"
        }
    ]
}
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Status: Needs work » Needs review
FileSize
21.83 KB
29.9 KB

Based on the preference described in the issue summary, this patch creates the following structure. I'm not sure that we will want to include the entity references in the state, but for now it is, with UUID as its only property.

We also may want to consider defining relations in a Drupal namespace. For example, site:type could be drupal:type instead since that is a relation that will always mean the same thing across the different sites.

{
    "_links": {
        "curies": [
            {
                "href": "/relations/{rel}",
                "name": "site",
                "templated": true
            }
        ],
        "self": {
            "href": "/entity/entity_test/3"
        },
        "site:type": {
            "href": "/types/entity_test/entity_test"
        },
        "site:user_id": [
            {
                "href": "/user/1",
                "lang": "de"
            }
        ],
        "site:field_test_entity_reference": [
            {
                "href": "/entity/entity_test/1",
                "lang": "de"
            },
            {
                "href": "/entity/entity_test/2",
                "lang": "en"
            }
        ]
    },
    "uuid": [
        {
            "value": "855877f3-2000-4738-8c00-b6f3972a5a2b"
        }
    ],
    "name": [
        {
            "value": "fFJkn9VE",
            "lang": "de"
        },
        {
            "value": "gWNJCsX0",
            "lang": "en"
        }
    ],
    "user_id": [
        {
            "uuid": "aaa680c3-17b5-1007-b2a7-143a68996533",
            "lang": "de"
        }
    ],
    "field_test_entity_reference": [
        {
            "uuid": "cb468093-1a35-4050-a027-392a689964d7",
            "lang": "de"
        },
        {
            "uuid": "c9d4f660-eb76-4d85-be04-4b000244e870",
            "lang": "en"
        }
    ]
}

This patch depends on #1921490: $supportedInterfaceOrClass should not be static.

Status: Active » Needs work

The last submitted patch, 1924220-01-hal-serialize.patch, failed testing.

Crell’s picture

Just quick notes for now:

Does UUID need to be a multi-value field? That seems odd.

What's curies?

Anonymous’s picture

Status: Needs review » Needs work

We could add special processing for UUID if we want to. However it would require a conditional check on the property name since there is no special Typed Data API datatype for UUID, and thus no class or interface to use to switch the Normalizer.

That refers to the CURIEs used in XML and RDF to shorten URIs by using prefixes... for example, site:user_id expands to http://example.com/relations/user_id. So the "curies" array provides namespace templates that can be used to expand the CURIE. The Web Linking RFC (rfc5988) recommends that link relations be URIs if you aren't going to register them with IANA. My understanding is that HAL does not require conformance with this RFC, but if you do use it then the HAL Browser is a more effective documentation tool. For example, checkout the browser for HAL Talk and click on the docs link next to a property like ht:users.

I realized after you asked that the /{rel} got dropped from the end of the 'href' in the curie object when I was editing the Normalizer, so I've added that back in. That's required for the templating to work.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
3.11 KB
22.22 KB
30.29 KB

The test fail was based on the change from #1891516: Remove $install parameter from DrupalUnitTestBase::enableModules(), encourage individual schema tables. I was able to make it stop failing, but it still throws exceptions based on the use of language() in EntityNG and EntityBCDecorator.

Status: Needs review » Needs work

The last submitted patch, 1924220-05-hal-serialize.patch, failed testing.

Anonymous’s picture

It turns out that there is a "type" link relation in the IANA registry, so we wouldn't need to coin drupal:type.

Anonymous’s picture

Issue summary: View changes

Added JSON.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
6.38 KB
22.94 KB
31 KB

This patch:

  • Changes the "site:type" CURIE to the registered link relation "type"
  • Fixes the tests by adding the language to the entity values before calling entity_create
  • Switches the structure of the JSON to have all property values for referenced entities added to _embedded (rather than included in the subject entity's state).

I wanted to use _embedded for now because it is more expressive... it makes it clear that the UUID of the target entity is a property of that separate entity, not of the entity_reference field. We can change this later if need be, but I'd rather try it out this way.

I have added the JSON structure to the issue summary.

Anonymous’s picture

The two big changes in this patch are:

  • The EntityReferenceItemNormalizer no longer creates the structure for the target entity. Instead, it passes it off to the EntityNormalizer, using the $context parameter to specify that only the UUID should be included.
  • It introduces a LinkManager in REST module. This LinkManager is used to get the URIs for types (implemented) and relations (not implemented yet). This is somewhat similar to the SiteSchemaManager that was created for a similar purpose in RDF module when we were working with JSON-LD.

EDIT: This also introduces a dependency on REST module. I felt that the hypermedia link relations should be independent of the format, which is why I put them in REST.

sun’s picture

I'd love to help you further here, but unfortunately, I'm not familiar with this (entire) part of the code-base yet. I hope that @klausi and others can review this as soon as possible. This patch actually looks relatively simple.

Architecturally, I only have one concern, but it is off-topic for this issue:

The hal.module is empty. I'm really not sure whether it is a good idea to add these serializers as independent modules, if their entire functionality consists of cleanly decoupled classes only. By the looks of this code, I'd almost be tempted classify serializers as plugins...?

But as mentioned, that's off-topic for this issue. Merely had a brief look at the patch and this is what crossed my mind, so I wanted to mention it :)

sun’s picture

Issue summary: View changes

removed user_id

Anonymous’s picture

Thanks for taking a look at the code :)

We don't need to make it a plugin in order to make it not it's own module. We could handle it the same way that the JSON and XML Normalizers and Encoders are handled, as part of serialization module. We don't add new Serializers, there is only one serializer which is registered with the container. In a compiler pass, the support for different formats is added to this Serializer using Normalizers and Encoders. The Serializer then uses a chain of command pattern to determine which Normalizers to use for a request.

So we don't need to have a module per format. However, in this patch I introduce a dependency on REST module. REST module has a dependency on Serialization module, so adding the HAL functionality to serialization module would create a circular dependency.

klausi’s picture

+++ b/core/modules/hal/hal.info
diff --git a/core/modules/hal/hal.module b/core/modules/hal/hal.module
index 0000000..b3d9bbc

index 0000000..b3d9bbc
--- /dev/null

--- /dev/null
+++ b/core/modules/hal/hal.module

+++ b/core/modules/hal/hal.module
+++ b/core/modules/hal/hal.module
@@ -0,0 +1 @@

@file doc block is missing that says that Drupal needs this empty file.

+++ b/core/modules/hal/lib/Drupal/hal/Encoder/JsonEncoder.php
@@ -0,0 +1,33 @@
+/**
+ * @file
+ * Definition of Drupal\hal\JsonEncoder.
+ */

Should be "Contains ...", see http://drupal.org/node/1354#files

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/EntityNormalizer.php
@@ -0,0 +1,103 @@
+      if ($property->getName() == 'id') {
+        continue;

Why do we skip the id? Please add a comment.

+++ b/core/modules/hal/lib/Drupal/hal/Tests/NormalizerTestBase.php
@@ -0,0 +1,91 @@
+use Drupal\Core\Language\Language;
+use Drupal\rdf\SiteSchema\SiteSchema;

is that needed?

+++ b/core/modules/rest/lib/Drupal/rest/LinkManager/TypeLinkManager.php
@@ -0,0 +1,92 @@
+  /**
+   * @var \Drupal\Core\Cache\CacheBackendInterface;
+   */

What is this cache for? Should have a short description.

But that are just nitpicks, otherwise looks fine from a visual review.

Anonymous’s picture

Fixed issues from Klaus's post and also fixed a bug when an empty entity is serialized.

Why do we skip the id? Please add a comment.

Because we shouldn't expose internal IDs to the external world. Only UUID/URI are suitable identifiers for a hypermedia context.

Anonymous’s picture

Updated for the change from .info to .info.yml.

Anonymous’s picture

Issue summary: View changes

Updated the JSON representation.

Anonymous’s picture

Issue summary: View changes

Updated the JSON.

Anonymous’s picture

This patch adds a RelationLinkManager and changes all of the CURIEs and relative URIs to absolute URIs. We can change the link relations back to CURIEs later if we want to.

Anonymous’s picture

I removed the caching of type and relation links, since that won't be used until #1880424: Handle entity references on import.

klausi’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/hal/hal.info.yml
@@ -0,0 +1,7 @@
+  - rest
+  - serialization

new line error

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/EntityNormalizer.php
@@ -0,0 +1,81 @@
+  protected function getEntityUri($entity) {
+    // @todo Remove this conditional once entities are converted to EntityNG.
+    if ($entity instanceof EntityNG) {
+      $uri_info = $entity->uri();
+      return url($uri_info['path'], array('absolute' => TRUE));
+    }

why do you have to check for EntityNG here? The uri method exists on the entity class already and works for me?

+++ b/core/modules/hal/lib/Drupal/hal/Tests/NormalizerTestBase.php
@@ -0,0 +1,90 @@
+use Drupal\Core\DependencyInjection\ContainerBuilder;
+

superfluous use statement

+++ b/core/modules/rest/lib/Drupal/rest/LinkManager/RelationLinkManager.php
@@ -0,0 +1,32 @@
+
+use Drupal\Core\Cache\CacheBackendInterface;

superfluous use statement.

I tested the module manually and it works as advertised. The test case also seems to cover the functionality sufficiently, so I would say this is RTBC.

Anonymous’s picture

Thanks, I made all the changes except for the one below. I also added HAL module to MAINTAINERS.txt.

new line error

By new line error, I assume you mean the dash in front of 'serialization'. If you look at picture.info.yml, it also includes a second dash. I'd say we stick with it for now and go through and correct all of the info files if we're going to go with more standard YAML format.

Anonymous’s picture

FileSize
2.63 KB

And here's the interdiff I forgot...

webchick’s picture

Assigned: Unassigned » Dries

My spidey sense is telling me this is the kind of thing Dries might want to sign off on.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks. Now let's remove JSON-LD from core.

Anonymous’s picture

There is a patch to remove it. However, it depends on other things getting in, like HAL deserialization #1931976: Support deserialization for hal+json (needs more language handling tests) and converting REST module to use HAL for its tests #1935538: Switch REST to default to HAL.

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

Anonymous’s picture

Issue summary: View changes

Rearranged _embedded

effulgentsia’s picture

This patch introduced some @todo Make the base path configurable. comments, but with no issue link. I'm proposing to remove these comments in #2423951: Document each use of base: in rest.module. Please comment there if you disagree with that.

effulgentsia’s picture

+++ b/core/modules/hal/lib/Drupal/hal/Tests/NormalizeTest.php
@@ -0,0 +1,179 @@
+    $this->assertFalse(isset($normalized['id']), 'Internal id is not exposed.');

#2304849: Stop excluding local ID and revision fields from HAL output proposes to expose the internal ID. What were the reasons for not wanting it exposed? Please comment on that issue. Thanks.