Refactor and document the MenuLinkParent process plugin

Self-assigning for review.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
   @@ -15,7 +15,28 @@
   + * Determines the menu link parent plugin IDs.
   

   For people who don't understand that menu links are plugins, this will be extraordinarily confusing. I think we should strike all mentions of plugin IDs. Let's change this, then, to something like "Determines the parent of a menu link."

   Ideally, we should also link to documentation that explains how menu links work in D8.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
   @@ -15,7 +15,28 @@
   + * The menu_link_parent process plugin figures out menu link parent plugin IDs.
   

   This sentence adds nothing. Let us be rid of it.

3. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
   @@ -15,7 +15,28 @@
   + * The source is an array of three values:
   + *   - parent_id: The parent link ID (plid) is the mlid of the link above in the hierarchy, or zero if the link is at the top level in its menu.
   + *   - menu_name: The menu name. All links with the same menu name (such as 'navigation') are part of the same menu.
   + *   - parent_link_path: The Drupal path or external path this link points to.
   

   These lines exceed 80 characters.

   Can we re-word the parent_id description to "The numeric ID of the parent menu link, or 0 if the link is at the top level of its menu"?

   In the parent_link_path description, "external path" should be "external URL".

4. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
   @@ -15,7 +15,28 @@
   + * @code
   + * process:
   + *   parent:
   + *     plugin: menu_link_parent
   + *     source:
   + *       - plid
   + *       - menu_name
   + *       - parent_link_path
   + * @endcode
   

   This example needs to be explained.

5. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
   @@ -64,9 +85,24 @@ public static function create(ContainerInterface $container, array $configuratio
   -   * {@inheritdoc}
       *
       * Find the parent link GUID.
   +   * Performs the associated process.
   +   *
   +   * @param string $value
   +   *   The input string.
   +   * @param \Drupal\migrate\MigrateExecutableInterface $migrate_executable
   +   *   The migration in which this process is being executed.
   +   * @param \Drupal\migrate\Row $row
   +   *   The row from the source to process.
   +   * @param string $destination_property
   +   *   The destination property currently worked on. This is only used together
   +   *   with the $row above.
   +   *
   +   * @return string
   +   *   The sub string of $value.
   +   *
   +   * @throws \Drupal\migrate\MigrateSkipRowException
   

   This should be {@inheritdoc}.

Changes in response to code review.

Here's the correct patch from #6.

Checked the patch in #8 and it as per suggestions given in #5.So marking it for review , so that it can be tested.

I'd like to take another look at this before we jump ahead with RTBC.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
   @@ -15,7 +15,33 @@
   + * The source is an array of three values:
   + *   - parent_id: The numeric ID of the parent menu link, or 0 if the link is at
   + *     the top level of its menu.
   + *   - menu_name: The menu name. All links with the same menu name (such as
   + *     'navigation') are part of the same menu.
   + *   - parent_link_path: The Drupal path or external URL this link points to.
   

   These will need to be outdented to be flush with the first line.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
   @@ -15,7 +15,33 @@
   + * In this example the parent_id of '20' will be looked up and returned from
   + * amongst the already migrated IDs. If it is not found then the
   

   This is a little unclear because it is directly referring to something that is done by the migration plugin, not this one. We should probably include a link, at least, to the migration plugin's documentation.

   Also, I notice that the plugin's transform() method throws a MigrateSkipRowException if it falls all the way through. That needs to be documented.

I have addressed #12.1 and documented the MigrateSkipRowException (I hope this is what you meant, @phenaproxima).

As for #12.2 - the example is an adapted version of that in d*_menu_links which does indeed involve the migration plugin, but I would argue that this example does not. I'm struggling to explain this any more clearly than I did the first time, so I am open to any guidance.

The patch applies well and also the required documentation have been added. So I guess that it can be changed to RTBC.

I took a closer look at the MenuLinkParent plugin and it's pretty far out of date; for example, it tries to catch a MigrateSkipRowException that is no longer thrown. I need the entire plugin needs sprucing up in addition to better documentation, so I think we should expand the scope of this issue and fix it up in here.

While removing the MigrateSkipRowException try-catch I discovered that is is still required because Migration::transform() can throw the exception if the 'no_stub' property of the migration destination is set to true (see modules/migrate/src/Plugin/migrate/process/Migration.php:124 which leads to modules/migrate/src/Plugin/Migration.php:400). I hope that makes some sense (happy to discuss in IRC)!

I can't see anything else in the plugin that needs sprucing up, but let me know if you have something specific in mind, @phenaproxima.

Setting back to Needs Review (in hope of RTBC).

Sorry -- I had to refactor the plugin a bit. It's out of date and confusing, I couldn't help myself.

I made the following changes:

1) Added a skip_row configuration option to control whether or not the plugin throws MigrateSkipRowException. The decision really shouldn't be up to the plugin at all -- if someone wants to skip the row, they should put a skip_on_empty in the process pipeline. However, to maintain BC, I added this option and defaulted it to TRUE.

