Migrate source plugins should use dependency injection

How or does this effect contrib and custom source plugins? Me thinks it wouldn't.

Moving to 8.2.x - we are trying very hard to stop BC breaks (move migrate to experimental beta status and commit to a stable API) with 8.2.

Created patch from patch 11 for branch 8.2.x

If this is going to get into migrate, then this will need to get in before it bumps from experimental.

Discussed with @phenaproxima and removing this as a blocker from beta. While it is a BC break, it should not block beta.

#18 needs reroll.

Your branch is up-to-date with 'origin/8.2.x'.
$ curl https://www.drupal.org/files/issues/2791041-18.patch | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 32703  100 32703    0     0  48926      0 --:--:-- --:--:-- --:--:-- 48956
patching file core/modules/migrate/src/Plugin/migrate/source/EmbeddedDataSource.php
patching file core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
Hunk #1 succeeded at 2 with fuzz 2.
Hunk #2 succeeded at 14 (offset 2 lines).
Hunk #3 FAILED at 26.
Hunk #4 succeeded at 150 with fuzz 1 (offset 10 lines).
Hunk #5 succeeded at 178 (offset 13 lines).
Hunk #6 succeeded at 200 (offset 13 lines).
Hunk #7 FAILED at 405.
Hunk #8 FAILED at 418.
3 out of 8 hunks FAILED -- saving rejects to file core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php.rej
patching file core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
patching file core/modules/migrate/tests/src/Unit/MigrateSourceTest.php
Hunk #1 FAILED at 89.
Hunk #2 FAILED at 126.
Hunk #3 FAILED at 258.
Hunk #4 succeeded at 305 (offset 26 lines).
Hunk #5 succeeded at 315 (offset 26 lines).
Hunk #6 FAILED at 326.
Hunk #7 succeeded at 339 (offset 26 lines).
Hunk #8 FAILED at 355.
Hunk #9 succeeded at 368 (offset 26 lines).
Hunk #10 FAILED at 384.
Hunk #11 succeeded at 397 (offset 26 lines).
Hunk #12 succeeded at 453 (offset 41 lines).
6 out of 12 hunks FAILED -- saving rejects to file core/modules/migrate/tests/src/Unit/MigrateSourceTest.php.rej
patching file core/modules/migrate/tests/src/Unit/MigrateSqlSourceTestCase.php
Hunk #1 FAILED at 2.
Hunk #2 succeeded at 85 with fuzz 2 (offset 7 lines).
Hunk #3 succeeded at 123 (offset 28 lines).
1 out of 3 hunks FAILED -- saving rejects to file core/modules/migrate/tests/src/Unit/MigrateSqlSourceTestCase.php.rej
patching file core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
patching file core/modules/migrate_drupal/src/Plugin/migrate/source/EmptySource.php
patching file core/modules/migrate_drupal/src/Plugin/migrate/source/Variable.php
patching file core/modules/migrate_drupal/src/Plugin/migrate/source/d6/i18nVariable.php
patching file core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
Hunk #2 succeeded at 46 with fuzz 1 (offset -1 lines).
patching file core/modules/migrate_drupal/tests/src/Unit/source/d6/Drupal6SqlBaseTest.php

adding updated patch.

Re-roll (but will still fail tests - further patch pending...).

Set missing module handler property of SourcePluginBase.

1. Set missing cache property of SourcePluginBase.
2. Correctly mock-up container in MigrateSqlSourceTestCase.
3. Add missing dependencies to d6/Node.

Correct final 2 test failures.

1. +++ b/core/modules/migrate/src/Plugin/migrate/source/EmbeddedDataSource.php
   @@ -1,11 +1,19 @@
   + * Expects at least two configuration values:
   

   Let's make this similar to the process plugin documentation.
   s/Expects at least two configuration values:/Available configuration keys:/

2. +++ b/core/modules/migrate/src/Plugin/migrate/source/EmbeddedDataSource.php
   @@ -1,11 +1,19 @@
   + *
   

   Remove blank line.

3. +++ b/core/modules/migrate/src/Plugin/migrate/source/EmbeddedDataSource.php
   @@ -1,11 +1,19 @@
   + * - 'data_rows': Array of data rows, each one an array of values keyed by
   

   s/'data_rows'/data_rows/

4. +++ b/core/modules/migrate/src/Plugin/migrate/source/EmbeddedDataSource.php
   @@ -1,11 +1,19 @@
   + *
   

   Remove blank line.

5. +++ b/core/modules/migrate/src/Plugin/migrate/source/EmbeddedDataSource.php
   @@ -1,11 +1,19 @@
   + * - 'ids': Array describing the unique ID fields for the source data, keyed
   

   s/'ids'/id/

Comment corrections made.

1. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
   @@ -146,7 +150,7 @@
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, ModuleHandlerInterface $module_handler, CacheBackendInterface $cache) {
   

   I don't see how this can be done without breaking BC.

2. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
   @@ -169,6 +173,24 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   +
   +    $this->moduleHandler = $module_handler;
   +
   +    $this->cache = $cache;
   

   Nit: not sure we need all this extra whitespace here.

3. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -62,8 +63,8 @@
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, ModuleHandlerInterface $module_handler, CacheBackendInterface $cache, StateInterface $state) {
   

   This feels like a BC break. Lots of contrib and custom migrations extend from SqlBase and might be calling parent. With this addition, it will break them.

Discussed with @phenaproxima in IRC and followed up with splitting out the migrate_drupal part since it's not in beta yet.

#2853647: Migrate Drupal source plugins should use dependency injection

Let's postpone this.

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

I am taking this issue and try to resolve this.

There has not been progress in a year, so I'm unassigning it.

Reroll patch against 9.5
Attached reroll diff against #34

Reroll patch against #49
Attached interdiff

Reroll patch against #50
Attached interdiff

@immaculatexavier, thanks for working on this issue! Have a look at the instructions for running the coding standard checks locally so you can be sure the tests will run before uploading a patch. It also has the advantage of saving resources, including money, for the Drupal association. There are real costs for running the tests.

It is good practice to test any tests that your patch changes before uploading your patch to an issue. Running PHPUnit tests.

According to #38. This will need a reroll after
#2853647: Migrate Drupal source plugins should use dependency injection. The other issue is not yet committed. The work in the latest patch includes work that should be in the other issue.

This is needs work to move the migrate_drupal parts to the other issue.

Actually, because of #20 in the other issue, I am postponing until discussed at a migrate meeting. I have added a comment to #3281351: [meeting] Migrate Meeting 2022-05-26 2100Z.