MenuLinkParent breaks migration when Parent URI is external

Assigning to myself for rvw

I have a look at this and it only raised questions. MenuLinkParent should be handling external links, but the parent issue already handle external parents, admittedly that did not fix the error that joelpittet was having. Still, it makes more sense to me to report the error there and improve that issue. It doesn't help that I don't know much about menu link either.

Second opinion anyone? Merge this into the parent or not?

Unassigning

Assigning to self for review

Discussed at a migrate meeting and there is no problem adding this separately from the parent issue. However, this does need tests.

Looking really good. Only one little nit.

+++ b/core/modules/migrate/tests/src/Unit/process/MenuLinkParentTest.php
@@ -35,4 +36,25 @@ public function testTransformException() {
+  /**
+   * @covers ::transform
+   */

Needs a short comment here "Test plugin when the parent is an external link"

All feedback addressed and tests added.

The patch in #15 caused my migration to fail:

In MenuLinkManager.php line 207:

  Plugin ID '9054' was not found.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
   @@ -85,17 +86,28 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +      $links = [];
   +      if (UrlHelper::isExternal($parent_link_path)) {
   +        $loaded = $this->menuLinkStorage->loadByProperties(['link__uri' => $parent_link_path]);
   +        foreach ($loaded as $plugin_id => $definition) {
   +          $links[$plugin_id] = $this->menuLinkManager->createInstance($plugin_id);
   +        }
   +      }
   

   This change is incorrect:$loaded is keyed by entity id, not plugin id:

     /**
      * Load entities by their property values.
      *
      * @param array $values
      *   An associative array where the keys are the property names and the
      *   values are the values those properties must have.
      *
      * @return \Drupal\Core\Entity\EntityInterface[]
      *   An array of entity objects indexed by their ids.
      */
     public function loadByProperties(array $values = []);
   

2. +++ b/core/modules/migrate/tests/src/Unit/process/MenuLinkParentTest.php
   @@ -35,4 +36,27 @@ public function testTransformException() {
   +    $menu_link_storage->loadByProperties([
   +      'link__uri' => 'http://example.com',
   +    ])->willReturn([
   +      'external_example_plugin_id' => [],
   +    ]);
   

   Menu link content id is numeric (eg. 9054), while a plugin_id looks like ‌menu_link_content:fe151460-dfa2-4133-8864-c1746f28ab27

Attached patch contains the following changes:

1. Updated \Drupal\Tests\migrate\Unit\process\MenuLinkParentTest: $menu_link_storage will now return MenuLinkContentInterface[]
2. \Drupal\migrate\Plugin\migrate\process\MenuLinkParent::transform: simplified $links variable similar to the current call to the current $this->menuLinkManager->loadLinksByRoute

Changes in #18 look good and make sense.

#17 sounds like an integration level test might be good for this, but the Unit tests seem to be the standard for these migration classes, so I guess this is sufficient.

Unrelated fail in MediaLibraryTest

Same unrelated fail #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest

All looks good so setting back to RTBC! Tests failure is not related (see #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest).

Re-uploading #18 as a new patch so we don't keep having test runs (and failures) on the test only patch.

Crediting @quietone for issue review and @mikelutz and @tim.plunkett as per #12.

Committed and pushed 296ba5d866 to 8.8.x and f21ee89c72 to 8.7.x. Thanks!