2) Got rid of the try-catch block. The migration process plugin does not throw MigrateSkipRowException unless it receives an empty value, which this plugin guarantees will not happen. Therefore the try-catch has no purpose.

3) Various other bits and pieces.

Okay, this should fix the broken tests.

I have streamlined things a little bit by making MenuLinkParent directly extend the Migration process plugin (aliased here as Lookup for clarity). I also restored the try-catch that I removed in #17 -- a path already well-trodden by @Jo Fitzgerald, but one I had to grok for myself -- and added a comment explaining it.

Additionally, I added support for external URIs. At the moment, MenuLinkParent doesn't know what to do if the parent_link_path is an external URI; it'll probably just fail with an InvalidArgumentException, thrown by Url::fromUserInput(). I made it more graceful; it will attempt to look up an existing link with that same external URI before falling through. It's a backwards-compatible change, so I'm not sure it'll need a change record...?

A very minor nit-pick, but perhaps Migration could be aliased as MigrationLookup (rather than just Lookup) because that's what it'll be called once #2845486: Rename Migration process plugin and add documentation is in.

Just realized that we'll need tests of the external URI handling.

Migration process plugin was renamed to migration_lookup. Needs re-roll.

Re rolled.

@pk188 Please can you explain the changes to core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php ?

I'll have a look at this in the near future.

Here's a patch with suggested improvements to the API documentation.

A couple of questions to @phenaproxima:

1. Is your idea that this TODO item should be addressed under this same issue or as a follow-up?

     catch (MigrateSkipRowException $e) {
-
+      // There are two reasons the exception may have been thrown. Either the
+      // input value was empty -- definitely not the case, since we checked for
+      // that at the top of this method -- or the lookup tried to stub the
+      // parent menu link, but the destination's configuration has 'no_stub' set
+      // to TRUE. In this latter case, we'll just act as if the lookup failed,
+      // and fall back to looking for the parent link using the combination of
+      // menu name  and  parent link path.
+      //
+      // TODO: Introduce a new exception to be thrown if stubbing fails or is
+      // disallowed, and catch that instead.
+      $parent_id = NULL;
+    }

2. And the same question for this

+    // TODO: Remove this behavior and the skip_row configuration option.
+    if (!empty($this->configuration['skip_row'])) {
+      throw new MigrateSkipRowException();
     }
-    throw new MigrateSkipRowException();
+    return NULL;

Cheers,
Markus

Apparently I forgot to attach the files to the previous comment...

I think we should probably address both in this issue. We're already refactoring the hell out of MenuLinkParent, so I see no reason not to attack those TODOs now.

Self assigning.

Here's a new version trying to address #32.2.

• The 'skip_row' configuration option was added in #19 i.e. it is not in the current codebase.
• I could not see why it was added in #19 with the TODO comment to remove it, so maybe @phenaproxima can elaborate the background a bit.

For the #32.1

• Did I understand correctly @phenaproxima that you are suggesting that we would create a new exception that we could throw in MigrationLookup?
• So instead of throwing MigrateSkipRowException MigrationLookup would through a new exception that we would catch here?
• I read through the MigrationLookup::transform() and with the comments that we have on this issue and in the patch TODO comments could not figure out the details... @phenaproxima, could you elaborate this as well?

Cheers,
Markus

Edit: formatting

[error] The internal path component 'https://example.com/publications.html' is external. You are not allowed to specify an external URL together with internal:/.''

Which is coming from MenuLinkParent because of the assumption that the parent link path is routeable through $url = Url::fromUserInput("/$parent_link_path");

The patch doesn't resolve this but does touch this class and line so I thought I'd mention it and hopefully I can resolve in another issue. Just and FYI

Wild idea: why not treat Menu Link Parents via the content system and use an entity reference field? Similar to taxonomy parent table removal at #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration given menus were already converted to entities to use fieldability? #916388: Convert menu links into entities

I moved my issue to it's own thing with a patch to avoid mudding up this one.
#2968170: MenuLinkParent breaks migration when Parent URI is external

The documentation here is excellent. Really nice and clear. It's also easy to follow the logic of the plugin -- these are significant improvements.

