Upgrade path for Shortcut 7.x

There should be a dependency on the d7_user migration, but as far as I can tell, this isn't in D8 core yet.

Go ahead and declare the dependency in the migration; when the patch is ready we'll just postpone this issue on the user migration (if it needs to be postponed -- that one is not far from being ready for commit).

D8 already provides a "default" shortcut set. We should probably write a special case where the D7 default ("shortcut_set_1") gets migrated to the D8 default set ("default"). Now the D8 default still exists, and the D7 default just becomes another migrated set.

You can achieve this by directly assigning the shortcut set ID. The static_map plugin is your friend :)

Postponing on #2414651: Migration Files for Drupal 7 Users.

Unblocked!

WOW! Terrific work @svendecabooter! Very thorough, clear, and easy to follow. I have some changes and fixes to request, but otherwise this is first-class work.

1. +++ b/core/modules/shortcut/migration_templates/d7_shortcut.yml
   @@ -0,0 +1,31 @@
   +  shortcut_set:
   +    -
   +      plugin: static_map
   +      bypass: true
   +      source: menu_name
   +      map:
   +        shortcut-set-1: default
   +    -
   +      plugin: machine_name
   +      field: shortcut_set
   

   It's preferable here to use the migration process plugin (because having a process plugin named "migration" isn't confusing at all!) to get the ID of the previously migrated shortcut set. Documentation on the migration plugin may be found at https://www.drupal.org/node/2149801

2. +++ b/core/modules/shortcut/migration_templates/d7_shortcut.yml
   @@ -0,0 +1,31 @@
   \ No newline at end of file
   

   Whoops :) Please add a blank line at the end of the file to satisfy the git gods.

3. +++ b/core/modules/shortcut/migration_templates/d7_shortcut_set_users.yml
   @@ -0,0 +1,24 @@
   +  uid: uid
   

   This should also use the migration process plugin to look up the uid from the d7_user migration.

4. +++ b/core/modules/shortcut/migration_templates/d7_shortcut_set_users.yml
   @@ -0,0 +1,24 @@
   +  set_name:
   +    -
   +      plugin: static_map
   +      bypass: true
   +      source: set_name
   +      map:
   +        shortcut-set-1: default
   +    -
   +      plugin: machine_name
   +      field: set_name
   

   Same here.

