Rename Migration process plugin and add documentation

Self-assigning for review.

+1 to the name change.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/Migration.php
   @@ -16,7 +16,120 @@
   + *   - source_ids:
   + *   - stub_id:
   

   Missing descriptions.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/Migration.php
   @@ -16,7 +16,120 @@
   + * migrated the user accounts in a migration named users, you would specify the
   

   Put quote marks on the name of the migration 'users'. Or better, give it a more distinctive name, such as users_migration. That avoids it also looking like a DB table name.

Actually, now I think about it, the change of a plugin name should really happen in a separate issue from documentation. One is quite a dramatic change that should be clearly signposted in git logs and release notes; the other is fairly uncontentious.

Based on #2824610-15: Rename DedupeBase/DedupeEntity process plugins to MakeUnique and add documentation we've been doing renames inside of the docs issue.

Changes in response to @joachim's code review in #6.

@todo: process plugin rename.

Setting back to Needs Work to rename the process plugin.

Does anyone have any suggestions for the new name of this plugin? Reading through the documentation here's the best I can come up with:

• Relationship
• Connection
• Maintain integrity

If we can settle on a name then I am happy to make the change and push this one through - it has been sitting around too long for such a basic change.

reference_lookup? relationship_lookup? Or, migration_lookup from the issue summary? I think the key point is that the source value is used to perform a lookup against the results of another migration...

• Deprecated the Migration process.
• Added the MigrationLookup process.
• Pointed the tests to MigrationLookup
• Updating current uses of migration to migration_lookup.

Let's pretend we didn't see that #embarrassed

Thanks, here's my first round of feedback:

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,291 @@
   + * Calculates the value of a property based on a previous migration.
   

   I'd say "Looks up" rather than "Calculates".

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,291 @@
   + * created from it may have a uid of 456. The migration_lookup process maintains
   + * the relationships between source and destination identifiers in map tables,
   + * and this information is leveraged by the migration process plugin.
   

   It's not the migration_lookup plugin that maintains the relationship, but it is that plugin which leverages the information. So, swap "migration_lookup process" and "migration process".

3. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,291 @@
   + * - migration: An array of migration ids.
   

   , or a single migration ID.

4. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,291 @@
   + *
   

   no_stub should also be documented here.

5. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,291 @@
   + *   uid:
   + *   plugin: migration_lookup
   + *   migration:
   + *     - users
   + *     - members
   + *   source_ids:
   + *     users:
   + *       - author
   + *     members:
   + *       - author
   

   Everything after "uid:" needs to be indented.

   It would be a clearer demonstration of the use case to have different source_ids for the different migrations.

6. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,291 @@
   + * If the migration_lookup does not find the source ID in the migration map it
   

   s/migration_lookup/migration_lookup plugin/

7. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,291 @@
   + * of migration list will be used, but you can select the migration you wish to
   

   "the" before "migration list".

8. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,291 @@
   + * use to create the stub:
   

   Should add something like "using the stub_id configuration key".

9. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,291 @@
   + *     source_ids:
   + *       users:
   + *         - author
   + *       members:
   + *         - author
   

   Let's take the source_ids out of this example.

10. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
    @@ -0,0 +1,291 @@
    + *   plugin: migration_lookup
    + *   migration: users
    + *   no_stub: true
    + *   source: author
    

    Indentation.

11. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
    @@ -0,0 +1,291 @@
    + * Scenario:
    + *   - field_drupal6_pages is a drupal 6 node reference field.
    + *   - field_drupal8_pages is a drupal 8 entity reference field.
    

    ...
    Do we need these scenarios? Everything we need already has documentation.

12. +++ b/core/modules/migrate/tests/modules/migrate_external_translated_test/migrations/migrate.migration.external_translated_test_node_translation.yml
    similarity index 90%
    rename from core/modules/migrate/tests/src/Unit/process/MigrationTest.php
    
    rename from core/modules/migrate/tests/src/Unit/process/MigrationTest.php
    rename to core/modules/migrate/tests/src/Unit/process/MigrationLookupTest.php
    

    We should copy rather than rename (thus having near-duplicate tests) so the BC layer is tested.