+++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
@@ -52,51 +94,76 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
+        $url = Url::fromUserInput('/' . ltrim($parent_link_path, '/'));

Don't we need to prefix the URL with internal:/?

Other than that, this needs dedicated tests of each possible path through the plugin:

• The menu link is a root item.
• The parent ID exists.
• The parent ID did not exist, but was stubbed.
• The parent ID did not exist and couldn't be stubbed.
• The parent ID did not exist, could not be stubbed, and the parent link path/menu name were not passed. (This should result in a MigrateSkipRowException.)
• The parent ID did not exist, could not be stubbed, and the parent link path is external and could be loaded.
• The parent ID did not exist, could not be stubbed, and the parent link path is external and could not be loaded. (Should result in a MigrateSkipRowException.)
• The parent ID did not exist, could not be stubbed, and the parent link path is an internal URI that exists and could be loaded.
• The parent ID did not exist, could not be stubbed, and the parent link path is an internal URI that doesn't exist. (Should result in a MigrateSkipRowException.)

Here's the beginning of the tests. I don't have time to finish this now, but maybe someone else can take up the baton?

Corrected the mis-named data provider. Still requires plenty more tests.

Patch from #47 no longer applies. Re-rolled.

Failed to incorporate test file in #49 patch. This is a re-roll of the patch in #47.

NW for remaining tests.

Did we update the list of tests in the IS that are now covered or is that still needed?

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
   @@ -2,38 +2,70 @@
   + * Menu link item belongs to a menu such as 'Navigation' or 'Administration'.
   + * Menu link item also has a parent item unless it is the root element of the
   + * menu.
   

   These phrases could be combined into a more simple single sentence.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
   @@ -52,51 +94,76 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   +      // that at the top of this method -- or the lookup tried to stub the
   +      // parent menu link, but the destination's configuration has 'no_stub' set
   ...
   +      // TODO: Introduce a new exception to be thrown if stubbing fails or is
   +      // disallowed, and catch that instead.
   

   This TODO either needs to be removed (my preference) or an issue opened.

Cleaning up tags and bumping back to NW.

Only patch reroll by now.

I think extending a plugin instead of calling to it diminishes the value of process plugin being plugins -- one could replace the migration_lookup plugin class with their own for their own needs and the callers would be oblivious to it. By extension a tight coupling is introduced which doesn't seem necessary to me.

Mostly this looks great, thanks! Definitely agree all this could use some clean-up and help. :)

Mostly nit picks, but this includes a few things of substance, so setting NW...

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
   @@ -7,17 +7,50 @@
   + * Menu link item belongs to a menu such as 'Navigation' or 'Administration'.
   ...
   + *       - admin
   ...
   + * determine the parent by a combination of 'admin' menu name and
   

   Originally, we call the menu name "Administration". Then in the example code, we have just 'admin', which we (sort of) explain. It's a bit of a confusing example, since the menu link path also starts with 'admin', and folks might think that's what's going on (especially if they skim or don't read the fine print).

   Could we use an example from a 'Main navigation' ('main') menu, so that there's no overlap between the menu machine name and the link path?

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
   @@ -7,17 +7,50 @@
   + * @see https://www.drupal.org/docs/8/api/menu-api
   

   Ooof, I know there's talk of restructuring d.o doc links to remove the 8 vs. 9 part of these paths, since the 9 vs. 8 handbooks are going to be so overlapping. I wonder how many @see links we already have like this. :/ I wonder what's the alternative.

3. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
   @@ -103,7 +140,6 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   -    $migration_configuration['migration'][] = $migration->id();
   

   I haven't read all the comments in the thread, but I don't see this line being added back elsewhere...

4. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
   @@ -142,9 +180,24 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +        // combination of menu name  and  parent link path.
   

   extra spaces around 'and' here.

   Otherwise, LOVE how thorough and helpful this comment is!

5. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
   @@ -142,9 +180,24 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +        // TODO: Introduce a new exception to be thrown if stubbing fails or is
   

   Should this be a formal @todo? Is there already an open issue about this? If not, can we file it and include a link to it with this comment?

6. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
   @@ -152,25 +205,32 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +    // Parent could not be determined by ID so we try to determine by the
   

   Wants a comma after "ID" before "so".

