Handle menu_items related to Drupal 6 and 7 node translations with different IDs

Tagging.

Soft postponing on #2912348: Handle entity_references related to Drupal 6 and 7 node translations with different IDs while we figure out a plan of action.

I started to look at this to see if what we are doing in #2912348: Handle entity_references related to Drupal 6 and 7 node translations with different IDs will help here.

For the entity reference field, the non-existent IDs were migrated into the field so it is possible to run a follow-up migration to update them with the right IDs.

But in the menu_items migration, we validate that the paths exists, so the paths pointing to non-existent nodes do not get migrated and we can't have a follow-up migration to update them.

I will try to see what we can do to fix that or if we will need a different solution here.

When #2912348: Handle entity_references related to Drupal 6 and 7 node translations with different IDs lands, here's a first POC to work with.

In fact, this issue doesn't need to be postponed on #2912348: Handle entity_references related to Drupal 6 and 7 node translations with different IDs. The problem we had with entity reference fields was that they needed a migration to be derived after all other migrations, so we came up with the idea of follow-up migrations.

But here, there's no need for a derived migration so we can add a new normal migration, making sure it will run at the right time by using the existing migration dependencies system.

I will add tests in the next few days.

Here's tests for D6 and D7.

The previous patch will fail, I forgot to update the entity counts, as always.

Fixed the tests and renamed the migration to node_translation_menu_links.

Oups, forgot to update the tests with the new migration ID.

I think this is really fantastic work. Wow! Great job, everyone. I have only a few things to complain about, hopefully all relatively minor.

1. +++ b/core/modules/menu_link_content/migrations/node_translation_menu_links.yml
   @@ -0,0 +1,77 @@
   +  # Get the new node ID by looking in the d7_node_translation mapping table.
   

   We're also looking in d6_node_translation :)

2. +++ b/core/modules/menu_link_content/migrations/node_translation_menu_links.yml
   @@ -0,0 +1,77 @@
   +  new_nid:
   +    -
   +      plugin: link_uri
   +      source:
   +        - link_path
   +    -
   +      plugin: explode
   +      delimiter: 'entity:node/'
   +    -
   +      plugin: extract
   +      default: false
   +      index:
   +        - 1
   +    -
   +      plugin: skip_on_empty
   +      method: row
   +    -
   +      plugin: migration_lookup
   +      migration:
   +        - d6_node_translation
   +        - d7_node_translation
   +      no_stub: true
   +    -
   +      plugin: skip_on_empty
   +      method: row
   +    -
   +      plugin: extract
   +      index:
   +        - 0
   

   This process pipeline is complex and opaque. I think we should add a big comment here to extensively document what is expected to happen at every stage of this transformation.

