Migrate Drupal source plugins should use dependency injection

Fix some (but probably not all) of the test failures.

Mop up a few more test failures.

NICE work! Self-assigning for review.

Looks good except for two type hints. However, I think this is going to need some major committer approval because it is a BC break. Even though DrupalSqlBase is technically experimental, a lot of source plugins scattered throughout core are extending it. It is heartening to see the tests pass, though, so maybe there's hope.

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
   @@ -44,11 +46,27 @@
   +   * @var \Drupal\Core\Cache\CacheBackendInterface|\PHPUnit_Framework_MockObject_MockObject
   

   This should not carry the PHPUnit_Framework_MockObject_MockObject type hint. That's only used in testing.

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/EmptySource.php
   @@ -30,11 +31,27 @@ class EmptySource extends BaseEmptySource implements ContainerFactoryPluginInter
   +   * @var \Drupal\Core\Cache\CacheBackendInterface|\PHPUnit_Framework_MockObject_MockObject
   

   Same here.

Patch lookin' good, but this will get kicked back for sure without a change record.

Change record written: https://www.drupal.org/node/2859980

I think the change record needs some work - no one is doing "new DrupalSqlBase", but what they are doing is extending DrupalSqlBase as the node plugin in the patch does. Anyone with a source plugin extending DrupalSqlBase that implements __construct() and/or create() will need to make changes to accommodate this patch. Just showing the node plugin changes should address this.

I'm just a bit wary here - while migrate_drupal is alpha-experimental and thus *allowed* to make hard BC breaks, to approach stability we have tried to avoid those with real-world impacts, and this one will break real source plugins in the wild. I do think it's worthwhile, but just want to explicitly acknowledge that this is a bigger BC break as a practical matter than we've had for a while.

Change record updated with more detailed info, plus a list of plugins directly affected by this change.

I also acknowledge this is a major BC break, although an allowed one -- in my view it's better to get this done now, while we still can, and be finished with it. If the committers disagree, they can mark this for Drupal 9.x.

I'm just a bit wary here - while migrate_drupal is alpha-experimental and thus *allowed* to make hard BC breaks, to approach stability we have tried to avoid those with real-world impacts, and this one will break real source plugins in the wild. I do think it's worthwhile, but just want to explicitly acknowledge that this is a bigger BC break as a practical matter than we've had for a while.

The problem is migrate_drupal is not a regular experimental module. It would be wonderful if we could achieve this without breaking everything.

1. +++ b/core/modules/migrate/tests/src/Unit/MigrateSqlSourceTestCase.php
   @@ -112,7 +114,7 @@ protected function setUp() {
   -    $plugin = new $plugin_class($this->migrationConfiguration['source'], $this->migrationConfiguration['source']['plugin'], [], $migration, $state, $entity_manager);
   +    $plugin = new $plugin_class($this->migrationConfiguration['source'], $this->migrationConfiguration['source']['plugin'], [], $migration, $module_handler, $cache, $state, $entity_manager);
   

   Shouldn't this just do $plugin::create($container);

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
   @@ -44,11 +46,27 @@
   -  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, StateInterface $state, EntityManagerInterface $entity_manager) {
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, ModuleHandlerInterface $module_handler, CacheBackendInterface $cache, StateInterface $state, EntityManagerInterface $entity_manager) {
   
   @@ -84,6 +102,8 @@ public static function create(ContainerInterface $container, array $configuratio
   +      $container->get('module_handler'),
   +      $container->get('cache.migrate'),
   

   I don't get why we not adding the new params on the end? Especially as state is passed up to the parent constructor. But the other thing I don't understand it why we're adding dependencies here that the abstract doesn't depend on... oh I see it's because it does via \Drupal\migrate\Plugin\migrate\source\SourcePluginBase. Well that's not simple.

3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/EmptySource.php
   @@ -30,11 +31,27 @@ class EmptySource extends BaseEmptySource implements ContainerFactoryPluginInter
   -  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, EntityManagerInterface $entity_manager) {
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, ModuleHandlerInterface $module_handler, CacheBackendInterface $cache, EntityManagerInterface $entity_manager) {
        parent::__construct($configuration, $plugin_id, $plugin_definition, $migration);
        $this->entityManager = $entity_manager;
   +    $this->moduleHandler = $module_handler;
   +    $this->cache = $cache;
      }
   

   I don't understand this change. This plugin is not dependent on either of the new dependencies as far as I can see. Oh same mistake as above - it is dependent just via \Drupal\migrate\Plugin\migrate\source\SourcePluginBase. Hopefully #2791041: Migrate source plugins should use dependency injection will make the injection all the way up to there.

4. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
   @@ -49,8 +50,8 @@ class Node extends DrupalSqlBase {
        $this->moduleHandler = $module_handler;
   

   This assignment is is not required now right? In fact can't the entire constructor and create be removed?

5. +++ b/core/modules/node/src/Plugin/migrate/source/d7/Node.php
   @@ -31,8 +31,8 @@ class Node extends FieldableEntity {
   -  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, StateInterface $state, EntityManagerInterface $entity_manager, ModuleHandlerInterface $module_handler) {
   -    parent::__construct($configuration, $plugin_id, $plugin_definition, $migration, $state, $entity_manager);
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, CacheBackendInterface $cache, StateInterface $state, EntityManagerInterface $entity_manager, ModuleHandlerInterface $module_handler) {
   +    parent::__construct($configuration, $plugin_id, $plugin_definition, $migration, $module_handler, $cache, $state, $entity_manager);
   
   @@ -45,6 +45,7 @@ public static function create(ContainerInterface $container, array $configuratio
   +      $container->get('cache.migrate'),
   

   Same here I think this can be removed.

Patch in #9 no longer applies so re-rolled before addressing @alexpott's comments in #14.

Addressing @alexpott's comments in #14:

1. Done
2. Can anything be done to make this more 'simple'?
3. #2791041: Migrate source plugins should use dependency injection is no longer targeted at D8, so is there anything else we can do to improve the clarity of this change?
4. $this->moduleHandler is required in query() to access moduleExists().
5. Similarly, $this->moduleHandler is required in query() to access moduleExists().

Address test failures as a result of #14.1.

This probably won't see much attention at this point in the lifecycle of Migrate, since it requires a BC break with little value. To make that a little more clear, moving to 9.x.

And postponing until we get closer to the date.

This would need to be done with BC/deprecation and would be a minor-only change. Since 8.9.x and 9.0.x are now in beta, I'm moving this to 9.1.x. Thanks!

Marking active since it's not postponed on anything specific.

There is a reroll over in #2791041-51: Migrate source plugins should use dependency injection, that has the migrate_drupal parts for this issue.

In #20, heddn commented "it requires a BC break with little value", therefore setting to postponed until discussed at a migrate meeting. I have added a comment to #3281351: [meeting] Migrate Meeting 2022-05-26 2100Z.