7. +++ b/core/modules/migrate/tests/src/Unit/process/MenuLinkParentTest.php
   @@ -56,44 +65,58 @@ protected function setUp() {
   +   * @param string|array $source_value
   

   If this is a string or array of strings, @param string|string[] would be better. We can almost always do better than array as a @param type.

8. +++ b/core/modules/migrate/tests/src/Unit/process/MenuLinkParentTest.php
   @@ -56,44 +65,58 @@ protected function setUp() {
   +    [$parent_id, $menu_name, $parent_link_path] = $source_value;
   

   Looks like $source_value is always treated as an array of strings...

   so the above should probably just be @param string[] $source_value.

9. +++ b/core/modules/migrate/tests/src/Unit/process/MenuLinkParentTest.php
   @@ -56,44 +65,58 @@ protected function setUp() {
   +    $plugin->transform([$parent_id, $menu_name, $parent_link_path], $this->migrateExecutable, $this->row, 'destinationproperty');
   

   On first glance, not clear why we're converting this back into an array manually. Why not just:

   $plugin->transform($source_value, ...) ?

10. +++ b/core/modules/migrate/tests/src/Unit/process/MenuLinkParentTest.php
    @@ -56,44 +65,58 @@ protected function setUp() {
    +   * Provides data for the MenuLinkParentTest.
    

    That's not the test this provides data for.

11. +++ b/core/modules/migrate/tests/src/Unit/process/MenuLinkParentTest.php
    @@ -56,44 +65,58 @@ protected function setUp() {
    +      // The parent ID did not exist, could not be stubbed, and the parent link
    +      // path is external and could not be loaded.
    +      'test one' => [
    

    A) In all cases, "The parent ID did not exist, could not be stubbed". Why repeat that every time? Can't we comment that once at the start and say it holds for all the examples?

    Once we do that, the part that's different for each case could be listed as the array key, so we wouldn't need comments at all. E.g.:

    'external path' => [...],
    'no menu name' => [...],
    'internal path does not exist' => ...
    

    (or something).

12. +++ b/core/modules/migrate/tests/src/Unit/process/MenuLinkParentTest.php
    @@ -56,44 +65,58 @@ protected function setUp() {
    +      // The parent ID did not exist, could not be stubbed, and the parent link
    +      // path/menu name were not passed.
    +      'test two' => [
    +        'source_value' => [1, NULL, 'http://drupal.org'],
    +      ],
    

    Looks like only the menu name isn't passed. Not sure why this is talking about "the parent link path/" in the comment. Maybe we don't need comments at all (see above), but if we keep them, can we simplify this to only mention the menu name?

13. +++ b/core/modules/migrate/tests/src/Unit/process/MenuLinkParentTest.php
    @@ -110,18 +133,84 @@ public function testLegacyTransformExternal() {
    +   * @param string|array $source_value
    +   *   The source value(s) for the migration process plugin.
    

    As above. This should be @param string[] $source_value.

14. +++ b/core/modules/migrate/tests/src/Unit/process/MenuLinkParentTest.php
    @@ -110,18 +133,84 @@ public function testLegacyTransformExternal() {
    +   * @param string|array $expected_result
    +   *   The expected value(s) of the migration process plugin.
    

    Looking at the provider, this is always a string. If it handles an array, it's not clear how. wouldn't transform() always return a single string, not an array? If it does take an array, it should be string[].

15. +++ b/core/modules/migrate/tests/src/Unit/process/MenuLinkParentTest.php
    @@ -110,18 +133,84 @@ public function testLegacyTransformExternal() {
    +      // The menu link is a root item.
    +      'menu link is route item' => [
    ...
    +      // The parent ID exists.
    +      'parent id exists' => [
    ...
    +      // The parent ID exists and is external.
    +      'parent id exists external' => [
    

    These keys are self-documenting. Not sure we also need to replicate them as comments.

Thanks again!
-Derek

Fantastic. Thanks for opening #3131567: Introduce a new exception if stubbing fails or is disallowed

One very tiny remaining nit:

+++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
@@ -142,9 +180,24 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+        // @todo Introduce a new exception to be thrown if stubbing fails or is
+        // disallowed. https://www.drupal.org/node/3131567

From what I've seen, the core convention for these is something like this:

    // @todo In https://www.drupal.org/node/3131567 introduce a
    //   new exception to be thrown if stubbing fails or is disallowed.

I'm composing this in dreditor, not emacs, so I'm not sure that's wrapped correctly, but hopefully you get the idea. ;) Key points:

a) Start with "In [link]"
b) Subsequent lines for the same @todo should be indented 2 spaces

Otherwise, this seems RTBC to me. Thanks again!

#74 Seems like a bug, or at least a code smell. Url::fromUserInput() will always return an unrouted internal:// url. I don't see why we need/have that code path, as I don't believe it can be executed.

Hmm, interesting. I guess I need to dig into it a little further.

Looking good. A few final concerns:

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
   @@ -99,10 +135,13 @@ public static function create(ContainerInterface $container, array $configuratio
   +    // Handle root elements of a menu.
        if (!$parent_id) {
          // Top level item.
   

   Why do we need both of these comments for the same early return?

2. +++ b/core/modules/migrate/tests/src/Unit/process/MenuLinkParentTest.php
   @@ -55,44 +66,210 @@ protected function setUp(): void {
   +   * Provides data for testTransformException.
   ...
   +   * Provides data for testMenuLinkParent.
   

   Core isn't totally consistent, but seems to use () at the end of function names in comments like this.

3. +++ b/core/modules/migrate/tests/src/Unit/process/MenuLinkParentTest.php
   @@ -55,44 +66,210 @@ protected function setUp(): void {
   +  /**
   +   * Provides data for testMenuLinkParent.
   +   */
   +  public function providerTransformExternal() {
   +    return [
   +      // The parent ID does not exist and the parent link path is external and
   +      // could be loaded.
   +      'test' => [
   +        'source_value' => [9054, 'admin', 'http://example.com'],
   +        'lookup_result' => NULL,
   +        'plugin_id' => 'menu_link_content:fe151460-dfa2-4133-8864-c1746f28ab27',
   +        'expected_result' => 'menu_link_content:fe151460-dfa2-4133-8864-c1746f28ab27',
   +      ],
   +    ];
   +  }
   

   Seems slightly weird to go through the trouble of a data provider when there's only 1 test case. I guess it's "good plumbing for later" but I wonder about YAGNI.

   If we keep it, the comment points to the wrong test function, and we should add ().

4. +++ b/core/modules/migrate/tests/src/Unit/process/MenuLinkParentTest.php
   @@ -55,44 +66,210 @@ protected function setUp(): void {
   +  /**
   +   * Provides data for testMenuLinkParent.
   +   */
   +  public function providerMenuLinkParenInternal() {
   +    return [
   +      'no parent id internal route' => [
   +        'source_value' => [20, 'admin', 'admin/content'],
   +        'lookup_result' => NULL,
   +        'plugin_id' => 'system.admin_structure',
   +        'expected_result' => 'system.admin_structure',
   +      ],
   +    ];
   +  }
   

   I believe all this is dead code. If not, typo in the function name.

Almost RTBC. :)

Thanks!
-Derek

Re: #84: Thanks for the fixes! Looking really close now.

Re: #84.3: I'd feel a lot better RTBC'ing if we added a 2nd case to that provider. Not sure what else makes sense for testing an external menu parent. If we truly can't think of any other cases to test for that, I'd advocate undoing the provider and just let it be a simple test case with a single case. A provider for 1 case seems like a wonky precedent to set for core tests. Of course, what I think doesn't matter as much as what the core committers think. ;) /shrug

Yeah, that's much better, thanks! However, with that change:

+++ b/core/modules/migrate/tests/src/Unit/process/MenuLinkParentTest.php
@@ -55,44 +66,160 @@ protected function setUp(): void {
+   * Tests menu link content process plugin when the parent is an internal link.

This comment is no longer true. :/

Sorry,
-Derek

Thanks for catching that the other test summary was wrong.

If we're going to go this far, we should use proper sentences for these. ;)

1. +++ b/core/modules/migrate/tests/src/Unit/process/MenuLinkParentTest.php
   @@ -77,7 +76,7 @@ protected function setUp(): void {
   +   * Tests an exception is thrown when the parent menu link is not found.
   

   "Tests that an ..."?

   Or maybe:

   "Tests which exception ..."?

2. +++ b/core/modules/migrate/tests/src/Unit/process/MenuLinkParentTest.php
   @@ -116,7 +115,7 @@ public function providerTransformException() {
   +   * Tests menu link content process plugin.
   

   Tests the menu ...

Sorry about the reminder. ;)

Nothing left to complain about, RTBC! 🎉

Thanks for your patience and perseverance...

This one bit has me a bit confused...

+++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
@@ -7,16 +7,49 @@
+ * - parent_link_path: The Drupal path or external URL this menu link points to.

parent_link_path - I think this should be The Drupal path or external URL the parent of this menu link points to.

Setting back to needs review for #92 - if I'm wrong this can be set back to rtbc.

Interdiff in #94 addresses #92. Sorry I missed that in earlier reviews. Back to RTBC.

Thanks!
-Derek

Needs a re-roll.

And back to RTBC.

Committed and pushed 244ccb2863 to 9.1.x. Thanks!

I considered backporting to 9.1.x but since this mixes documentation and code changes I felt that this was unnecessary. I'm pretty sure that the code changes are safe but been caught out in the past so not taking the risk.