Fixed

I'm digging this extensive documentation. Great work. Migration lookup is very hard to explain (lots of facets to it) and you've all done a beautiful job. I have a few complaints:

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/Migration.php
   @@ -21,156 +10,8 @@
   + * @deprecated in Drupal 8.3.x and will be removed in Drupal 9.0.x. Use
   + *   \Drupal\migrate\Plugin\migrate\process\MigrationLookup instead.
   

   Nit: I believe these two lines should be flush with each other.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,286 @@
   + * change in the process of migration. For example, on the source site a given
   + * user account may have an "author" ID of 123, but the Drupal user account
   

   Should be "For example, on the source site, a given user account may have an ID of 123..."

3. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,286 @@
   + * - migration: A single migration id, or an array of migration ids.
   

   'id' should be 'ID', and 'ids' should be 'IDs'.

4. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,286 @@
   + * - source_ids: (optional) An array keyed by migration ids with values that are
   

   IDs

5. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,286 @@
   + * - stub_id: (optional) Identifies which entry in the migration array will be
   + *   used to create any stub entities.
   

   Can this be "Identifies the migration which will be used to create any stub entities"?

6. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,286 @@
   + * The value of migration can be a list of migration ids. When using multiple
   

   Can this be "'migration' can be a list of migration IDs"?

7. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,286 @@
   + * case one can use source_ids which is an array keyed by the migration ids
   

   IDs

8. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,286 @@
   + * and the value is a list of source properties with the same features as normal
   + * source (see the get plugin for more on this).
   

   The end of this sentence is really unclear. Off the top of my head I'm not sure how to rephrase it, but I think it is confusing as-is. It sounds important, but is hard to grok.

9. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,286 @@
   + * migration you wish to* by using the stub_id configuration key:
   

   Should be "...the migration you wish to use..."

10. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
    @@ -0,0 +1,286 @@
    + * Here, the value of stub_id selects the members migration to create any stub
    + * entities, overriding the default of the users migration.
    

    I think we should remove everything after the final comma ("entities, overriding..."). The word "overriding" muddies the clarity of the sentence.

11. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
    @@ -0,0 +1,286 @@
    + * migration map, use no_stub. It can be set to any non-empty value:
    

    Let's remove the last sentence and put a colon after "use no_stub". As far as developers need be concerned, no_stub is a boolean value.

12. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
    @@ -0,0 +1,286 @@
    + *   - field_drupal6_pages is a drupal 6 node reference field.
    + *   - field_drupal8_pages is a drupal 8 entity reference field.
    

    "drupal" should be capitalized.

13. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
    @@ -0,0 +1,286 @@
    + *   - field_drupal7_pages is a drupal 7 entity reference field.
    + *   - field_drupal8_pages is a drupal 8 entity reference field.
    

    Ditto.

Sorry @Yogesh Pawar, I've already done this one. Perhaps I should've self-assigned.

Well, I screwed that up! Here's the actual patch.

It's okay @Jo Fitzgerald no problem.

I think you missed one thing #23.1 so added updated patch with interdiff.

Aww, I should've mentioned that I'd discussed that with @phenaproxima in IRC and we'd decided to not make that change. Oh well, it's there now and you have made your contribution. Everyone's happy :)

Some of my feedback in #18 has not been addressed:

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,285 @@
   + * - stub_id: (optional) Identifies the migration which will be used to create
   + *   any stub entities.
   + *
   

   no_stub should also be listed here.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,285 @@
   + *       source_ids:
   + *         users:
   + *           - author
   + *         members:
   + *           - author
   

   The source keys here should be different (otherwise this would be unnecessary).

3. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,285 @@
   + * Scenario:
   + *   - field_drupal6_pages is a Drupal 6 node reference field.
   + *   - field_drupal8_pages is a Drupal 8 entity reference field.
   + *
   ...
   

   Again, is there a purpose to these scenarios, which don't seem to add anything to the other examples we have?

