Config entity references broken and other bugs

ok, merged in changes from #2135573: Hal Entity Normalizer assumes bundle key is always 'type' to berdir's patch here. Let's see how this gets on. I think it should pass ok actually.

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/ContentEntityNormalizer.php
@@ -61,21 +80,21 @@ public function normalize($entity, $format = NULL, array $context = array()) {
     // If the properties to use were specified, only output those properties.
     // Otherwise, output all properties except internal ID.
     if (isset($context['included_fields'])) {
-      $properties = array();
-      foreach ($context['included_fields'] as $property_name) {
-        $properties[] = $entity->get($property_name);
+      $fields = array();
+      foreach ($context['included_fields'] as $field_name) {
+        $fields[] = $entity->get($field_name);
       }
     }

The comment still speaks of properties, that should be changed to fields as well?

Any reason why the new test is not a PHPUnit test? Maybe it makes sense to have this kind of integration test, so I'm just asking. Otherwise looks reasonable to me.

How about that?

We can discuss changing the NormalizerTestBase class in a follow up. That should be be the concern of this issue.

Looks good after reviewing the patch one more time.

Great. Thanks!

7: 2219795-7.patch queued for re-testing.

This looks like a great clean-up! Here some comments:

1. +++ b/core/modules/hal/lib/Drupal/hal/Normalizer/EntityReferenceItemNormalizer.php
   @@ -57,6 +58,12 @@ public function normalize($field_item, $format = NULL, array $context = array())
   +    // If this is not a content entity, let the parent implementation handle it,
   +    // only content entities are supported as embedded resources.
   ...
   +      return parent::normalize($field_item, $format, $context);
   

   Sounds like this would be better handled in supportsNormalization() ? Probably not a big deal anyway.

2. +++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FieldItemNormalizer.php
   @@ -99,28 +99,19 @@ protected function constructValue($data, $context) {
   +    // Create a new instance and return it.
   +    $count = $field_items_translation->isEmpty() ? 0 : $field_items_translation->count();
   +    return $field_items_translation->get($count);
   

   I'm not 100% sure one can rely on all numeric deltas being there, i.e. count == index of next item. As alternative - it supports the [] syntax, but then you won't get back the item. So when this is problematic (nothing introduced here), does that mean we should introduce a public appendItem() which returns the new item?

3. +++ b/core/modules/serialization/serialization.services.yml
   @@ -39,3 +39,7 @@ services:
   +  serialization.entity_resolver.target_id:
   ...
   +    tags:
   

   Wondering how that introduction is related or not?

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/ContentEntityNormalizer.php
@@ -113,14 +132,24 @@ public function denormalize($data, $class, $format = NULL, array $context = arra
-    $entity = entity_create($typed_data_ids['entity_type'], array('langcode' => $langcode, 'type' => $typed_data_ids['bundle']));

uhm - great to see this fixed.

Sounds like this would be better handled in supportsNormalization() ? Probably not a big deal anyway.

Yeah :/ We would have to get around this by just having two normalizers. with something like:

    if ($field_item->get('entity')->getValue() instanceof ContentEntityInterface) {
      return TRUE;
    }

So not sure this IS really worth it tbh. We don't gain much, and it will just be slightly slower.

Thanks for the clarifications! ad #15: Yeah, and as berdir pointed out it wouldn't work even!
So this is great as it is :-)

Committed and pushed to 8.x

diff --git a/core/modules/hal/lib/Drupal/hal/Normalizer/FileEntityNormalizer.php b/core/modules/hal/lib/Drupal/hal/Normalizer/FileEntityNormalizer.php
index 0433ee7..36b650d 100644
--- a/core/modules/hal/lib/Drupal/hal/Normalizer/FileEntityNormalizer.php
+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FileEntityNormalizer.php
@@ -9,7 +9,6 @@
 
 use Drupal\Core\Entity\EntityManagerInterface;
 use Drupal\Core\Extension\ModuleHandlerInterface;
-use Drupal\file\Plugin\Core\Entity\File;
 use Drupal\rest\LinkManager\LinkManagerInterface;
 use Guzzle\Http\ClientInterface;
 
diff --git a/core/modules/hal/lib/Drupal/hal/Tests/NormalizerTestBase.php b/core/modules/hal/lib/Drupal/hal/Tests/NormalizerTestBase.php
index 1b12a18..7f859a1 100644
--- a/core/modules/hal/lib/Drupal/hal/Tests/NormalizerTestBase.php
+++ b/core/modules/hal/lib/Drupal/hal/Tests/NormalizerTestBase.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\hal\Tests;
 
-use Drupal\Component\Uuid\Uuid;
 use Drupal\Core\Cache\MemoryBackend;
 use Drupal\Core\Language\Language;
 use Drupal\hal\Encoder\JsonEncoder;

Removed these unused uses during commit.

The issue summary says:

There's also the question of the revision_id, which we should then exclude as well.

And the patch committed here does that, but I don't see any comments explaining why. #2304849: Stop excluding local ID and revision fields from HAL output proposes to stop excluding revision ID. Please comment there with objections to that if you have any.