MenuLinkContent entities pointing to nodes are not deployable: LinkItem should have a "target_uuid" computed property

Personally I think getting this fixed before RC1 is going to be hard, so we should push the quickest way for now, and even if we do or don't get it in 8.0.0 we work on the "best" way for 8.1.0.

Option 3 sounds like the most feasible approach right now. We should add a uuid property in the JSON that contains the node/entity UUID. I would not mess with the uri property since that would be an API change if it suddenly contains UUIDs. On denormailze() we can lookup the ID from the UUID and replace it.

Well, we're past RC now. :-) My concern with option 3 is that only fixes the problem when going through the serializer. Is that the only place this problem manifests? Are you certain?

Few minor points below

1. +++ b/core/modules/menu_link_content/src/MenuLinkContentServiceProvider.php
   @@ -0,0 +1,34 @@
   +        new Reference('entity.manager'),
   

   Do we want to use the now deprecated entity manager or inject entity_type and entity.repository.

2. +++ b/core/modules/menu_link_content/src/Normalizer/MenuLinkContentNormalizer.php
   @@ -0,0 +1,144 @@
   +          if ($target_entity = $this->entityManager->getStorage($target_entity_type_id)->load($target_entity_id)) {
   ...
   +          // Entity-type not found.
   +          continue;
   

   Could we use $manager->hasHandler('storage') here? Might read a little clearer than try/catch

I'm wondering why this is specific to hal serialization. The same problem should existing in JSON and XML etc. as well?

+++ b/core/modules/menu_link_content/src/Normalizer/MenuLinkContentNormalizer.php
@@ -0,0 +1,144 @@
+        try {
+          if ($target_entity = $this->entityManager->getStorage($target_entity_type_id)->load($target_entity_id)) {
+            $normalized = $this->embedEntity($entity, $format, $context, $target_entity, $normalized, self::PSUEDO_FIELD_NAME);
+            $normalized['link'][$key] += [
+              'target_uuid' => $target_entity->uuid(),
+            ];
+          }
+        }
+        catch (PluginNotFoundException $e) {
+          // Entity-type not found.
+          continue;
+        }

If this is the only case we could check with an if whether the entity type exists already. Do you prefer one style over the other?

Added this issue as a use case in #2690747: [PLAN] Create an index of UUIDs.

Also, switching this to needs work, the main thing I noticed in the patch was the use of EntityManger which is not deprecated, as noted in #11.

Looks good to me just minor issues.

1. +++ b/core/modules/menu_link_content/src/MenuLinkContentServiceProvider.php
   @@ -0,0 +1,44 @@
   +      $service_definition = new Definition('Drupal\menu_link_content\Normalizer\HalMenuLinkContentNormalizer', [
   ...
   +      $service_definition = new Definition('Drupal\menu_link_content\Normalizer\MenuLinkContentNormalizer', [
   
   +++ b/core/modules/menu_link_content/src/Normalizer/HalMenuLinkContentNormalizer.php
   @@ -0,0 +1,140 @@
   +  protected $supportedInterfaceOrClass = 'Drupal\menu_link_content\MenuLinkContentInterface';
   
   +++ b/core/modules/menu_link_content/src/Normalizer/MenuLinkContentNormalizer.php
   @@ -0,0 +1,70 @@
   +  protected $supportedInterfaceOrClass = 'Drupal\menu_link_content\MenuLinkContentInterface';
   

   We can use ::class here.

2. +++ b/core/modules/menu_link_content/src/Normalizer/HalMenuLinkContentNormalizer.php
   @@ -0,0 +1,140 @@
   +        $scheme = parse_url($link['uri'], PHP_URL_SCHEME);
   ...
   +        $path = parse_url($link['uri'], PHP_URL_PATH);
   ...
   +        $scheme = parse_url($link['uri'], PHP_URL_SCHEME);
   ...
   +        $path = parse_url($link['uri'], PHP_URL_PATH);
   
   +++ b/core/modules/menu_link_content/src/Normalizer/MenuLinkContentNormalizer.php
   @@ -0,0 +1,70 @@
   +        $scheme = parse_url($link['uri'], PHP_URL_SCHEME);
   ...
   +        $path = parse_url($link['uri'], PHP_URL_PATH);
   

   We should combine these calls. Store the return value in an array.

3. +++ b/core/modules/menu_link_content/src/Normalizer/HalMenuLinkContentNormalizer.php
   @@ -0,0 +1,140 @@
   +        if (strpos($parent['value'], PluginBase::DERIVATIVE_SEPARATOR) !== FALSE) {
   

   Can we please add a comment here explaining this?

Seems ready to me.

Nice test coverage!

1. +++ b/core/modules/menu_link_content/src/MenuLinkContentServiceProvider.php
   @@ -0,0 +1,46 @@
   +    if (isset($modules['hal'])) {
   +      // Add a hal normalizer service for menu-link-content entities.
   +      $service_definition = new Definition(HalMenuLinkContentNormalizer::class, [
   +        new Reference('rest.link_manager'),
   +        new Reference('entity.manager'),
   +        new Reference('module_handler'),
   +      ]);
   +      // The priority must be higher than that of
   +      // serializer.normalizer.entity.hal in hal.services.yml.
   +      $service_definition->addTag('normalizer', array('priority' => 40));
   +      $container->setDefinition('menu_link_content.normalizer.menu_link_content.hal', $service_definition);
   +    }
   +    elseif (isset($modules['serialization'])) {
   +      // Add a generic normalizer service for menu-link-content entities.
   +      $service_definition = new Definition(MenuLinkContentNormalizer::class, [
   +        new Reference('entity.manager'),
   +      ]);
   +      // The priority must be higher than that of
   +      // serializer.normalizer.content_entity in serialization.services.yml.
   +      $service_definition->addTag('normalizer', array('priority' => 10));
   ...
   +    }
   

   Don't we want both, you might want to support HAL and another format

2. +++ b/core/modules/menu_link_content/src/Normalizer/MenuLinkContentNormalizer.php
   @@ -0,0 +1,70 @@
   +/**
   + * @file
   + * Contains Drupal\menu_link_content\Normalizer\MenuLinkContentNormalizer.
   + */
   

   Let's not add it

Nice work!

Menu link content entities when normalized contain references to node serial IDs (nid) and not UUIDs, this means they cannot be deployed.

Isn't this a problem for every entity type that uses a link field? Why are fixing only menu_link_content here instead of fixing the link field directly?

Good point!

+++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
@@ -12,6 +12,8 @@
 class EntityReferenceItemNormalizer extends FieldItemNormalizer implements UuidReferenceInterface {

@larowlan do these changes affect DER?

I completely agree with #23. This makes much more sense now.

The issue summary could do with an update as it is not clear what the changes are and also whether we should do this or #2353611: Make it possible to link to an entity by UUID - or do both.

Some tiny tiny nits but because i don't know what to change the test comment too setting to needs work. @larowlan please rtbc once fixed.

+++ b/core/modules/menu_link_content/tests/src/Kernel/Normalizer/HalMenuLinkContentNormalizerTest.php
@@ -0,0 +1,100 @@
+  /**
+   * Tests prepare passengers event.
+   */

+++ b/core/modules/menu_link_content/tests/src/Kernel/Normalizer/MenuLinkContentNormalizerTest.php
@@ -0,0 +1,85 @@
+  /**
+   * Tests prepare passengers event.
+   */

What event is this?

diff --git a/core/modules/hal/src/Normalizer/EntityReferenceEmbeddingTrait.php b/core/modules/hal/src/Normalizer/EntityReferenceEmbeddingTrait.php
index b8d6507..2424f43 100644
--- a/core/modules/hal/src/Normalizer/EntityReferenceEmbeddingTrait.php
+++ b/core/modules/hal/src/Normalizer/EntityReferenceEmbeddingTrait.php
@@ -1,8 +1,4 @@
 <?php
-/**
- * @file
- * Contains Drupal\hal\Normalizer\EntityReferenceEmbeddingTrait.
- */
 
 namespace Drupal\hal\Normalizer;
 
diff --git a/core/modules/menu_link_content/src/Normalizer/HalMenuLinkContentNormalizer.php b/core/modules/menu_link_content/src/Normalizer/HalMenuLinkContentNormalizer.php
index f9db016..ed66761 100644
--- a/core/modules/menu_link_content/src/Normalizer/HalMenuLinkContentNormalizer.php
+++ b/core/modules/menu_link_content/src/Normalizer/HalMenuLinkContentNormalizer.php
@@ -4,7 +4,6 @@
 
 use Drupal\Component\Plugin\PluginBase;
 use Drupal\Component\Utility\NestedArray;
-use Drupal\Core\Entity\EntityInterface;
 use Drupal\hal\Normalizer\ContentEntityNormalizer;
 use Drupal\hal\Normalizer\EntityReferenceEmbeddingTrait;
 use Drupal\menu_link_content\MenuLinkContentInterface;

Some coding standards that need fixing.

1. +++ b/core/modules/link/src/LinkServiceProvider.php
   @@ -0,0 +1,46 @@
   + */
   +class LinkServiceProvider implements ServiceModifierInterface {
   ...
   +  public function alter(ContainerBuilder $container) {
   

   Does this need to be an alter? I think we're adding services here. I guess we're copying \Drupal\menu_link_content\MenuLinkContentServiceProvider but I'm not sure this is right. Do we need to do this for the container.modules param?

2. +++ b/core/modules/link/src/LinkServiceProvider.php
   @@ -0,0 +1,46 @@
   +      $container->setDefinition('serializer.normalizer.link_item.hal', $service_definition);
   ...
   +      $container->setDefinition('serializer.normalizer.link_item', $service_definition);
   

   These services should be called 'link.SOMETHING' because they are provided by the link module.

It is a bug so it can be committed to 8.1.x. RTBC as #45 and #41 are addressed.

1. +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
   @@ -12,6 +12,8 @@
   +  use EntityReferenceEmbeddingTrait;
   +
   

   This is not a change allowed in a patch release. And the beta is closed by now so I honestly think this is only eligible for 8.3.x

2. +++ b/core/modules/menu_link_content/src/Normalizer/HalMenuLinkContentNormalizer.php
   @@ -0,0 +1,51 @@
   +  /**
   +   * Psuedo field name for embedding parent target entity.
   +   *
   +   * @var string
   +   */
   +  const PSUEDO_PARENT_FIELD_NAME = 'menu_link_content_parent_entity';
   

   PSEUDO no?

3. +++ b/core/modules/link/src/Normalizer/HalLinkItemNormalizer.php
   @@ -0,0 +1,115 @@
   +        list($target_entity_type_id, $target_entity_id) = explode('/', $path);
   ...
   +      list($target_entity_type_id, $target_entity_id) = explode('/', $path);
   

   No point declaring $target_entity_id

4. +++ b/core/modules/link/src/Normalizer/HalLinkItemNormalizer.php
   @@ -0,0 +1,115 @@
   +      if (isset($link['target_uuid'])) {
   

   This can be checked in the first if in the this method. Less indentation...

5. +++ b/core/modules/menu_link_content/src/Normalizer/MenuLinkContentNormalizer.php
   @@ -0,0 +1,41 @@
   +        if (isset($link['target_uuid'])) {
   

   This can be checked earlier to do less in the foreach.

As https://www.previousnext.com.au/blog/we-could-add-default-content-drupal… mentions, this also affects the REST experience.

AFAICT #49 addressed #48's feedback. I agree with #49.1 though, I also don't fully understand why that change is problematic. It's fully BC. However, I think that the mere introduction of a new trait is not acceptable. Although I'm not sure how that is a helpful rule in this case. Alex, can you confirm that?

I'd RTBC, but have further feedback myself:

1. The main critical question: doesn't this mean that the link a menu link points to becomes somewhat meaningless, because target_uuid can "override" it? Wouldn't it be better to just land #2353611: Make it possible to link to an entity by UUID, ensure that provides an upgrade path for all existing menu link content entities, and then this problem would also be solved?

2. +++ b/core/modules/hal/src/Normalizer/EntityReferenceEmbeddingTrait.php
   @@ -0,0 +1,66 @@
   +   *   Entity being reference.
   

   Nit: s/reference/referenced/

3. +++ b/core/modules/hal/src/Normalizer/EntityReferenceEmbeddingTrait.php
   @@ -0,0 +1,66 @@
   +   *   Rest link manager service.
   

   Nit: s/Rest/REST/

4. +++ b/core/modules/hal/src/Normalizer/EntityReferenceEmbeddingTrait.php
   @@ -0,0 +1,66 @@
   +    return array(
   

   Nit: Why not use [] notation for this new trait?

5. +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
   --- /dev/null
   +++ b/core/modules/link/src/LinkServiceProvider.php
   
   +++ b/core/modules/link/src/LinkServiceProvider.php
   @@ -0,0 +1,46 @@
   +    if (isset($modules['hal'])) {
   +      // Add a hal normalizer service for LinkItem fields.
   ...
   +    if (isset($modules['serialization'])) {
   

   Hm this is kind of … painful. Can't this just live in hal module and link module, respectively? Without the conditionality?

6. +++ b/core/modules/menu_link_content/src/MenuLinkContentServiceProvider.php
   @@ -0,0 +1,46 @@
   +    if (isset($modules['hal'])) {
   ...
   +    if (isset($modules['serialization'])) {
   

   Same concerns here.

7. +++ b/core/modules/menu_link_content/tests/src/Kernel/Normalizer/MenuLinkContentNormalizerTest.php
   @@ -0,0 +1,85 @@
   +    $normalized = $serializer->normalize($link, 'xml');
   ...
   +    $denormalized = $serializer->denormalize($normalized, MenuLinkContent::class, 'json');
   

   Nit: should be consistent, both 'json'?

Addressing @Wim Leers' review in #52

2,3,4,7 Fixed nits

5,6 Won't the hal and serialization ServiceProviders modules get complicated if they have a to register every other modules' normalization services. Also won't those ServiceProviders classes have their own if clauses because they would need to check if Link module is enabled. For instance for this
$service_definition = new Definition(HalLinkItemNormalizer::class, [

1, @larowlan you would be better to answer the question about #2353611: Make it possible to link to an entity by UUID

#54.6: You're right, I'm not sure what I was thinking. What I meant to say: can't we unconditionally define those, which means we can just define them in link.services.yml and in menu_link_content.services.yml. The services won't be instantiated if no REST is used, and hence would also never cause errors. They're just available for discovery/use.

#55: Can you then add that justification at least to the IS, and preferably also in some sensible location in the patch? So that our future selves won't get confused by this. The test coverage seems a prime location for this. i.e. doesn't this mean that the link a menu link points to becomes somewhat meaningless, because target_uuid can "override" it? + the serial ID will remain the default link representation (canonical). + other relevant information.

Since no more 8.1.x releases are happening, this won't be backported to 8.1.x anymore. Removing issue tag.

RE #54.6:
Moved services out of ServiceProvider Classes and in to *.services.yml files.

So there we go, that's what we want to document on this service provider then! :)

Ok doing my interdiff against #54 because that is what started from.

1. So we need a service provider for 2 of the services. But for 1 of the services in each module we can use *.services.yml so I used those.
re

So there we go, that's what we want to document on this service provider then! :)

Added a comment in each ServiceProvider
2. also re @alexpott's comment in #41 about LinkServiceProvider implements ServiceModifierInterface

Does this need to be an alter? I think we're adding services here. I guess we're copying \Drupal\menu_link_content\MenuLinkContentServiceProvider but I'm not sure this is right. Do we need to do this for the container.modules param?

LinkServiceProvider was changed to use ServiceProviderInterface but it is not clear why MenuLinkContentServiceProvider is using ServiceModifierInterface in the first placeand not ServiceProviderInterface. Since MenuLinkContentServiceProvider is new in this patch it seems it should also use ServiceProviderInterface.

So changed that. Maybe someone will point out something I missed.

It is ready imo.

AFAICT this was never addressed:

#55: Can you then add that justification at least to the IS, and preferably also in some sensible location in the patch? So that our future selves won't get confused by this. The test coverage seems a prime location for this. i.e. doesn't this mean that the link a menu link points to becomes somewhat meaningless, because target_uuid can "override" it? + the serial ID will remain the default link representation (canonical). + other relevant information.

Please think about out future selves.

Please think about out future selves.

Pausing to think of our future selves...

Ok here is a patch that adds comments about the the process and how ids are used for canonical links but recreated on denormalization based on UUID.

1. +++ b/core/modules/link/src/LinkServiceProvider.php
   @@ -0,0 +1,46 @@
   +  public function register(ContainerBuilder $container) {
   ...
   +    if (isset($modules['hal'])) {
   ...
   +    if (isset($modules['serialization'])) {
   

   ubernitpick. I would have started with $modules['serialization'] and then use 'hal', giving that the serialization is the more generic module than the hal one.

2. +++ b/core/modules/link/src/Normalizer/HalLinkItemNormalizer.php
   @@ -0,0 +1,112 @@
   +class HalLinkItemNormalizer extends FieldItemNormalizer {
   
   +++ b/core/modules/link/src/Normalizer/LinkItemNormalizer.php
   @@ -0,0 +1,59 @@
   +class LinkItemNormalizer extends ComplexDataNormalizer {
   

   Is there a reason the link/generic implementation doesn't support denormalize()? Maybe we should document this here

3. +++ b/core/modules/menu_link_content/src/MenuLinkContentServiceProvider.php
   @@ -0,0 +1,46 @@
   +  public function alter(ContainerBuilder $container) {
   ...
   +    if (isset($modules['hal'])) {
   ...
   +    if (isset($modules['serialization'])) {
   

   Same thing as above.

4. +++ b/core/modules/menu_link_content/tests/src/Kernel/Normalizer/HalMenuLinkContentNormalizerTest.php
   @@ -0,0 +1,100 @@
   +
   +namespace Drupal\Tests\menu_link_content\Kernel\Normalizer;
   +
   
   +++ b/core/modules/menu_link_content/tests/src/Kernel/Normalizer/MenuLinkContentNormalizerTest.php
   @@ -0,0 +1,90 @@
   +
   +namespace Drupal\Tests\menu_link_content\Kernel\Normalizer;
   +
   

   One more thing: I'd expected to see a test in link and hal module as well.

#65: Thanks! Nits for that interdiff:

+++ b/core/modules/link/src/Normalizer/LinkItemNormalizer.php
@@ -46,6 +46,10 @@ public function normalize($object, $format = NULL, array $context = array()) {
+        // create the correct 'uri' for the entity even when the entity id has

+++ b/core/modules/menu_link_content/src/Normalizer/MenuLinkContentNormalizer.php
@@ -7,6 +7,12 @@
+ * on demoralization the 'uri' of the link will be updated to use the current id
...
+ * entities they will link to the correct entities even if the entity ids have

+++ b/core/modules/menu_link_content/tests/src/Kernel/Normalizer/MenuLinkContentNormalizerTest.php
@@ -71,10 +71,15 @@ public function testMenuLinkNormalizer() {
+    // entity's id has changed such as in a deployment.
...
+    // the same node. The canonical links for entities use the entity id but the
...
+    // entity id.

Nit: s/id/ID/

#66: Thanks for the review, @dawehner! Glad that you're also taking a look at this :)

@dawehner and @Wim Leers thanks for reviews!

#67
1,3 fixed.

2.

Is there a reason the link/generic implementation doesn't support denormalize()? Maybe we should document this here

@dawehner good question!
I think supporting denormalize() is a mistake it means that deploy denormalization benefit will only work for MenuLinkContent entities not other fieldable entities that have Link fields.

This patch adds a test for this which should fail currently. I will now work on patch that added denormalize().

Ok here is patch that should the test introduced in #69 working. Hopefully it does break to many other ;)

I found the problem:

1. \Drupal\serialization\Normalizer\ContentEntityNormalizer didn't override denormalize
2. \Drupal\serialization\Normalizer\EntityNormalizer::denormalize was simply creating the entity like $entity = $this->entityManager->getStorage($entity_type_id)->create($data);
3. For this reason no denomarlizers on FieldItem Normalizers were being called.
4. I had to introduced \Drupal\serialization\Normalizer\FieldNormalizer and \Drupal\serialization\Normalizer\FieldItemNormalizer which are very close copies of the HAL versions of these normalizers.
5. All of the above allowed deleting MenuLinkContentNormalizer because the MenuLinkContent entity no longer needs a denormalizer as this can be done on the field item level.
6. \Drupal\menu_link_content\Normalizer\HalMenuLinkContentNormalizer is still necessary because it needs to handle 'parent' which a string but references the plugin id of the menu item so it can be handled generically.

Some questions/thoughts

1. I would image that all the changes related to serialization should really be done in separate issue against the serialization module, something like Support denomorlization on fielditem normlizers
2. Is all the above really necessary are other entities going to use the link field with the "entity" scheme. If add configurable link field to an entity now and point to /node/1 the scheme is internal not "entity". This also true as far as I can tell if you make a regular menu item. It only uses "entity" scheme when added on the node form.
3. Are their other fields where having a denormalizer would be useful.

Not surprised there were bunch of test failures. Before I look into fixing them.

Anybody have feedback on whether this is useful change or should we revert back to approach of #66?

1. +++ b/core/core.services.yml
   @@ -1345,7 +1345,7 @@ services:
   -    arguments: ['@module_handler', '@cache.discovery', '@language_manager', '@cache_tags.invalidator', '@renderer']
   +    arguments: ['@module_handler', '@cache.default', '@language_manager', '@cache_tags.invalidator', '@renderer']
   

   This is from #2824548: Move token info cache to data cache bin. Should be removed from this patch.

2. +++ b/core/modules/link/src/LinkServiceProvider.php
   @@ -27,7 +27,7 @@ public function register(ContainerBuilder $container) {
   -      $service_definition->addTag('normalizer', array('priority' => 10));
   +      $service_definition->addTag('normalizer', array('priority' => 30));
   

   Let's document why this needs priority 30.

3. +++ b/core/modules/serialization/serialization.services.yml
   @@ -74,3 +74,11 @@ services:
   +        - { name: normalizer, priority: 11 }
   ...
   +      - { name: normalizer, priority: 11 }
   

   Let's document the rationale behind these priorities.

4. +++ b/core/modules/serialization/src/Normalizer/ContentEntityNormalizer.php
   @@ -32,4 +32,26 @@ public function normalize($object, $format = NULL, array $context = array()) {
   +  public function denormalize($data, $class, $format = NULL, array $context = []) {
   
   +++ b/core/modules/serialization/src/Normalizer/FieldItemNormalizer.php
   @@ -0,0 +1,93 @@
   +class FieldItemNormalizer extends ComplexDataNormalizer implements DenormalizerInterface {
   
   +++ b/core/modules/serialization/src/Normalizer/FieldNormalizer.php
   @@ -0,0 +1,101 @@
   +class FieldNormalizer extends NormalizerBase implements DenormalizerInterface {
   

   What I don't understand yet is how this then used to work until now?

5. +++ b/core/modules/serialization/src/Normalizer/FieldNormalizer.php
   index fc85050..d06ae66 100644
   --- a/core/modules/views/tests/src/Kernel/Plugin/ViewsBlockTest.php
   

   This is also from an unrelated issue.

• #72.1: Yes, but then we'll need test coverage to prove why it's necessary. It's still a mystery to me why this has worked fine so far, why this is only being discovered here.
• #72.2: Any link field can use entity: URIs AFAIK? So even if by default it'll only be used in NodeForm, that can change when contrib starts using it.
• #72.3: Yes! We have separate issues for those. For example, #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX.

@Wim Leers thanks for review

I create #2827218: Denormalization on field items is never called: add FieldNormalizer + FieldItemNormalizer with denormalize() methods to solve the FieldItem denormalization problem first

Postponing this 1 on that

#2827218: Denormalization on field items is never called: add FieldNormalizer + FieldItemNormalizer with denormalize() methods landed, this is now unblocked!

Ok then is just a re-roll of #71 without the serialization module changes as they were committed as part of #2827218: Denormalization on field items is never called: add FieldNormalizer + FieldItemNormalizer with denormalize() methods. Yay!!!

I tested the 3 tests this patch provides locally and they passed but presumably other tests will fail. Let's see.

Some @Wim Leers' review #74 no longer applies. The comments about documenting the priorities probably do will need to see what they will be. Might have to change the current ones and re-read this issue for numbers used so far.

#2827218: Denormalization on field items is never called: add FieldNormalizer + FieldItemNormalizer with denormalize() methods was committed to 8.3.x only, which makes it pretty difficult to backport this. If we need to backport this, then we shouldn't have the Needs backport ot 8.2.x tag, but we should just mark this against 8.2.x-dev.

EDIT: forgot to say, in #2827218-62: Denormalization on field items is never called: add FieldNormalizer + FieldItemNormalizer with denormalize() methods, I changed that issue from "Fixed" in 8.3.x to "RTBC" in 8.2.x. The patch applies cleanly. Fingers crossed.

This looks really tidy in general. A couple of smalls:

1. +++ b/core/modules/hal/src/Normalizer/EntityReferenceEmbeddingTrait.php
   @@ -0,0 +1,66 @@
   +   * @param \Symfony\Component\Serializer\Normalizer\NormalizerInterface $normalizer
   +   *   Normalizer service.
   

   This parameter should be called $serializer. This makes it a bit more confusing IMO. Yes, the serializer class is a hot mess of interfaces :)

   Could also consider just assuming that the serializer and linkManager as available as properties on classes where this trait is used? So you just use $this->serializer->normalize() instead? It makes the function signature more manageable, but does hide those dependencies a little.

2. +++ b/core/modules/link/src/LinkServiceProvider.php
   @@ -0,0 +1,47 @@
   +      // Add a generic normalizer service for LinkItem fields.
   

   This doesn't seem like a generic normalizer, just for LinkItems?

3. +++ b/core/modules/link/src/Normalizer/HalLinkItemNormalizer.php
   @@ -0,0 +1,112 @@
   +  protected $supportedInterfaceOrClass = LinkItem::class;
   

   nit: Can you move this to the top instead (below the trait usage). Makes it easier to scan.

NW for #80.

Ok here is fix for 3 points in #80

Ok #82 had a bunch of test failures for 8.2.x but since comment #84 this won't get committed to 8.2.x anyways.

All the failures from #82 on 8.3.x seem to be because we are using the deprecated REST link manager. #2758897: Move rest module's "link manager" services to serialization module
Since in \Drupal\link\LinkServiceProvider we only check if Hal is enabled not Rest(no longer a dependency) this causes a error.

Changing to use new serialization.link_manager service. This also lets us get rid of any use of "rest" in this module including tests!

Changing to use new serialization.link_manager service. This also lets us get rid of any use of "rest" in this module including tests!

Hurray! :D

Looking back at the last dozen comments, it seems my review in #74 was never addressed. #77 said some of it no longer applies, and that's true. Only point 2 needs to be addressed still!

#74 also replied to the 3 questions at the end of #71.

1. The first question there is no longer relevant; this patch no longer makes any changes to the serialization module.
2. The second question I answered in #74.
3. The third question I answered in #74.

None of these need extra work.

Finally, another re-review now that we have a green patch again:

1. +++ b/core/modules/hal/src/Normalizer/EntityReferenceEmbeddingTrait.php
   @@ -0,0 +1,60 @@
   +trait EntityReferenceEmbeddingTrait {
   

   We moved logic from HAL's EntityReferenceItemNormalizer here, to this trait, so it can be reused by HAL's HalLinkItemNormalizer.

2. +++ b/core/modules/hal/src/Normalizer/EntityReferenceEmbeddingTrait.php
   @@ -0,0 +1,60 @@
   +   * Embeds an entity into normalized data.
   

   s/normalized data/a normalization/

3. +++ b/core/modules/link/src/LinkServiceProvider.php
   @@ -0,0 +1,47 @@
   +      $service_definition->addTag('normalizer', array('priority' => 30));
   

   s/array()/[]/

4. +++ b/core/modules/link/src/Normalizer/HalLinkItemNormalizer.php
   @@ -0,0 +1,112 @@
   +class HalLinkItemNormalizer extends FieldItemNormalizer {
   

   Why not just call this LinkItemNormalizer? The class it extends also does not have a Hal prefix.

5. +++ b/core/modules/link/src/Normalizer/HalLinkItemNormalizer.php
   @@ -0,0 +1,112 @@
   +   * Constructs a new LinkItemNormalizer object.
   

   This ironically already does not have the prefix :)

6. +++ b/core/modules/link/src/Normalizer/HalLinkItemNormalizer.php
   @@ -0,0 +1,112 @@
   +    /** @var \Drupal\link\Plugin\Field\FieldType\LinkItem $field_item */
   +    $field_name = $field_item->getParent()->getName();
   +    $entity = $field_item->getEntity();
   +    $normalized = parent::normalize($field_item, $format, $context);
   

   Let's move the parent::normalize() call to the very top. Because it doesn't require any of the variables created before it's called.

   Also, I think $normalization would be a better name than $normalized.

7. +++ b/core/modules/link/src/Normalizer/HalLinkItemNormalizer.php
   @@ -0,0 +1,112 @@
   +        $url = parse_url($link['uri']);
   +        if (!isset($url['scheme']) || $url['scheme'] !== 'entity') {
   +          continue;
   +        }
   

   This means we're skipping any URI that is not using the entity: scheme.

   It'd be much clearer if we then moved all of the logic that comes after this to a helper method.

   Then the function body would be much clearer:

   function normalize() {
     $normalization = parent::normalize(…);
   
     $field_name = …;
     if (isset($normalization[$field_name]) {
       foreach ($field as $item) {
         $uri = $item->something();
         if ($uri_uses_entity_scheme) {
           $this->decorateWithTargetUuid(…);
         }
       }
     }
   }
   

   i.e. exactly like #2825812: ImageItem should have an "derivatives" computed property, to expose all image style URLs is doing.

   We should also update the docs of LinkItemNormalizer, because just like ImageItmeNormalizer in #2825812, this is just decorating an existing normalizer, to deal with the special extra capabilities of this field type!

8. +++ b/core/modules/link/src/Normalizer/HalLinkItemNormalizer.php
   @@ -0,0 +1,112 @@
   +            $normalized[$field_name][$delta]['target_uuid'] = $target_entity->uuid();
   ...
   +      if ($entity = $this->entityRepository->loadEntityByUuid($target_entity_type_id, $link['target_uuid'])) {
   +        $link['uri'] = 'entity:' . $target_entity_type_id . '/' . $entity->id();
   +      }
   
   +++ b/core/modules/link/src/Normalizer/LinkItemNormalizer.php
   @@ -0,0 +1,88 @@
   +        $link['target_uuid'] = $target_entity->uuid();
   ...
   +      if ($entity = $this->entityRepository->loadEntityByUuid($target_entity_type_id, $data['target_uuid'])) {
   +        $data['uri'] = 'entity:' . $target_entity_type_id . '/' . $entity->id();
   +      }
   

   All of this logic is also hugely repetitive. Just like #2825812: ImageItem should have an "derivatives" computed property, to expose all image style URLs has ImageItemNormalizerTrait, used by both serialization and hal modules, we need an LinkItemNormalizerTrait here.

9. +++ b/core/modules/link/src/Normalizer/HalLinkItemNormalizer.php
   @@ -0,0 +1,112 @@
   +    $url = parse_url($link['uri']);
   +    if (isset($url['scheme']) && $url['scheme'] === 'entity' && isset($link['target_uuid'])) {
   
   +++ b/core/modules/link/src/Normalizer/LinkItemNormalizer.php
   @@ -0,0 +1,88 @@
   +    $url = parse_url($link['uri']);
   +    if (!isset($url['scheme']) || $url['scheme'] !== 'entity') {
   ...
   +      $url = parse_url($data['uri']);
   +      if (!isset($url['scheme']) || $url['scheme'] !== 'entity') {
   

   This is the second (and third and fourth) time we have URI scheme parsing logic. This indicates we need a protected static function uriUsesEntityScheme() helper method, that we call everywhere.

@Wim Leers thanks again for the review
#74.2 updated comment about priority

re #86
1. not sure if this needs action or just a comment. HalLinkItemNormalizer is currently using EntityReferenceEmbeddingTrait
2. fixed
3. fixed both array() usages in this file
4(and 5). This class is in the namespace "Drupal\link\Normalizer" which already has a LinkItemNormalizer. The normalizers provided directly by the Hal module don't use the 'Hal' prefix but when other modules provide normalizers they have to differentiate between the 2 normalizers they provide.
#2825812: ImageItem should have an "derivatives" computed property, to expose all image style URLs has the same pattern. But in that patch we chose ImageItemHalNormalizer with "Hal" not as a prefix but before "Normalizer". We should probably use the same pattern in both issues. Either ThingHalNormalizer or HalThingNormalizer

I think I prefer ThingHalNormalizer but not changing now to get feedback.

6. fixed
7. Add the method decorateWithTargetUuid and update the class comment on both normalizers.
8. wow, I actually did this before I got to this comment when fixing above :) I called it EntityLinkItemNormalizerTrait because it all the methods deal with links that are to entities.
9. I had added this to trait above. I used the name isEntityLink will change to uriUsesEntityScheme.
Also added getTargetEntity(), getEntityPathParts(), and updateEntityUri() to this trait.

1. Sorry for not being more clear: this was solely an observation! :)

4. Thanks for pointing that out. Wrote #2825812-64: ImageItem should have an "derivatives" computed property, to expose all image style URLs to fix that there too. Let's drop the Hal mention from the classname altogether, all other HAL normalizers also don't have that! Ah but damn, I realized why this is not possible:
   

   Ah, damn, #64 does not make sense, because we have ImageItemNormalizer for serialization (i.e. default normalizer) and ImageItemHalNormalizer for hal, and both live in the same directory and namespace. That's why #64 is not feasible.

Interdiff review:

1. +++ b/core/modules/link/src/Normalizer/EntityLinkItemNormalizerTrait.php
   @@ -0,0 +1,72 @@
   +   * @param array $link
   +   *   The link info.
   ...
   +   * @param array $link
   +   *   The link info.
   ...
   +   * @param array $link
   +   *   The link info.
   

   Hm, wouldn't $link_normalization and The link normalization. not make more sense?

2. +++ b/core/modules/link/src/Normalizer/EntityLinkItemNormalizerTrait.php
   @@ -0,0 +1,72 @@
   +   *   The linked if available or NULL.
   

   s/linked/linked entity/
   s/available/any/

3. +++ b/core/modules/link/src/Normalizer/EntityLinkItemNormalizerTrait.php
   @@ -0,0 +1,72 @@
   +   *   First item is entity type. The second item is entity id.
   

   s/id/ID/

4. +++ b/core/modules/link/src/Normalizer/HalLinkItemNormalizer.php
   @@ -12,11 +12,21 @@
   + * the ids of the entities may have changed.
   

   s/ids/IDs/

5. +++ b/core/modules/link/src/Normalizer/HalLinkItemNormalizer.php
   @@ -63,50 +73,57 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager, Lin
   -    $url = parse_url($link['uri']);
   -    if (isset($url['scheme']) && $url['scheme'] === 'entity' && isset($link['target_uuid'])) {
   -      $path = isset($url['path']) ? $url['path'] : '';
   -      list($target_entity_type_id,) = explode('/', $path);
   -      if ($entity = $this->entityRepository->loadEntityByUuid($target_entity_type_id, $link['target_uuid'])) {
   -        $link['uri'] = 'entity:' . $target_entity_type_id . '/' . $entity->id();
   -      }
   -    }
   +    $this->updateEntityUri($link);
   
   +++ b/core/modules/link/src/Normalizer/LinkItemNormalizer.php
   @@ -71,17 +75,7 @@ public function normalize($object, $format = NULL, array $context = array()) {
   -    if (isset($data['target_uuid'])) {
   -      $url = parse_url($data['uri']);
   -      if (!isset($url['scheme']) || $url['scheme'] !== 'entity') {
   -        return $data;
   -      }
   -      $path = isset($url['path']) ? $url['path'] : '';
   -      list($target_entity_type_id,) = explode('/', $path);
   -      if ($entity = $this->entityRepository->loadEntityByUuid($target_entity_type_id, $data['target_uuid'])) {
   -        $data['uri'] = 'entity:' . $target_entity_type_id . '/' . $entity->id();
   -      }
   -    }
   +    $this->updateEntityUri($data);
   

   So much better! (Also the other places where the trait is used.) Reuse of methods and much easier to understand code. Wins all around :)

Full patch review:

1. +++ b/core/modules/link/src/Normalizer/EntityLinkItemNormalizerTrait.php
   @@ -0,0 +1,72 @@
   +trait EntityLinkItemNormalizerTrait {
   

   Let's mark this @internal.

2. +++ b/core/modules/link/src/Normalizer/HalLinkItemNormalizer.php
   @@ -0,0 +1,129 @@
   +  public function normalize($field_item, $format = NULL, array $context = array()) {
   ...
   +  public function denormalize($link, $class, $format = NULL, array $context = array()) {
   
   +++ b/core/modules/link/src/Normalizer/LinkItemNormalizer.php
   @@ -0,0 +1,82 @@
   +  public function normalize($object, $format = NULL, array $context = array()) {
   
   +++ b/core/modules/menu_link_content/src/MenuLinkContentServiceProvider.php
   @@ -0,0 +1,36 @@
   +      $service_definition->addTag('normalizer', array('priority' => 40));
   
   +++ b/core/modules/menu_link_content/src/Normalizer/HalMenuLinkContentNormalizer.php
   @@ -0,0 +1,51 @@
   +  public function normalize($entity, $format = NULL, array $context = array()) {
   

   s/array()/[]/

3. +++ b/core/modules/menu_link_content/src/MenuLinkContentServiceProvider.php
   @@ -0,0 +1,36 @@
   +    if (isset($modules['hal'])) {
   +      // Add a hal normalizer service for menu-link-content entities.
   +      $service_definition = new Definition(HalMenuLinkContentNormalizer::class, [
   
   +++ b/core/modules/menu_link_content/src/Normalizer/HalMenuLinkContentNormalizer.php
   @@ -0,0 +1,51 @@
   +/**
   + * A normalizer to handle menu-link content links to entities.
   + */
   +class HalMenuLinkContentNormalizer extends ContentEntityNormalizer {
   

   Why only for hal and not also for serialization?

This is getting very close now :)

#88
Interdiff review
1. fixed @param name and comment
2. fixed
3. fixed with 3 other occurrences
4. fixed with 2 other occurrences
5 Yes!

Full patch review
1. added
2. fixed
3. see #71 5,6

All of the above allowed deleting MenuLinkContentNormalizer because the MenuLinkContent entity no longer needs a denormalizer as this can be done on the field item level.

\Drupal\menu_link_content\Normalizer\HalMenuLinkContentNormalizer is still necessary because it needs to handle 'parent' which a string but references the plugin id of the menu item so it can be handled generically.

Basically in Hal we normalize parent in menu item.

This is getting very close now :)

Yay!

+++ b/core/modules/link/src/Normalizer/HalLinkItemNormalizer.php
@@ -0,0 +1,129 @@
+class HalLinkItemNormalizer extends FieldItemNormalizer {

Can we add the explanation for why this normalizer exists only for hal and not for normalization (i.e. #71.5 and #71.6)?

Otherwise somebody else will surely wonder about that in the future. (I know I will!)

Other than that, this is RTBC now :)

Added comment for our future selves.

Also fix 1 misspelling in a comment.

Great suggestions, thanks :)

@larowlan thanks for the suggestions.

Added

CI flux

CI Flux again

Sorry didn't look close enough. Needs re-roll for #2854830: Move rest/serialization module's "link manager" services to HAL module

Changed to use link manager in Hal module

Whoops missed another reference to the old link manager.

+++ b/core/modules/menu_link_content/tests/src/Kernel/Normalizer/HalMenuLinkContentNormalizerTest.php
@@ -0,0 +1,99 @@
+    $mock_field_uri = $link_manager->getRelationUri('menu_link_content', 'menu_link_content', 'link_target', []);
+    $parent_field_uri = $link_manager->getRelationUri('menu_link_content', 'menu_link_content', HalMenuLinkContentNormalizer::PSEUDO_PARENT_FIELD_NAME, []);
+    $node_url = $node->toUrl('canonical', ['absolute' => TRUE])->setRouteParameter('_format', 'hal_json')->toString();
+    $parent_url = $parent->toUrl('canonical', ['absolute' => TRUE])->setRouteParameter('_format', 'hal_json')->toString();
+    $context['included_fields'] = ['uuid'];
+    $embedded = $serializer->normalize($node, 'hal_json', $context);
+    $embedded_parent = $serializer->normalize($parent, 'hal_json', $context);
+    $normalized = $serializer->normalize($link, 'hal_json');
+    $this->assertEquals($node->uuid(), $normalized['link'][0]['target_uuid']);
+    $this->assertEquals([$embedded], $normalized['_embedded'][$mock_field_uri]);
+    $this->assertEquals([['href' => $node_url]], $normalized['_links'][$mock_field_uri]);
+    $this->assertEquals([$embedded_parent], $normalized['_embedded'][$parent_field_uri]);
+    $this->assertEquals([['href' => $parent_url]], $normalized['_links'][$parent_field_uri]);
+    $this->assertEquals('menu_link_content:' . $parent->uuid(), $normalized['parent'][0]['value']);

+++ b/core/modules/menu_link_content/tests/src/Kernel/Normalizer/MenuLinkContentNormalizerTest.php
@@ -0,0 +1,89 @@
+    $serializer = $this->container->get('serializer');
+    $normalized = $serializer->normalize($link, 'json');
+    // On normalization the 'target_uuid' of the entity will added so that
+    // correct URI can be created for the entity regardless of whether the
+    // entity's id has changed such as in a deployment.
+    $this->assertEquals($node->uuid(), $normalized['link'][0]['target_uuid']);

I wanted to re-RTBC this issue.

But then I realized that what these things are testing… is what should be tested in a \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase test, really.

Maybe it makes sense to keep these tests. But we definitely should have functional test coverage.

In fact, we have that test coverage being added in #2832859: Write EntityResourceTestBase subclasses for: MenuLinkContent!

Once that lands, we can just modify MenuLinkContentResourceTestBase::createEntity() to point to a node instead, and to have a parent link. Then we can repeat exactly this there, and we'll have a far better idea what these normalizations actually look like.

#2832859: Write EntityResourceTestBase subclasses for: MenuLinkContent landed. Let's retest #105. If it still passes, it's RTBC.

Eh, #107 is wrong. Because, quoting myself from the bottom of #106:

Once that lands, we can just modify MenuLinkContentResourceTestBase::createEntity() to point to a node instead, and to have a parent link. Then we can repeat exactly this there, and we'll have a far better idea what these normalizations actually look like.

NW for that.

Rerolling #105 against latest 8.3.x

@AComben thanks for re-rolling!

Setting to "Needs Review" so the test bot runs

Never mind, sorry.

+++ b/core/modules/link/src/Normalizer/EntityLinkItemNormalizerTrait.php
@@ -0,0 +1,73 @@
+    list($target_entity_type_id, $target_entity_id) = $this->getEntityPathParts($link_normalization);
...
+  protected static function getEntityPathParts($link_normalization) {
+    $path = parse_url($link_normalization['uri'], PHP_URL_PATH);

I'm wondering whether this method would better just accept the URI. It'd be a bit more obvious what its actually doing.

#113 @dawehner that sounds better, fixed.

+++ b/core/modules/link/src/Normalizer/LinkItemNormalizer.php
@@ -0,0 +1,82 @@
+        $link['target_uuid'] = $target_entity->uuid();

This is adding something that exists only in the normalization. Which means it won't exist in OpenAPI documentation.

If #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts would be in, then we would be able to just add this as a property on LinkItem and have it work automatically.

#2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts is in, #2910211: Allow computed exposed properties in ComplexData to support cacheability. is the last remaining blocker (was split off from #2871591 to make its scope narrower and help it land sooner), and that too is now RTBC!

#2910211: Allow computed exposed properties in ComplexData to support cacheability.. also landed, this is now fully unblocked!

I've rerolled #115 entirely in the spirit of #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts. This reuses only the logic bits from prior patches, it removes all the normalizers.

This will still need test coverage, I should be able to recycle some of the test coverage of prior patches. For now, please just review for the overall approach.

The result in the REST module:

And together with #2921257-10: On Drupal 8.5, JSON API should respect Typed Data's new ::isInternal(), and JSON API Extras should use ::setInternal(), it even works automatically in the contrib JSON API module:

Has there been any update on progressing this forward so we can have menu links exported with a node?

👏 2 years later and patch still applies, and works with Default Content module, 🎉

While working on some basic test coverage I noticed that the computed property won't actually change when URI property is changed. Thus the test will fail.

Patch in #127 was working well for the most part but it would fail if the user enters an external link in the UI. The attached patch just does a simple check for that.

+++ b/core/modules/link/src/LinkTargetEntityUuid.php
@@ -83,7 +83,11 @@
+    if (count($entity_parts) < 2){

Nitpick: Code style error here. Should be a space between parenthesis and opening curly brace.

Updated patch for comment #130 and also rerolled patch for 9.1.x

Fixing failed tests.

working on this.

Fixing all the issues mentioned in #141

Updated default value of the computed field to NULL, instead of false. Few test cases are passing now. Let's see how it goes.