1. Added documentation for no_stub.
2. Edited the "members" source_id to make sense of a convoluted example.
3. Removed the redundant scenarios.

The patch itself is RTBC as far as I'm concerned. Just one more thing - let's have a change record for this.

Added a CR.

Drupal CI is temperamental on this fine day, but that is no reason for it to impugn so fine a patch. Back to RTBC.

Some small nits. Tagging novice for the docs updates.

1. +++ b/core/modules/migrate/tests/src/Unit/process/MigrationTest.php
   @@ -14,6 +17,9 @@
   + *   \Drupal\Tests\migrate\Unit\processMigrationLookupTest instead.
   

   Nit: missing a slash between process\MigrationLookupTest

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,261 @@
   + * source site, but in most instances the unique identifiers of the content
   

   "Most" provides a value judgement that doesn't add value and isn't necessarily true. Let's avoid that dilemma and remove it entirely.

3. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -0,0 +1,261 @@
   + * Here, the value of stub_id selects the members migration to create any stub
   + * entities.
   

   It isn't clear that these lines refer to the code example above it.

Applied changes from comments in #41.

+++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
@@ -17,8 +17,7 @@
  * It is important to maintain relationships among content coming from the
- * source site, but in most instances the unique identifiers of the content
- * change in the process of migration. For example, on the source site, a given
+ * source site. For example, on the source site, a given
  * user account may have an ID of 123, but the Drupal user account created from
  * it may have a uid of 456. The migration process maintains the relationships
  * between source and destination identifiers in map tables, and this

Super small nit, but could we get all these lines to line up with the 80 character limit? With the clean-up of the comment, there is plenty of space to re-flow the text.

This looks great to me. When phenaproxima verbally described the issue I got confused by 'migration ' as in "migration migration process plugin", so "migration lookup process plugin" is a lot easier to figure out.

DrupalCS isn't happy though:

PHPCS: core/modules/migrate/src/Plugin/migrate/process/DefaultValue.php passed

FILE: ....x/core/modules/migrate/src/Plugin/migrate/process/Migration.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 5 | ERROR | [x] Concat operator must be surrounded by a single space
 6 | ERROR | [x] Concat operator must be surrounded by a single space
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 17ms; Memory: 4Mb

PHPCS: core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php passed

FILE: ...e/modules/migrate/tests/src/Unit/process/MigrationLookupTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 93 | WARNING | @expectedException tags should not be used, use
    |         | $§this->setExpectedException() or
    |         | $this->expectException() instead
----------------------------------------------------------------------

Time: 37ms; Memory: 6Mb


FILE: ....x/core/modules/migrate/tests/src/Unit/process/MigrationTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 5 | ERROR | [x] Concat operator must be surrounded by a single space
 6 | ERROR | [x] Concat operator must be surrounded by a single space
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 39ms; Memory: 6Mb

Looks good. Back to RTBC assuming Drupal CI passes.

Had to re-roll core/modules/migrate/tests/src/Unit/process/MigrationLookupTest.php to account for changes in d66aaae6c77cca28eb0cec31588c00c7778d9d55 . Interdiff is ugly and not usable.

OK, I think I mucked that patch up a bit in #48. Let's try this. Interdiff against 44.

Renamed that patch to .patch

This should really only be RTBC when the patch goes green.

It came back green.

On renaming in the docs issue, it's a bit tricky because renaming would mean updating some documentation, but actually writing the full documentation relies on the rename (at least in these two cases) - so we'd have two patches with a direct dependency. On the other hand it's made this patch a bit harder to review since the diff is quite large. So not sure what's best with that.

There was still one phpcs violation in the patch, I think it's just this stray line but since the annotation is used by phpunit, uploading a patch just in case rather than directly fixing on commit. Leaving RTBC will commit once this comes back green.

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

Tagging for mention in 8.4.0 release notes.

The change record should be sufficient to capture the API improvement here since it has BC, but we'll include a bullet in the release notes for these and other Migrate and Migrate Drupal API improvements. Thanks!

Ah, looks like this one was also backported to 8.3.x.