Support deserialization for hal+json (needs more language handling tests)

+++ b/core/modules/hal/lib/Drupal/hal/Tests/DenormalizeTest.php
@@ -0,0 +1,169 @@
+    try {
+      $this->container->get('serializer')->denormalize($data_with_invalid_type,   $this->entityClass, $this->format);

superfluous white space.

I did some manual testing with REST module and creating entity_test entities, which works. So I guess we can move forward here, RTBC.

Crell, what say you?

Only the first comment is commit-blocking, probably. This all makes sense to the extent that I understand what all has been written. :-) I'm still behind in my understanding here.

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/EntityNormalizer.php
@@ -62,6 +63,63 @@ public function normalize($entity, $format = NULL, array $context = array()) {
+    $type_uri = $data['_links']['type']['href'];

I think we need to account for type being an array. There's no reason it should ever be multi-value, but from discussion on the HAL list it looks like we should account for *any* link being an array at any time.

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FieldNormalizer.php
@@ -53,6 +56,34 @@ public function normalize($field, $format = NULL, array $context = array()) {
+      $count = $field->isEmpty() ? 0 : $field->count();

If this is using the Countable interface in PHP, we should be able to do count($field), and defaulting to 0 should be handled internally there.

If that's not the case, then this patch can continue but then we need to go change Fields to use Countable properly. :-)

+++ b/core/modules/hal/lib/Drupal/hal/Tests/DenormalizeTest.php
@@ -0,0 +1,169 @@
+    $denormalized = $this->container->get('serializer')->denormalize($data_with_valid_type, $this->entityClass, $this->format);
+    $this->assertEqual(get_class($denormalized),   $this->entityClass, 'Request with valid type results in creation of correct bundle.');

I'm probably late to this game and should have caught it earlier, but... Containers have no business in unit tests. Wire things up manually. Is this code not actually unit testable?

If there's some dependency that prevents it from being unit testable, we need @todos to mark that so we know how much work we have to do when we finally clean up those parts of core that prevent real unit tests.

+++ b/core/modules/hal/lib/Drupal/hal/Tests/DenormalizeTest.php
@@ -0,0 +1,169 @@
+      $this->fail('Exception thrown when type is invalid.');

Shouldn't this have a negative message for WHY it failed, not what it would have said if it passed? I thought that was the convention.

I might be out of date, but last I remember (from a discussion a month or so ago) was that we wanted to structure our regular json to be the same as our hal+json, just without the _links and _embedded sections. And for us to have full deserialization (including entity references, and lang on field items) working in regular json. Is that still the case? If so, how much overlap would there be between that code and the code here, and does it make sense to do the other one first, and have the code here extend that with just the part specific to _links (and possibly _embedded, though not sure if there's anything relevant to deserialization in _embedded, since it's a duplicate of what already exists in the "normal" part of the json)?

I would rather tackle hal+json first and consider whether to handle deserialization for regular JSON as a separate issue.

Ok. That means a fair amount of the code this patch is adding within HAL module will eventually move somewhere more reusable, but that's fine.

Other than that, after spending some time with this patch, and Lin patiently explaining some HAL concepts to me, I think this looks great other than some nits. I don't consider any of these to be commit blockers (they can be cleaned up in a follow up), but CNW to see if Lin is willing to do one more reroll to address some of them.

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/EntityNormalizer.php
@@ -62,6 +63,58 @@ public function normalize($entity, $format = NULL, array $context = array()) {
+    // Get language. language_default() is used because otherwise the entity
+    // system cannot understand the langcode when working in the DrupalUnitTest
+    // environment and importing data without an explicit langcode.
+    $langcode = isset($data['langcode']) ? $data['langcode'][0]['value'] : language_default();

I think we want to use LANGUAGE_NOT_SPECIFIED here rather than language_default(). language_default() is the language the site should output things into by default, but here we're receiving input, and if the input hasn't specified the language, then that's what LANGUAGE_NOT_SPECIFIED is for. I think we can remove the comment about DrupalUnitTest, since proper language handling isn't just for unit testing.

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/EntityNormalizer.php
@@ -62,6 +63,58 @@ public function normalize($entity, $format = NULL, array $context = array()) {
+    $typed_data_ids = $this->getTypedDataIds($data['_links']['type']);

I don't like the term "typed data ids" for referring to things like 'entity_type' and 'bundle', but looks like we already have code in HEAD (RDF module) that does this, so we can punt that discussion to a follow up.

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/EntityNormalizer.php
@@ -62,6 +63,58 @@ public function normalize($entity, $format = NULL, array $context = array()) {
+    // Get links and remove from data array.
+    $links = $data['_links'];
+    unset($data['_links']);
+    // Get embedded resources and remove from data array.
+    $embedded = array();
+    if (isset($data['_embedded'])) {
+      $embedded = $data['_embedded'];
+      unset($data['_embedded']);
+    }

Can we expand the comments as to why we're removing them from $data? Especially for the _embedded one, let's add a @todo pointing to #1880424: Handle entity references on import.

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/EntityNormalizer.php
@@ -62,6 +63,58 @@ public function normalize($entity, $format = NULL, array $context = array()) {
+      $class = get_class($field);

Setting $class here overrides the function parameter of the same name. Not a functional problem, since we no longer use the original parameter, but confusing. Perhaps calling it $field_class would be clearer?

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/EntityNormalizer.php
@@ -62,6 +63,58 @@ public function normalize($entity, $format = NULL, array $context = array()) {
+      // Pass in the empty field object as a target instance. Since the context
+      // is already prepared for the field, any data added to it is
+      // automatically added to the entity.
+      $context['target_instance'] = $field;
+      $this->serializer->denormalize($field_data, $class, $format, $context);

The name 'target_instance' tripped me up, because:
1) Seeing it next to $field made me think of what $instance is within Field API, which is *the configuration* of a field specific to a particular bundle. That's not what 'instance' is referring to here though: here it just means an object we want the denormalizer we're about to call to populate rather than instantiating a new one.
2) I'm not clear what 'target' is intending to convey. Entityreference module uses the term 'target' to refer to the referenced entity, but this code has nothing to do with entity references.

Therefore, how about simply $context['object']?

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FieldItemNormalizer.php
@@ -38,4 +40,72 @@ public function normalize($field_item, $format = NULL, array $context = array())
+      if ($field_definition['translatable'] == TRUE) {

"== TRUE" is redundant and can be removed.

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FieldItemNormalizer.php
@@ -38,4 +40,72 @@ public function normalize($field_item, $format = NULL, array $context = array())
+        $field_item = $this->createTranslatedInstance($field_item, $langcode);

Can we rename this method to getTranslatedFieldItem()?

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FieldItemNormalizer.php
@@ -38,4 +40,72 @@ public function normalize($field_item, $format = NULL, array $context = array())
+    $parent = $field_item->getParent();

Lets call this $field instead of $parent.

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FieldItemNormalizer.php
@@ -38,4 +40,72 @@ public function normalize($field_item, $format = NULL, array $context = array())
+    // Get the property path.
+    while (!method_exists($parent, 'getTranslation')) {
+      array_unshift($ancestors, $parent);
+      $parent = $parent->getParent();
+    }

I think it's ok for us to know Drupal's data model of entity/field/item, and replace this with simply $entity = $field->getParent(). However, if there's some reason we want flexibility to accommodate other data models here, then at least let's replace the method_exists() with instanceof. The interface is Drupal\Core\TypedData\TranslatableInterface.

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/NormalizerBase.php
@@ -36,6 +37,22 @@ public function supportsNormalization($data, $format = NULL) {
+      $target = new \ReflectionClass($type);
+      $supported = new \ReflectionClass($this->supportedInterfaceOrClass);
+      if ($supported->isInterface()) {
+        return $target->implementsInterface($this->supportedInterfaceOrClass);
+      }
+      else {
+        return ($target->getName() == $this->supportedInterfaceOrClass || $target->isSubclassOf($this->supportedInterfaceOrClass));
+      }

Drupal now requires PHP 5.3.10, so this can all be replaced with is_a($type, $this->supportedInterfaceOrClass, TRUE).

+++ b/core/modules/hal/lib/Drupal/hal/Tests/DenormalizeTest.php
@@ -0,0 +1,185 @@
+    $this->assertTrue(!empty($no_field_value) && empty($empty_field_value), 'A field set to an empty array in the data is structured differently than an empty field.');

Why are we only testing $no_field_value for not being empty? What do we actually expect it to be?

Using LANGUAGE_NOT_SPECIFIED makes it so that the call to $denormalized->get('field_test_translatable_text')->getValue() fails

Fixed that. The problem is just one of consistency. If langcode isn't specified, then 'en' values are translations, not default language values. So we can either change the test to identify the resource as 'en', or we can remove 'en' from the data items we want to assert as nontranslated ones. I suppose in principle we should test both variations, but here, I just arbitrarily picked the latter.

Can you post a patch for the fixes you made in #22, and then I think the rest can be a follow up.

My understanding is we can't call the REST web services stuff complete unless this is done, so marking this as major.

This was RTBC already in #12. It has only improved since then. I'm ok with my nits from #21 being addressed in a follow up patch. Getting this in will help resume progress on #1880424: Handle entity references on import. Therefore, I suggest committing #23 and then setting this issue back to needs work with normal priority.

It seems like the major portions of eff's review (namely the language handling stuff) were addressed here. Looks like lin meant to upload a patch in #22, but it didn't quite make it. In the interests of moving things along with web services, though, I think it makes sense to commit this and then push the rest (ha, get it?) of the more minor things to a follow-up issue.

Committed and pushed to 8.x. Thanks!

Here's @linclark's fixes from #22 extracted out of #1935538-17: Switch REST to default to HAL.

Well that's easy enough.

Committed and pushed to 8.x. Thanks!

Thanks! Back to needs work for #27. That shouldn't be too hard: I'll try to post a patch here soon.

proposed resolution.

Sigh. This was filed against the wrong component. This was supposed to be a mandatory follow-up for #1924220: Support serialization in hal+json, but obviously it was never completed.

Because, yes, this is true:

Doesn't this mean that we don't actually test that we can ingest the data that we export?

It's very much broken. We're dealing with the fallout of this over at #2135829: [PP-1] EntityResource: translations support and #2904423: Translated field denormalization creates duplicate values.

The hal module has moved out of Drupal Core and into a Contrib Module.
Moving this issue to the Contrib Module queue.