Migrate D6 menu links

Partial work.
Migration is done, but untested. Call it a day.

Still WIP, almost done. Works quite well.

Added tests for the menu links source.
The migration test is still not working because the menu_name is not set to the menu link, or gets unset somewhere.

Should this issue have #2221733: [Meta] Test individual D6 > D8 content migrations as its parent, so all D6->D8 migration development/testing can continue to be marshalled into one checklist?

I have tried this and get no menu links - do I need to apply one of the above patches (seem really old) or is it just not working yet?

It won't work, too old and D8 menu_links were buggy at that time. I will try to get to this soonish, but unassigned so noone is stopped to work on this one.

I started updating this patch - it's not complete:

• I'm pretty sure a new Migrate Destination needs to be written for menu links.
• I didn't look at the MigrateLinkSourceTest.

-mike

penyaskito and I were talking on IRC and think that we probably don't need to migrate all menu links - perhaps just where menu_links.module = 'menu' and menu_links.customized = 1. Thoughts?

I made some more progress on this patch tonight, but still some work to do in mapping fields and updating MigrateLinkSourceTest.

-mike

Ok - making steady progress now. Updated patch and interdiff attached.

At this point, the functional test is mostly there, I still haven't started updating the unit test.

According to a previous version of the patch, the route_name and route_parameters should be calculated on preSave() - but I don't see a preSave() method for menu_link_content...

I'm also not sure if we need to do anything with the p1...p9 D6 fields.

-mike

I just updated the unit test. Interdiff and patch attached.

@chx - I think you gave me a hint a couple of weeks ago on how to proceed on the route_name, route_parameters, and p1-p9 fields, but I don't remember what it was (sorry!) Can you point me in the right direction on what I need to do to finish this up?

Thanks,
-mike

chx gave me some guidance on how to move forward with this patch, I've made some progress, but I've hit a wall.

chx and I talked about using $pathValidator->getUrlIfValid() in Drupal\migrate_drupal\Plugin\migrate\source\d6\MenuLink::prepareRow() to set the route_name and route_parameters values for each link.

Unfortunately, when testing this, regardless of the path that I set up to migrate in MigrateMenuLinkTest, $pathValidator->getUrlIfValid() always returns false ($pathValidator->getPathAttributes() is actually what returns false). What am I missing?

Also - should I inject the $pathValidator into Drupal\migrate_drupal\Plugin\migrate\source\d6\MenuLink? I looked into doing this, but didn't see any other source classes that were using dependency injection, so I didn't have anything to model the code on.

I went ahead and added another groups of tests for an external URL.

-mike

@chx,

The URLs I tried passing to $pathValidator->getUrlIfValid() are the ones in the Drupal6MenuLink dump (the "link_path" field):

1. admin
2. admin/module
3. http://drupal.org

Running drush ev 'var_dump(Drupal::service("path.validator")->getUrlIfValid("admin");' (or "admin/modules" or "https://drupal.org") returns nothing. Nada. Zip. Zilch. http://note.io/1tfftur

When I step through MigrateMenuLinkTest with my debugger, $pathValidator->getUrlIfValid() (in MenuLink::prepareRow()) returns false for the internal paths and a $url object for the external path.

Any ideas?

-mike

