Ignore: Migration plugin testing issue

So here's a shallow, partial review. I found some minor documentation issues, but I need to review this more thoroughly later. In the meantime, it's still a work-in-progress so I feel OK about where it's heading.

1. +++ b/core/modules/comment/src/Tests/Migrate/d7/MigrateCommentTypeTest.php
   @@ -62,7 +62,7 @@ public function testMigration() {
   +    $migration = $this->container->get('plugin.manager.migration')->createInstance('d7_comment_type');
   

   Call me paranoid, but just in case the service ID changes at some point in the future (or some other unforeseen snafu)...I think this should be a utility method on MigrateTestBase.

2. +++ b/core/modules/file/src/Tests/Migrate/d6/FileMigrationTestTrait.php
   @@ -0,0 +1,34 @@
   +/**
   + * @file
   + * Contains
   + */
   

   Contains null? The suspense is killing me :)

3. +++ b/core/modules/file/src/Tests/Migrate/d6/FileMigrationTestTrait.php
   @@ -0,0 +1,34 @@
   +trait FileMigrationTestTrait {
   

   Missing a doc comment.

4. +++ b/core/modules/migrate/src/Plugin/Migration.php
   @@ -251,6 +231,25 @@ class Migration extends ConfigEntityBase implements MigrationInterface, Requirem
   +  public function __construct(array $configuration, $plugin_id, array $plugin_definition) {
   +    parent::__construct($configuration, $plugin_id, $plugin_definition);
   +    foreach ($plugin_definition as $key => $value) {
   +      $this->$key = $value;
   +    }
   +  }
   +
   +  public function id() {
   +    return $this->pluginId;
   +  }
   +
   +  public function get($key) {
   +    return isset($this->$key) ? $this->$key : NULL;
   +  }
   +
   +  public function getIdMapPlugin() {
   +    return $this->idMapPlugin;
   +  }
   

   All missing doc comments.

5. +++ b/core/modules/migrate/src/Plugin/Migration.php
   @@ -578,21 +578,11 @@ public function trustData() {
   +    foreach (parent::getPluginDefinition() as $key => $value) {
   +      $definition[$key] = isset($this->$key) ? $this->$key : $value;
        }
   

   Not entirely sure what's going on here; can you provide a comment?

6. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
   @@ -0,0 +1,71 @@
   +/**
   + * @file
   + * Contains \Drupal\migrate\Plugin\MigrationPluginManager.
   + */
   +
   

   Nit: There's an extra blank line after the comment.

7. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
   @@ -0,0 +1,71 @@
   +  /**
   +   * Provides default values for a contextual link definition.
   +   *
   +   * @var array
   +   */
   

   Contextual links? I don't think so :)

8. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
   @@ -0,0 +1,71 @@
   +  protected $pluginInterface = 'Drupal\migrate\Entity\MigrationInterface';
   

   Needs a comment.

9. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
   @@ -0,0 +1,71 @@
   +  /**
   +   * @var \Drupal\Core\Extension\ModuleHandlerInterface
   +   */
   +  protected $moduleHandler;
   

   Missing description.

10. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
    @@ -0,0 +1,71 @@
    +  public function createDerivativeInstances($base_plugin_id, array $configuration = array()) {
    +    $factory = $this->getFactory();
    +    $instances = [];
    +    foreach (preg_grep('/^' . $base_plugin_id . ':/', array_keys($this->getDefinitions())) as $plugin_id) {
    +      $instances[$plugin_id] = $factory->createInstance($plugin_id, isset($configuration[$plugin_id]) ? $configuration[$plugin_id] : []);
    +    }
    +    return $instances;
    +  }
    

    This seems like a good place to use a plugin collection. Also, it's implementing a method on an interface which the class does not actually implement.

11. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManagerInterface.php
    @@ -0,0 +1,31 @@
    +interface MigrationPluginManagerInterface extends PluginManagerInterface {
    

    Is this actually implemented anywhere, or is it old cruft from a previous generation of the patch?

12. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManagerInterface.php
    @@ -0,0 +1,31 @@
    +   * @return object[]
    +   *   Fully configured plugin instances.
    +   *
    +   * @throws \Drupal\Component\Plugin\Exception\PluginException
    +   *   If the instance cannot be created, such as if the ID is invalid.
    +   */
    +  public function createDerivativeInstances($base_plugin_id, $configuration = array());
    

    I think this should return a plugin collection. Collections are, best I can figure, designed for exactly this sort of use, so better to not reinvent that.

13. +++ b/core/modules/migrate/src/Plugin/MigrationTemplateDiscovery.php
    @@ -0,0 +1,20 @@
    +namespace Drupal\migrate\Plugin;
    +
    

    Nit: Extra blank line.

14. +++ b/core/modules/migrate/src/Plugin/MigrationTemplateDiscovery.php
    @@ -0,0 +1,20 @@
    +class MigrationTemplateDiscovery extends YamlDiscovery {
    

    I get the sense that this class is only around for backwards compatibility. It should have a doc comment explaining that and marking it @deprecated.

15. +++ b/core/modules/migrate/src/Plugin/MigrationTemplateDiscovery.php
    @@ -0,0 +1,20 @@
    +  function __construct(array $directories) {
    +    $this->discovery = new MigrationYamlDiscovery($directories);
    +  }
    

    Should be public, needs a doc comment.

16. +++ b/core/modules/migrate/src/Plugin/migrate/process/Migration.php
    @@ -8,6 +8,7 @@
     use Drupal\Core\Entity\EntityStorageInterface;
    

    Is EntityStorageInterface still used? If not, should be removed.

17. +++ b/core/modules/migrate/src/Plugin/migrate/process/Migration.php
    @@ -38,11 +39,16 @@ class Migration extends ProcessPluginBase implements ContainerFactoryPluginInter
       /**
    +   * @var \Drupal\Component\Plugin\PluginManagerInterface
    +   */
    +  protected $migrationPluginManager;
    

    Needs a description.

18. +++ b/core/modules/migrate/src/Tests/MigrationTest.php
    @@ -7,16 +7,16 @@
    -class MigrationTest extends KernelTestBase {
    +class MigrationTestDeleted extends KernelTestBase {
    

    "MigrationTestDeleted"? #wat

19. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/CckMigration.php
    @@ -0,0 +1,108 @@
    +namespace Drupal\migrate_drupal\Plugin\migrate;
    +
    

    Nit: There's an extra blank line after this.

20. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/CckMigration.php
    @@ -0,0 +1,108 @@
    +  /**
    +   * @var bool
    +   */
    +  protected $init = FALSE;
    

    Needs a description.

21. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/CckMigration.php
    @@ -0,0 +1,108 @@
    +  /**
    +   * @var \Drupal\Component\Plugin\PluginManagerInterface
    +   */
    +  protected $migrationPluginManager;
    

    Also needs a description.

22. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/D6NodeDeriver.php
    @@ -0,0 +1,148 @@
    +  /**
    +   * @param $source_plugin_id
    +   * @return \Drupal\migrate\Plugin\MigrateSourceInterface
    +   */
    +  protected function getSourcePlugin($source_plugin_id) {
    

    Missing description in the doc comment.

#2625696: Make migrations themselves plugins instead of config entities is long closed now.