Add Better Normalizers as a dependency

URI seems to be exported as "entity:node/12345": I'm re-rolling the patch to take that into account in MenuLinkNormalizer::getNidFromUri().

Re-rolling making sure tests pass.

+++ b/src/MenuLinkNormalizer.php
@@ -0,0 +1,65 @@
+  function getNidFromUri($uri) {
+    preg_match_all('/entity:node\/(\d*)/', $uri, $matches);
...
+        $node_id = $this->getNidFromUri($link['uri']);
...
+          $target_entity = Node::load($node_id);
...
+        $node_id = $this->getNidFromUri($link['uri']);
...
+          $node = $this->entityManager->loadEntityByUuid('node', $link['_uuid']);
+          $data['link'][$key]['uri'] = str_replace($node_id, $node->id(), $link['uri']);

Can we make this entity-type agnostic. E.g. parse out the entity-type too, and inject the EntityTypeManager so we can use $this->entityTypeManager->getStorage($entity_type)->load($entity_id) instead of Node::load

Also, instead of using preg_match_all, we should use parse_url() like \Drupal\Core\Url::fromUri does.

Looking great though, nice work

• Changed MenuLinkNormalizer in an entity-agnostic way.
• Changed preg_match_all with parse_url()
• Fixed coding standard issues.

Sorry, I had to upload .txt file.

Uploading full patch file.
/cc @sanduhrs

Changed method name, made it more clear and consistent with its purpose.

Looking good! Marking as RTBC.

Nevermind

Working with this patch I had conflicts with Better Normalizers sandbox module, given that MenuLinkNormalizer class is not defining its specific $supportedInterfaceOrClass property.

Attaching a new patch that forces MenuLinkNormalizer to be used only for Drupal\menu_link_content\MenuLinkContentInterface objects.

Thanks

When both my menu links and nodes are installed as default content from the same module, I hit this error:

Error: Call to a member function id() on null in Drupal\default_content\MenuLinkNormalizer->denormalize() (line 120 of /XXX/modules/contrib/default_content/src/MenuLinkNormalizer.php).

This is because the menu links don't declare their dependencies on the nodes, so are created before the nodes, so the denormalization introduced in these patches fails.

1. +++ b/default_content.services.yml
   @@ -2,3 +2,8 @@ services:
   +    arguments: ['@rest.link_manager', '@entity.manager', '@module_handler', '@entity_type.manager']
   

   entity.manager is not needed (deprecated)

2. +++ b/src/MenuLinkNormalizer.php
   @@ -0,0 +1,128 @@
   + * @file
   + * Contains \Drupal\default_content\MenuLinkNormalizer.
   

   please remove

3. +++ b/src/MenuLinkNormalizer.php
   @@ -0,0 +1,128 @@
   + * Class MenuLinkNormalizer.
   ...
   +class MenuLinkNormalizer extends ContentEntityNormalizer {
   

   Better brifly describe what if does and for

I experienced the same problem as described in #17.

I decided to experiment a bit, here's a bit of a different take on the problem.

Resolved a conflict with other normalizers with the same priority by stating a supported interface.

Adding detail here per #2842533-2: Menu hierarchy not maintained after enabling menu link content

Latest patch doesn't fix the problem with menu hierarchy.
Menu item parents are still not assigned correctly when enabling default menu link content.

Latest patch *does* fix the problem with menu hierarchy, but requires all menu_link_content to be re-exported in order to build the dependencies.

We should just make default_content depend on Better Normalizers and move http://cgit.drupalcode.org/entity_pilot/tree/src/Normalizer/MenuLinkCont... into it, and use it in both Entity Pilot and Default content.

MenuLinkContentNormalizer has a chain of dependencies on entity_pilot, starting with UnsavedUuidResolverInterface and including entity_pilot/Exists plugin.

How do you suggest that this be untangled?

Ah right, because Entity Pilot needs to dereference unsaved entities so it can preview incoming content.

We don't need that, but we need the other part of that logic to handle parent references.

OK, that makes this patch much simpler.
Postponing on #2845455: Menu Link Content normalizer and #2845456: Move MenuLinkContentNormalizer to better_normalizers, except for UnsavedUuidResolverInterface

#2845455 is resolved - MenuLinkNormalizer is in better_normalizers now, so both this issue and #2845456 can proceed to review

results from testbot:

Unable to install modules: module 'default_content' is missing its dependency module better_normalizers.

uh... what?
this is a one-line patch to add a dependency.
is testbot broken?

TIL about "test_dependencies"

If this patch passes tests, then #27 should be considered for inclusion.

This should not be so difficult

OK, I give up.
I don't know how to add a dependency that will pass tests.

re-submitting #27

+++ b/default_content.info.yml
@@ -5,3 +5,4 @@ package: Web services
+  - better_normalizers

composer.json also needs update

let's see what testbot will tell

Adding a dependency on another module require test coverage

Released new beta to allow composer to find dependency https://www.drupal.org/project/better_normalizers/releases/8.x-1.0-beta2

One more round on composer dependencies

Tests for 8.2 core passed, also commited #2849133: Add rest dependency

Keeping NW for tests

patch rerolled

I wasn't able to get patch #46 to apply to the commit before "20 May 2017 at 12:15". I was able to apply patch #43 to the commit before "11 February 2017 at 05:05", and then rebased and resolved conflicts. Attached is a rerolled patch of 43 with an interdiff.

Missed a commit. Take 2.

Rerolled on last dev version

This patch:
- removes an uneeded dependency on the rest module introduced by #49
- removes the test_dependency addition to the info file as the documentation indicates it's only useful if it is different from dependencies

This issue is super useful to deal with menus.
It'd be really cool if people that use these patches could jump in and say "hi" :)

+1 to have better_normalizers here.

I tested it and it works very well.

Please find an updated version of DuaelFr's patch.

Ok, I just edited DualFr test without properly testing it and I missed to update the drupal/core version in composer.json. Also the info.yml does not contain the core attribute anymore, only core_version_requirement. Please find an updated patch.
It sill fails on my local due https://www.drupal.org/project/drupalorg/issues/3066468

Fixed require failure issue in #58, please review.

Thanks mrinalini9. It works well.

I've recently created the 2.0.x branch, see the project page on all the improvements in the 2.0.x branch. Testing that and providing feedback would be very welcome. The 1.x branch isn't actively maintained and won't receive new features anymore, so I'm closing this and other issues as won't fix.

2.0.x uses a completely different normalization implementation that does not depend on hal.module anymore and all the features of the better normalizers module are built-in.