I chatted with chx about this, and he suggested that we add an access_check boolean to PathValidator and PathValidatorInterface that will allow us to check paths without also checking access to those paths (via #2346189: Denormalizing paths into route names/parameters is brittle / broken).

The attached patch incorporates these changes.

-mike

Based on a conversation with chx, we decided to:

1. Move the pathValididator stuff from the MenuLink source to a process plugin. In fact, I created two Migrate (not Migrate Drupal) process plugins - one for route_name and one for route_parameters. I wasn't sure if there was a good way to calculate two destination fields from one source field using a single process plugin. If there is a better way, let me know and I'll be happy to implement.
2. I removed an obsolete test and some comments.
3. I updated the migrate_drupal.source.schema.yml with the new d6_menu_link stuff.

Updated patch and interdiff attached.

-mike

chx showed me the way on IRC on this one. Changes include:

1. Combined "route_name" and "route_parameters" Migrate process plugins into a single "route" plugin.
2. New "route" plugin now takes "options" as a second source field.
3. New "route" plugin now outputs "url" and "options" in addition to "route_name" and "route_parameters".
4. New "route" plugin will support D6 and D7 "options['query']" variable.
5. MigrateMenuLinkTest now includes a test with a link that has a query string.

Patch and interdiff attached.

-mike

This patch looks great, nice work guys. Few super tiny nitpicks below related to comments. I also think we might need some unit tests for the new path validator methods?

1. +++ b/core/modules/migrate_drupal/config/install/migrate.migration.d6_menu_links.yml
   @@ -0,0 +1,50 @@
   +  route:
   +    plugin: route
   +    source:
   +      - link_path
   +      - options
   +  route_name: @route/route_name
   +  route_parameters: @route/route_parameters
   +  url: @route/url
   

   Nice approach :)

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/MenuLink.php
   @@ -0,0 +1,107 @@
   +  public function prepareRow(Row $row) {
   +    $row->setSourceProperty('options', unserialize($row->getSourceProperty('options')));
   +    $row->setSourceProperty('enabled', !$row->getSourceProperty('hidden'));
   

   Missing comment.

3. +++ b/core/modules/migrate_drupal/src/Tests/Dump/Drupal6MenuLink.php
   @@ -0,0 +1,289 @@
   + * Contains \Drupal\migrate_drupal\Tests\Dump\Drupal6Menu.
   

   Wrong comment.

4. +++ b/core/modules/migrate_drupal/src/Tests/Dump/Drupal6MenuLink.php
   @@ -0,0 +1,289 @@
   + * Database dump for testing menu migration.
   

   Wrong comment.

Looks great!

1. +++ b/core/lib/Drupal/Core/Path/PathValidator.php
   @@ -81,7 +81,28 @@ public function isValid($path) {
   +  public function isValidWithoutAccessCheck($path) {
   +    return (bool) $this->getUrlIfValidWithoutAccessCheck($path);
   +  }
   

   Why are we adding this? I guess for parity with the IfValid() methods but the only usecase is in a test. Seems superfluous.

2. +++ b/core/lib/Drupal/Core/Path/PathValidatorInterface.php
   @@ -24,6 +24,19 @@
   +   * Returns an URL object, if the path is valid.
   +   *
   +   * Unlike getUrlIfValid(), access check is not performed.
   +   *
   +   * @param string $path
   +   *   The path to check.
   +   *
   +   * @return \Drupal\Core\Url|false
   +   *   The url object, or FALSE if the path is not valid.
   +   */
   +  public function getUrlIfValidWithoutAccessCheck($path);
   

   Do we need to document the security concerns with using this method? Like we should only be using this if the path is not going to be presented to a user - right?

All points in #31 have been addressed.

This issue is a unfrozen change (migrate) as per https://www.drupal.org/core/beta-changes and it's benefits outweigh any disruption. Committed 92c2514 and pushed to 8.0.x. Thanks!

I just ran a migration from a fairly simple legacy D6 site to a clean 8.0.x site and I'm getting the following error. The full stack trace is attached.

Running d6_menu_links                                                  [ok]
exception 'PDOException' with message 'SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'menu_name' cannot be null' in                                   [error]
/var/www/tag1eight/drupal/core/lib/Drupal/Core/Database/Statement.php:61

Hmm, that's interesting menu_name is a look-up from the menu migration. Did that run first without any errors?

Relevant yaml:

  menu_name:
    plugin: migration
    migration: d6_menu
    source: menu_name

We could add a process to skip the row on null menus but i'm more interested in how the menu_name ended up null in the first place.