5. +++ b/core/modules/shortcut/src/Plugin/migrate/destination/ShortcutSetUsers.php
   @@ -0,0 +1,105 @@
   +class ShortcutSetUsers extends DestinationBase implements ContainerFactoryPluginInterface {
   +  /**
   

   Nit: please add a new line between the class declaration and the doc comment.

6. +++ b/core/modules/shortcut/src/Plugin/migrate/destination/ShortcutSetUsers.php
   @@ -0,0 +1,105 @@
   +   * @param \Drupal\Core\Path\AliasStorage $alias_storage
   +   *   The alias storage service.
   

   This doc comment doesn't match the actual argument. :)

7. +++ b/core/modules/shortcut/src/Plugin/migrate/destination/ShortcutSetUsers.php
   @@ -0,0 +1,105 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function fields(MigrationInterface $migration = NULL) {
   +    return [
   +      'uid' => 'The users.uid for this set.',
   +      'source' => 'The shortcut_set.set_name that will be displayed for this user.',
   +    ];
   +  }
   

   IIRC, fields() doesn't apply to destinations, so this can be removed

8. +++ b/core/modules/shortcut/src/Plugin/migrate/destination/ShortcutSetUsers.php
   @@ -0,0 +1,105 @@
   +    return TRUE;
   

   import() is supposed to return the ID of the saved entity (matching what's declared in getIds()), not a boolean. In this case it should return array($account->id()), and you don't need to check for $account. Because, if you use the migration process plugin to look up the uid (in the d7_shortcut_users migration), a MigrateSkipRowException will be thrown if the user cannot be found, which is an appropriate response in this case.

9. +++ b/core/modules/shortcut/src/Plugin/migrate/destination/ShortcutSetUsers.php
   @@ -0,0 +1,105 @@
   \ No newline at end of file
   

   D'oh. Needs a new line.

10. +++ b/core/modules/shortcut/src/Plugin/migrate/source/d7/Shortcut.php
    @@ -0,0 +1,50 @@
    + * @MigrateSource(
    + *   id = "d7_shortcut"
    + * )
    

    Needs source_provider = "shortcut" in the annotation.

11. +++ b/core/modules/shortcut/src/Plugin/migrate/source/d7/Shortcut.php
    @@ -0,0 +1,50 @@
    +    $query = $this->select('menu_links', 'ml')
    +      ->fields('ml', array('mlid', 'menu_name', 'link_path', 'link_title', 'weight'))
    +      ->condition('hidden', '0')
    +      ->condition('menu_name', 'shortcut-set-%', 'LIKE');
    +    return $query;
    

    Nit: You can return $query directly, no need for a separate return statement.

12. +++ b/core/modules/shortcut/src/Plugin/migrate/source/d7/Shortcut.php
    @@ -0,0 +1,50 @@
    \ No newline at end of file
    

    Needs a new line.

13. +++ b/core/modules/shortcut/src/Plugin/migrate/source/d7/ShortcutSet.php
    @@ -0,0 +1,45 @@
    + * @MigrateSource(
    + *   id = "d7_shortcut_set"
    + * )
    

    Also needs source_provider = "shortcut" in the annotation.

14. +++ b/core/modules/shortcut/src/Plugin/migrate/source/d7/ShortcutSet.php
    @@ -0,0 +1,45 @@
    +    $query = $this->select('shortcut_set', 'ss')
    +      ->fields('ss', array('set_name', 'title'));
    +    return $query;
    

    Same nit: you don't need to return $query separately.

15. +++ b/core/modules/shortcut/src/Plugin/migrate/source/d7/ShortcutSet.php
    @@ -0,0 +1,45 @@
    \ No newline at end of file
    

    Needs a new line.

16. +++ b/core/modules/shortcut/src/Plugin/migrate/source/d7/ShortcutSetUsers.php
    @@ -0,0 +1,45 @@
    + * @MigrateSource(
    + *   id = "d7_shortcut_set_users"
    + * )
    

    Ditto on the source_provider annotation.

17. +++ b/core/modules/shortcut/src/Plugin/migrate/source/d7/ShortcutSetUsers.php
    @@ -0,0 +1,45 @@
    +    $query = $this->select('shortcut_set_users', 'ssu')
    +      ->fields('ssu', array('uid', 'set_name'));
    +    return $query;
    

    Ditto on the return $query nit.

18. +++ b/core/modules/shortcut/src/Plugin/migrate/source/d7/ShortcutSetUsers.php
    @@ -0,0 +1,45 @@
    \ No newline at end of file
    

    Ditto on the new line. I swear, I'll stop repeating myself now.

19. +++ b/core/modules/shortcut/src/Tests/Migrate/d7/MigrateShortcutSetTest.php
    @@ -0,0 +1,58 @@
    +  public function testShortcutSetMigration() {
    

    This method looks a little light on assertions. I recommend asserting everything you can think of that the migration may have touched. The more assertions we have, the more bugs we will catch.

20. +++ b/core/modules/shortcut/src/Tests/Migrate/d7/MigrateShortcutSetUsersTest.php
    @@ -0,0 +1,59 @@
    +    $shortcut_set = shortcut_current_displayed_set($account);
    

    If there is an OO way to do this, it's preferable. If not, no worries.

21. One last thing: please add unit tests for the source plugins. There are many you can look at for reference -- for instance, take a look at core/modules/user/tests/src/Unit/Plugin/migrate/source/d7.

Changing status.

Postponing on #2382985: Migration Files for Drupal 7 Menu & Menu Entries. Shortcut sets and menu links are tightly coupled, so it makes sense to get this done once menu links are migrating OK.

Unblocked!

Looks excellent. I have some changes I'd like to see, but I'll take care of these and we'll get this in soon.

1. +++ b/core/modules/shortcut/migration_templates/d7_shortcut.yml
   @@ -0,0 +1,33 @@
   +label: Drupal 7 shortcut links
   

   We no longer include the "Drupal 6" and "Drupal 7" prefixes in migration labels, so this should just be "Shortcut links".

2. +++ b/core/modules/shortcut/migration_templates/d7_shortcut.yml
   @@ -0,0 +1,33 @@
   +       source:
   +         - menu_name
   

   Nit: This can just be source: menu_name.

3. +++ b/core/modules/shortcut/migration_templates/d7_shortcut_set.yml
   @@ -0,0 +1,20 @@
   +label: Drupal 7 shortcut sets
   

   Ditto the label fix here.

4. +++ b/core/modules/shortcut/migration_templates/d7_shortcut_set.yml
   @@ -0,0 +1,20 @@
   +    -
   +      plugin: machine_name
   +      field: id
   

   This might not be necessary -- it looks like shortcut set IDs are already machine names.

5. +++ b/core/modules/shortcut/migration_templates/d7_shortcut_set_users.yml
   @@ -0,0 +1,38 @@
   +label: Drupal 7 Shortcut set user mapping
   

   Ix-nay on the "Drupal 7".

6. +++ b/core/modules/shortcut/migration_templates/d7_shortcut_set_users.yml
   @@ -0,0 +1,38 @@
   +      source:
   +        - uid
   

   Nit: Should be source: uid.

7. +++ b/core/modules/shortcut/migration_templates/d7_shortcut_set_users.yml
   @@ -0,0 +1,38 @@
   +    -
   +      plugin: default_value
   +      default_value: [1]
   +    -
   +      plugin: extract
   +      field: uid
   +      index:
   +        - 0
   

   This stuff might not be entirely necessary, because the uid returned by the lookup in d7_user should be a single value, not an array.

   IMO, we should not assign a default value if the user lookup fails; in this case I think it's appropriate to simply skip the row.

8. +++ b/core/modules/shortcut/src/Plugin/migrate/destination/ShortcutSetUsers.php
   @@ -0,0 +1,108 @@
   +  /**
   +   * Active database connection.
   +   *
   +   * @var \Drupal\Core\Database\Connection
   +   */
   +  protected $database;
   

   It's unclear why we're injecting the database as a dependency; it doesn't seem to be used for anything in the plugin...

9. +++ b/core/modules/shortcut/src/Plugin/migrate/destination/ShortcutSetUsers.php
   @@ -0,0 +1,108 @@
   +    return array($account->id());
   

   It seems to me that we'd want to return the ID as an array containing the set name and user ID (and alter the ids() method to match), since that's really what identifies the imported data, no?

10. +++ b/core/modules/shortcut/src/Plugin/migrate/source/d7/ShortcutSet.php
    @@ -0,0 +1,45 @@
    +    return $this->select('shortcut_set', 'ss')
    +      ->fields('ss', array('set_name', 'title'));
    

    Nit: Let's do fields('ss') instead of cherry-picking.

11. +++ b/core/modules/shortcut/src/Plugin/migrate/source/d7/ShortcutSetUsers.php
    @@ -0,0 +1,45 @@
    +    return $this->select('shortcut_set_users', 'ssu')
    +      ->fields('ssu', array('uid', 'set_name'));
    

    Ditto.

12. +++ b/core/modules/shortcut/src/Plugin/migrate/source/d7/ShortcutSetUsers.php
    @@ -0,0 +1,45 @@
    +  public function getIds() {
    +    $ids['uid']['type'] = 'integer';
    +    return $ids;
    

    Let's identify the rows by set_name AND uid, rather than just uid.

13. +++ b/core/modules/shortcut/src/Tests/Migrate/d7/MigrateShortcutSetTest.php
    @@ -0,0 +1,78 @@
    +  protected function assertEntity($shortcut_set, $id, $label) {
    

    It's preferable to pass the shortcut set ID and let assertEntity() load it and assert that it's a ShortcutSetInterface.

14. +++ b/core/modules/shortcut/src/Tests/Migrate/d7/MigrateShortcutSetTest.php
    @@ -0,0 +1,78 @@
    +    $this->assertIdentical(count($shortcuts), 2);
    

    Let's not hard-code the number of expected shortcuts in the set -- pass that in as an argument to assertEntity().

15. +++ b/core/modules/shortcut/src/Tests/Migrate/d7/MigrateShortcutTest.php
    @@ -0,0 +1,80 @@
    +  public function testShortcutMigration() {
    

    This method should probably use the assertEntity() pattern. (The purpose of which is for easier conversion to the data provider pattern that PHPUnit gives us, which we'll probably do in 8.1 or 8.2.)

16. +++ b/core/modules/shortcut/tests/src/Unit/Plugin/migrate/source/d7/ShortcutSetTest.php
    @@ -0,0 +1,46 @@
    +    $this->databaseContents['shortcut_set'][] = array(
    +      'set_name' => 'shortcut-set-2',
    +      'title' => 'Alternative shortcut set',
    +    );
    

    I'd prefer if you assigned $this->databaseContents['shortcut_set'] = $this->expectedResults, rather than repeating the code.

17. +++ b/core/modules/shortcut/tests/src/Unit/Plugin/migrate/source/d7/ShortcutSetUsersTest.php
    @@ -0,0 +1,46 @@
    +    $this->databaseContents['shortcut_set_users'][] = array(
    +      'uid' => '2',
    +      'set_name' => 'shortcut-set-2',
    +    );
    +
    

    Ditto here.

Made the changes I asked for in #24. Let's give this a whirl...

QA is failing patch #25 for no apparent reason -- each time I re-queue it for testing, it fails an update-related test (this patch does not need or introduce any update-related code), for different, random reasons. On @tim.plunkett's advice, setting this issue to NR because DrupalCI passed it.

DrupalCI has passed patch #37 and @benjy reviewed on IRC this morning. Per #36, RTBC.

Let's have a quick-fix follow-up.

Follow-up critical issue filed: #2570265: Migrate test failure on PostgreSQL

With a quick-fix applied, from #2570265: Migrate test failure on PostgreSQL.

#2571499: idMap source and destination id filtering requires keys has been committed and it fixes the root of the problem which caused this issue to break PostgreSQL. I have re-tested the original patch (#37) and it now passes PostgreSQL with flying colors -- and everything else, for that matter. Restoring RTBC.

Committers, note that #37 is RTBC!

Er. Restoring RTBC.