Remove load plugins

1. +++ b/core/modules/migrate/src/MigrationStorage.php
   @@ -64,52 +64,54 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
   +  protected function getVariantIds(array $ids) {
   

   Don't know about using the word "variant" for these migrations, given we have display variants in Drupal already it could be confusing?

2. +++ /dev/null
   @@ -1,318 +0,0 @@
   -class CckFieldValues extends DrupalSqlBase implements SourceEntityInterface, CckFieldMigrateSourceInterface {
   

   Has all the code from this source already been moved into a builder?

This is a scary looking patch, I'd like to see someone test this on an a real site before getting committed similar to how i did here: https://codedrop.com.au/blog/video-drupal-6-drupal-8-migration

I'll be testing this with a real site using migrate_upgrade as well as reviewing the code, but a patch this big should get benjy's oversight as well.

Running migrate_upgrade on my D6 site, things went well until migrating a node type with an image field:

Importing d6_node__blog
PHP Fatal error:  Call to a member function getFileUri() on a non-object in /Users/mryan/Sites/d8/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php on line 318
PHP Stack trace:
PHP   1. {main}() /Users/mryan/.composer/vendor/drush/drush/drush.php:0
PHP   2. drush_main() /Users/mryan/.composer/vendor/drush/drush/drush.php:11
PHP   3. Drush\Boot\BaseBoot->bootstrap_and_dispatch() /Users/mryan/.composer/vendor/drush/drush/drush.php:70
PHP   4. drush_dispatch() /Users/mryan/.composer/vendor/drush/drush/lib/Drush/Boot/BaseBoot.php:62
PHP   5. call_user_func_array:{/Users/mryan/.composer/vendor/drush/drush/includes/command.inc:178}() /Users/mryan/.composer/vendor/drush/drush/includes/command.inc:178
PHP   6. drush_command() /Users/mryan/.composer/vendor/drush/drush/includes/command.inc:178
PHP   7. _drush_invoke_hooks() /Users/mryan/.composer/vendor/drush/drush/includes/command.inc:210
PHP   8. call_user_func_array:{/Users/mryan/.composer/vendor/drush/drush/includes/command.inc:359}() /Users/mryan/.composer/vendor/drush/drush/includes/command.inc:359
PHP   9. drush_migrate_upgrade() /Users/mryan/.composer/vendor/drush/drush/includes/command.inc:359
PHP  10. Drupal\migrate_upgrade\MigrateUpgradeDrushRunner->import() /Users/mryan/Sites/d8/modules/contrib/migrate_upgrade/migrate_upgrade.drush.inc:42
PHP  11. drush_op() /Users/mryan/Sites/d8/modules/contrib/migrate_upgrade/src/MigrateUpgradeDrushRunner.php:50
PHP  12. drush_call_user_func_array() /Users/mryan/.composer/vendor/drush/drush/includes/drush.inc:705
PHP  13. call_user_func_array:{/Users/mryan/.composer/vendor/drush/drush/includes/drush.inc:714}() /Users/mryan/.composer/vendor/drush/drush/includes/drush.inc:714
PHP  14. Drupal\migrate\MigrateExecutable->import() /Users/mryan/.composer/vendor/drush/drush/includes/drush.inc:714
PHP  15. Drupal\migrate\Plugin\migrate\destination\EntityContentBase->import() /Users/mryan/Sites/d8/core/modules/migrate/src/MigrateExecutable.php:266
PHP  16. Drupal\migrate\Plugin\migrate\destination\EntityContentBase->save() /Users/mryan/Sites/d8/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php:77
PHP  17. Drupal\Core\Entity\Entity->save() /Users/mryan/Sites/d8/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php:92
PHP  18. Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() /Users/mryan/Sites/d8/core/lib/Drupal/Core/Entity/Entity.php:340
PHP  19. Drupal\Core\Entity\EntityStorageBase->save() /Users/mryan/Sites/d8/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:746
PHP  20. Drupal\Core\Entity\ContentEntityStorageBase->doPreSave() /Users/mryan/Sites/d8/core/lib/Drupal/Core/Entity/EntityStorageBase.php:388
PHP  21. Drupal\Core\Entity\EntityStorageBase->doPreSave() /Users/mryan/Sites/d8/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:258
PHP  22. Drupal\Core\Entity\ContentEntityStorageBase->invokeHook() /Users/mryan/Sites/d8/core/lib/Drupal/Core/Entity/EntityStorageBase.php:434
PHP  23. Drupal\Core\Entity\ContentEntityStorageBase->invokeFieldMethod() /Users/mryan/Sites/d8/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:373
PHP  24. Drupal\Core\Field\FieldItemList->preSave() /Users/mryan/Sites/d8/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:415
PHP  25. Drupal\Core\Field\FieldItemList->delegateMethod() /Users/mryan/Sites/d8/core/lib/Drupal/Core/Field/FieldItemList.php:207
PHP  26. Drupal\image\Plugin\Field\FieldType\ImageItem->preSave() /Users/mryan/Sites/d8/core/lib/Drupal/Core/Field/FieldItemList.php:249

I think this is looking pretty good reviewing the code. Still NW until #29 is fixed.

A couple of small nitpicks, quite small since we discussed much more in IRC.

1. +++ b/core/modules/migrate/tests/src/Unit/MigrationStorageTest.php
   @@ -123,8 +105,8 @@ class TestMigrationStorage extends MigrationStorage {
   +  public function getVariantIds(array $ids) {
   +    return parent::getVariantIds($ids);
      }
   

   This doesn't do anything?

2. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
   @@ -214,28 +204,39 @@ protected function getCckData(array $field, Row $node) {
   +        // that CCK field schemata usually define their most important
   

   s/schemata/schema

3. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
   @@ -214,28 +204,39 @@ protected function getCckData(array $field, Row $node) {
   +        // alter the query directly before it's run, but this will do for
   +        // the time being.
   

   Sounds like we want a follow-up to discuss?

Oh, should have said - interdiffs look good, last patch works fine with my site (no fatal, node with the dangling file reference successfully migrates).

Looks good.

Walked through this with @phenaproxima, and all looks good from my side, other than it's missing a change record. However, Adam has sworn a Blood Oath that this will be written immediately after we're done talking. ;)

Here are some things that came up in review, nothing blocking.

1. +++ b/core/modules/book/src/Tests/Migrate/d6/MigrateBookTest.php
   @@ -29,7 +29,15 @@ protected function setUp() {
   -    $id_mappings = array();
   +    // Create a default bogus mapping for all variants of d6_node.
   +    $id_mappings = array(
   +      'd6_node:*' => array(
   +        array(
   +          array(0),
   +          array(0),
   +        ),
   +      ),
   +    );
   

   WAT? :)

   Adam explained that this is basically mocking the expected results. The reason this is needed is because now we're migrating a story specifically rather than nodes as a whole.

2. +++ b/core/modules/file/src/Plugin/migrate/process/d6/CckFile.php
   @@ -8,40 +8,95 @@
   -class CckFile extends Route implements ContainerFactoryPluginInterface {
   +class CckFile extends ProcessPluginBase implements ContainerFactoryPluginInterface {
   

   Uh.. wat? :) Good to see this cleaned up.

3. +++ b/core/modules/file/src/Plugin/migrate/process/d6/CckFile.php
   @@ -8,40 +8,95 @@
   +    // If the migration plugin completely fails its lookup process, it will
   +    // throw a MigrateSkipRowException. It shouldn't, but that is being dealt
   +    // with at https://www.drupal.org/node/2487568. Until that lands, return
   +    // an empty item.
   +    catch (MigrateSkipRowException $e) {
   +      return [];
   

   I asked about this, this special handling is unique to file fields, only because sometimes those can refer to files that have failed to migrate, and are thus prone to causing issues for remaining migrations, so we do a safety switch here.

4. +++ b/core/modules/link/src/Plugin/migrate/process/d6/CckLink.php
   @@ -45,15 +45,13 @@ public static function create(ContainerInterface $container, array $configuratio
   diff --git a/core/modules/migrate/config/schema/migrate.load.schema.yml b/core/modules/migrate/config/schema/migrate.load.schema.yml
   
   diff --git a/core/modules/migrate/config/schema/migrate.load.schema.yml b/core/modules/migrate/config/schema/migrate.load.schema.yml
   deleted file mode 100644
   

   Need to make sure we warn people in the change record that we no longer use these files and to use builders now instead.

5. +++ b/core/modules/migrate/config/schema/migrate.schema.yml
   @@ -16,9 +16,6 @@ migrate.migration.*:
   -    load:
   -      type: migrate.load.[plugin]
   -      label: 'Source'
   

   Same deal; lets make sure this is part of the change record.

6. +++ b/core/modules/migrate/src/MigrationStorage.php
   @@ -64,52 +64,54 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
   +    // Build an array of dependencies and set the order of the migrations.
   +    return $this->buildDependencyMigration($migrations, []);
   

   Had a question about this, this is just moving code to where it belongs.

7. +++ b/core/modules/migrate/src/MigrationStorage.php
   @@ -64,52 +64,54 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
   -  protected function expandDependencies(array $dependencies) {
   ...
   +  protected function getVariantIds(array $ids) {
   

   Why rename these? The old name was much clearer.

   Rationale: getVariantIds() also used for loading, not just dependencies.

8. +++ /dev/null
   @@ -1,32 +0,0 @@
   -interface SourceEntityInterface {
   

   Change record.

9. +++ b/core/modules/migrate_drupal/migrate_drupal.module
   @@ -8,16 +8,6 @@
   -function migrate_drupal_entity_type_alter(array &$entity_types) {
   

   This was migrate drupal formerly overriding some classes, both of which are gone now, so this is awesome to remove. :)

10. +++ /dev/null
    @@ -1,38 +0,0 @@
    -class Migration extends BaseMigration implements MigrationInterface {
    
    +++ /dev/null
    @@ -1,20 +0,0 @@
    -interface MigrationInterface extends BaseMigrationInterface {
    
    +++ /dev/null
    @@ -1,240 +0,0 @@
    -class MigrationStorage extends BaseMigrationStorage {
    
    +++ /dev/null
    @@ -1,45 +0,0 @@
    -interface MigrateLoadInterface extends PluginInspectionInterface {
    

    This was Migrate Drupal's custom override of Migrate's class, because load plugins were needed. Now we don't need those, so we can just use the parent class. Less code FTW!

11. +++ b/core/modules/user/src/Plugin/migrate/source/d6/User.php
    @@ -18,7 +17,7 @@
    -class User extends DrupalSqlBase implements SourceEntityInterface {
    +class User extends DrupalSqlBase {
    

    Change record.

Committed and pushed to 8.0.x. Thanks!