Remove migration of shortcuts from menu_link migration

+++ b/core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php
@@ -21,7 +21,8 @@ class MenuLink extends DrupalSqlBase {
+      ->condition('ml.menu_name', 'shortcut-set-%', 'NOT LIKE');

🤓 Let's add an @see to point to the place where these are then handled.

This definitely needs some sort of tests. There are three shortcut-set- links in the core d7 test fixture, and they don't cause any issues with testbot, so there must be something more happening here that needs to be investigated and documented.

We still need some sort of test to show what we are fixing.

@chandrashekhar_srijan: Could you also upload a test-only patch to verify that that is failing? 😊🙏

1. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php
   @@ -21,7 +21,10 @@ class MenuLink extends DrupalSqlBase {
   +      // Shortcut set links are migrated by the "d7_shortcut" migration.
   

   The MenuLink source plugin is used for D6 and D7 sources. Can we expand this comment to state how this applies to D6?
   And a small nit, I am not sure we need to quote d7_shortcut.

2. +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d7/MigrateMenuLinkTest.php
   @@ -141,6 +141,12 @@ public function testMenuLinks() {
   +    // Test shortcut menu links exclusion.
   +    $menu_link = MenuLinkContent::load('244');
   +    $this->assertEqual($menu_link, NULL);
   +    $menu_link = MenuLinkContent::load('473');
   +    $this->assertEqual($menu_link, NULL);
   

   The shortcut link will not exist with or without the patch so this isn't going to test for the attempt to stub a config entity. Instead of this we can test for the presence of the migrate messages, 'Config entities can not be logged' from the d7_menu migration by checking the message table. Turns out there are 4 of those without the patch and 0 with the patch.

   I think this is a better way to test that,

       // Test there have been no attempts to stub a shortcut in a MigrationLookup
       // process.
       $messages = $this->getMigration('d7_menu')->getIdMap()->getMessages()->fetchAll();
       $this->assertCount(0, $messages);
   

Addressed 14.1 and 14.2. @quietone can you please elaborate on this "Can we expand this comment to state how this applies to D6?"

@ayushmishra206, the question is will this break Drupal 6 migrations? I don't know if Drupal 6 has the same functionality for shortcuts as Drupal 7. Does it? The Drupal 6 fixture does not have any shortcuts but that is not proof to me. Looking at the query itself, it shouldn't be a problem. I just think it is prudent to ask the question and explain the answer, whatever it is, in the comment.

Ah, so there are no shortcuts in D6. So, while the comment could state that but it is also not necessary.

What needs to happen here is to upload a fail patch (a patch with just the test) to provide proof that the fix is actually working. The working patch will also need to be uploaded again or this will sit at NW. Remember to upload the fail patch first and then the working patch.

Thanks.

Adding tag novice for the work outlined in #16.

When making a fail patch, the patch name will be like 3152934-13-fail.patch, with the correct comment number of course. Then load the fail patch first followed by the complete/good patch. That way the good patch is run last and the issue isn't automatically set to NW but the testbot.

I hope I have understood the task, sorry if not.

Ugh, my bad, uploaded a patch relative to the module and not core, sorry, fixing that now.

Looking good.

1. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php
   @@ -21,7 +21,10 @@ class MenuLink extends DrupalSqlBase {
   +      // Shortcut set links are migrated by the d7_shortcut migration.
   

   Since this source plugin is used by D6 source as well we should comment that d6 doesn't have short cuts so this change will not effect d6 sources.

2. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php
   @@ -21,7 +21,10 @@ class MenuLink extends DrupalSqlBase {
   +      ->condition('ml.menu_name', 'shortcut-set-%', 'NOT LIKE');
   

   Ah, since this changes the source plugin query the source plugin test should change as well, \Drupal\Tests\menu_link_content\Kernel\Plugin\migrate\source\MenuLinkTest. I think it is sufficient to add a shortcut to the menu_link table to the existing test[0]. But then a fail patch will be needed for proof.

Addressed 25.1,2.

1. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php
   @@ -21,7 +21,11 @@ class MenuLink extends DrupalSqlBase {
   +      // Shortcut set links are migrated by the d7_shortcut migration.
   +      // d6 doesn't have short cuts so this change will not effect d6 sources.
   

   I think something like this would be better

   Shortcut set links are migrated by the d7_shortcut migrations. Shortcuts are not used in Drupal 6.

2. +++ b/core/modules/menu_link_content/tests/src/Kernel/Plugin/migrate/source/MenuLinkTest.php
   @@ -17,7 +17,7 @@ class MenuLinkTest extends MigrateSqlSourceTestBase {
   +  protected static $modules = ['menu_link_content', 'migrate_drupal', 'shortcut'];
   

   The shortcut module does not need to be installed. This IS explains that here we are removing getting short cuts from the source database.

3. +++ b/core/modules/menu_link_content/tests/src/Kernel/Plugin/migrate/source/MenuLinkTest.php
   @@ -26,7 +26,7 @@ public function providerSource() {
   +    $tests[0]['source_data']['menu_links']['shortcut-set-2'] = [
   
   @@ -245,10 +245,10 @@ public function providerSource() {
   +    $tests[0]['source_data']['menu_links'][0]['options']['attributes']['title']['shortcut-set-2'] = $title;
   ...
   +    $expected = $tests[0]['source_data']['menu_links']['shortcut-set-2'];
   

   Instead of this (these changes need to be reverted) what is needed here is to prove that the query does not return menu rows that are short cuts. A new row needs to be added to 'menu_links' to do that. If we look at the query itself we see that rows with a particular name are ignored. Therefor the new row must have a name like that.

Hey @quietone
I have made the change as per the comment on #28 but I'm not sure about the changes for 28.3.
I understood that we need to revert the changes done for $tests[0]. And add a new row to menu_links. Also found out that the 5th row is been ignored. But I'm not sure what's needed to be in the newly added row.

@abhisekmazumdar, good question. I didn't explain that too well so here is more detail.

The array $tests[0]['source_data']['menu_links'] contains 7 arrays which represent a row in the 'menu_links' table that is used during the test. That is the data that the query changed here will use. Currently, none of those rows represents a short cut set so we don't have proof that the query will do what we want, which is to exclude menus that are short cut sets.

We actually have two options a) to add a whole set of test data in $tests[1] or b) add a row that represents a short cut set to the existing tests, $tests[0]. I think option b is sufficient which means that an 8th array needs to be added to $tests[0]['source_data']['menu_links']. We don't need to change the expected data, or any of the other input data to the test. We just need to make sure the 8th array represents a short cut row. If we look at the query itself it is excluding rows with where menu_name is 'shortcut-set-%'. That means that a menu_link for a short cut has a menu_name something like 'shortcut-set-foo', the value of the rest of the array keys doesn't matter. So, it is pretty much a copy/paste job to add a new array and change the menu_name.

Now, you mentioned the 5th row. That is excluded from the results because the router path is customized. It is this condition.
->condition('ml.router_path', ['admin/build/menu-customize/%', 'admin/structure/menu/manage/%'], 'NOT IN');

I changed the test method per #31.

@rocketeerbkw, thanks for updated patch.

One last small thing.

+++ b/core/modules/menu_link_content/tests/src/Kernel/Plugin/migrate/source/MenuLinkTest.php
@@ -241,6 +241,37 @@ public function providerSource() {
+        // D7 shortcut set link

Comments needs a period at the end.

Thanks! I didn't bother uploading another fail-only patch, see #38.

Took a look at this, it makes sense to me that we should be excluding these items here. Patch LGTM

To other committers, I'll do this one in a few days.

Updating title and credit

This is changing the behavior of a source plugin such that shortcut links are not returned. In this case, that is not an problem because the saving a shortcut with the menu_link destination causes an error. This is cleaning up that error. I originally suggested that the assertion test for the text of the error message. Instead, the patch checks that there are zero error messages. That may be slightly better in that it increases the likelihood of failing due to other introduced errors.

Committed 679dff2 and pushed to 10.1.x. Thanks!

And cherry picked to 10.0.x and 9.5.x