3. +++ b/core/modules/menu_link_content/src/Plugin/migrate/process/LinkUri.php
   @@ -82,7 +82,13 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +        if (preg_match('/^base:node\/\d+/', $uri)) {
   

   This should probably have a $ anchor at the end.

4. +++ b/core/modules/menu_link_content/src/Plugin/migrate/process/LinkUri.php
   @@ -82,7 +82,13 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +          return preg_replace('/^base:/', 'entity:', $uri);
   

   I wonder if it might not be preferable to use str_replace() or regex capture here.

5. +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTest.php
   @@ -3,19 +3,24 @@
   +use Drupal\Tests\node\Kernel\Migrate\d6\MigrateNodeTestBase;
   

   Doesn't this sort of establish a hard testing dependency on Node?

6. +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTest.php
   @@ -70,6 +83,47 @@ public function testMenuLinks() {
   +    $this->assertIdentical('The Real McCoy', $menu_link->getTitle());
   +    $this->assertIdentical('primary-links', $menu_link->getMenuName());
   +    $this->assertIdentical(NULL, $menu_link->getDescription());
   +    $this->assertIdentical(TRUE, $menu_link->isEnabled());
   +    $this->assertIdentical(FALSE, $menu_link->isExpanded());
   +    $this->assertIdentical(['attributes' => ['title' => ''], 'alter' => TRUE], $menu_link->link->options);
   +    $this->assertIdentical('entity:node/10', $menu_link->link->uri);
   +    $this->assertIdentical(0, $menu_link->getWeight());
   

   All of this should be assertSame().

@phenaproxima: Thanks for the review, I'm on it.

1. Done.
2. Done. I hope my comments are clear enough.
3. Done.
4. Done.
5. I think that the menu_links migration should in fact run after the node migration and I added an optional dependency for that. Why? Because the link_uri process plugin do a validation of the paths and if the nodes have not been migrated then all paths to nodes will fail.
6. I changed the assertIdentical() to assertSame(), but while at it, I decided to use a helper method like the one in the D7 test.

1. +++ b/core/modules/menu_link_content/migrations/node_translation_menu_links.yml
   @@ -0,0 +1,89 @@
   +      source:
   +        - link_path
   

   Nit: This can combine into a single line.

2. +++ b/core/modules/menu_link_content/migrations/node_translation_menu_links.yml
   @@ -0,0 +1,89 @@
   +      # Try to isolate the node ID.
   

   Be more assertive. "Isolate the node ID."

3. +++ b/core/modules/menu_link_content/migrations/node_translation_menu_links.yml
   @@ -0,0 +1,89 @@
   +      # If there's a node ID, get it.
   ...
   +      # If a new node ID was found, get it.
   

   Extract node ID. We use 0 because...? Why do we use zero? That's a question I don't know the answer to. But comments that explain why are usually better than ones that explain what is going on here.

4. +++ b/core/modules/menu_link_content/migrations/node_translation_menu_links.yml
   @@ -0,0 +1,89 @@
   +      # If there's no node ID, skip the row.
   

   Skip row if node ID is empty.

5. +++ b/core/modules/menu_link_content/migrations/node_translation_menu_links.yml
   @@ -0,0 +1,89 @@
   +      # d7_node_translation mapping tables to try to find the new node ID.
   

   Nit: "to find the new node ID." We can drop the words "try to".

6. +++ b/core/modules/menu_link_content/migrations/node_translation_menu_links.yml
   @@ -0,0 +1,89 @@
   +      # If no new node ID was found, skip the row.
   

   Skip row if the new node ID is empty.

7. +++ b/core/modules/menu_link_content/migrations/node_translation_menu_links.yml
   @@ -0,0 +1,89 @@
   +  link_path:
   +    plugin: concat
   +    source:
   +      - 'constants/node_prefix'
   +      - '@new_nid'
   +  link/uri:
   +    plugin: concat
   +    source:
   +      - 'constants/entity_prefix'
   +      - '@link_path'
   

   Can this combine into a single step?

8. +++ b/core/modules/menu_link_content/src/Plugin/migrate/process/LinkUri.php
   @@ -82,7 +82,18 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +        $uri = $url->getUri();
   +        if (preg_match('/^base:node\/(\d+)$/', $uri, $matches)) {
   +          return 'entity:node/' . $matches[1];
   

   This seems to be loosing functionality here. i.e. introducing a regression. What about sites that do not have i18n menus and see this situation?

Thanks for the review!

1. The link_uri plugin expects an array, that's why it's on a new line. We could also do source: [link_path] but I prefer the new line.
2. Done.
3. Done.
4. Done.
5. Done.
6. Done.
7. We cannot combie this because the link_path value is used in two processes below, link/uri & route.
8. What about adding a configuration to the plugin, like translations or keep_unrouted_node_paths? Another way would be to write a new specialized plugin just to deal with unrouted node paths.

Back to NR.

Tests failed because now the node_translation_menu_links migration does not update previously migrated menu links, but creates new ones, so it needs all the menu link properties do to so.

+++ b/core/modules/menu_link_content/migrations/node_translation_menu_links.yml
@@ -0,0 +1,92 @@
diff --git a/core/modules/menu_link_content/src/Plugin/migrate/process/LinkUri.php b/core/modules/menu_link_content/src/Plugin/migrate/process/LinkUri.php

+++ b/core/modules/menu_link_content/src/Plugin/migrate/process/LinkUri.php
@@ -82,7 +82,18 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+        if (!empty($this->configuration['translations']) && preg_match('/^base:node\/(\d+)$/', $uri, $matches)) {

I see, we are doing a check on if this is a translation and ignoring things. What if we did this more generically and had a config flag to ignore checking? Something like "ignore_route_checking"

What about this?

+++ b/core/modules/menu_link_content/src/Plugin/migrate/process/LinkUri.php
@@ -82,14 +82,11 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+        if (!empty($this->configuration['ignore_route_validation'])) {

Yes, perfect. We just need to document the config option in doxygen.

Would you be OK with a follow-up for that? There's no existing documentation on this plugin so this seems out of scope. And also we'll have to figure out and document (or fix) why the plugin expects an array and then only use the first item:

  public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
    list($path) = $value;
    [...]
  }

Self-review nits.

re #26: absolutely, let's open a docs issue. No reason to block a critical on that. Let's add the TODO and see if there is any other feedback. But I think this is pretty close to ready now.

Follow-up created and @todo added.

All feedback addressed.

Looks great!! Only found a couple of things, but I'm not sure either should block commit.

1. +++ b/core/modules/menu_link_content/migrations/node_translation_menu_links.yml
   @@ -0,0 +1,119 @@
   +      ignore_route_validation: true
   

   I don't like this double-negative construction; it's confusing. I'd prefer something like validate_route: false, which would override the default value of TRUE. Is it too late to change that?

2. +++ b/core/modules/menu_link_content/src/Plugin/migrate/process/LinkUri.php
   @@ -82,7 +84,15 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +        // If the URL is not routed, we might want to get something back to do
   +        // other processing. If this is the case, the "ignore_route_validation"
   +        // configuration option can be use to return the URI.
   +        if (!empty($this->configuration['ignore_route_validation'])) {
   +          return $url->getUri();
   +        }
   +        else {
   +          throw new MigrateException(sprintf('The path "%s" failed validation.', $path));
   +        }
   

   Do we have an explicit, direct test of this code path in LinkUriTest?

Done!

This has gone through a couple rounds of feedback. All of which is now addressed. Let's try RTBC on for size.

Agreed. +1 for RTBC.

+++ b/core/modules/menu_link_content/tests/src/Kernel/Plugin/migrate/process/LinkUriTest.php
@@ -103,20 +120,68 @@ public function providerTestNotRouted() {
+  public function testRouteValidation(array $value, $expected) {

Nitpicky but here you are testing disabling route validation so the test method name testRouteValidation is confusing - same with the provider. Patch attached fixes this. Leaving at RTBC because this is just a test method name change. I've also moved the doTransform() method to the bottom again. Tests pass locally - uploading to confirm they still pass on the bots.

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

This introduced problems for menu link migrations. See #2833248-8: Migrations of menu links associated with entities failing.