Add the MigrationPluginManager to Drupal\migrate\Plugin\migrate\id_map\Sql

I don't think this is blocked any more.

Since 8.9.x and 9.0.x are now in beta, I'm moving this to 9.1.x. Is there a way to make this change with BC/deprecation for the continuous upgrade path? Thanks!

The issue probably also does not need to be postponed since it doesn't appear to be postponed on anything specific. Marking active.

Here's a shot at taking care of this. This has an update to an existing Unit test so that it doesn't fail due to the missing service plugin.manager.migrate service. In normal situations there's not a BC problem. It's just the nature of unit tests.

I ran MigrateSqlIdMapTest locally and it's passing. If the deprecation is as graceful as I think it is I wouldn't expect anything else to fail. But we'll see!

Not too graceful apparently! This should be better.

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
@@ -993,18 +1010,6 @@ public function valid() {
-  protected function getMigrationPluginManager() {
-    return \Drupal::service('plugin.manager.migration');
-  }

Need to deprecate this for removal as well, rather than just deleting it.

Hoping to nudge this along. Adding tag because this has deprecations for 9.4.

• 13. Started a CR so we have the URL for the deprecation messages.
• 13. Done, Added the deprecation message to getMigrationPluginManager().
• And changed the name of $migrate_manager and $migrateManager to include the word plugin so that it is very clear that this manager is not the MigratePluginManager.

Had a coding standards errors, just ignore the patch in #15

Still need to trigger a deprecation error in getMigrationPluginManager, I think, unless we are doing that automatically from the annotation now. I confess it's been a while since I've done a deprecation..

Writing the test for that took most of the time :-)

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
@@ -996,12 +1013,16 @@ public function valid() {
+    @trigger_error("deprecated in drupal:9.4.0 and is removed from drupal:10.0.0. No direct replacement is provided. See https://www.drupal.org/node/3277306", E_USER_DEPRECATED);

Let's push to get this in 9.5. We try to avoid deprecations in X.last, but this class is not one that would normally be extended, so it should be fine.

Also, there is a replacement, `use $this-migrationPluginManager instead`

Here are the updates requested by @mikelutz.

Ugh. This patch changes the double quotes to single quotes so there's no interpolation in the deprecation warning.

*cough*

--- a/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
@@ -176,6 +186,12 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
     $this->mapTableName = mb_substr($this->mapTableName, 0, 63 - $prefix_length);
     $this->messageTableName = 'migrate_message_' . mb_strtolower($machine_name);
     $this->messageTableName = mb_substr($this->messageTableName, 0, 63 - $prefix_length);
+
+    if (!$migration_plugin_manager) {
+      @trigger_error('Calling Sql::__construct() without the $migration_manager argument is deprecated in drupal:9.4.0 and the $migration_manager argument will be required in drupal:10.0.0. See https://www.drupal.org/node/3277306', E_USER_DEPRECATED);

Still mentioning is deprecated in drupal:9.4.0
*cough*

Oof. Thanks, @Spokje. Let's try this one.

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
@@ -176,6 +186,12 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
+      @trigger_error('Calling Sql::__construct() without the $migration_manager argument is deprecated in drupal:9.5.0 and the $migration_manager argument will be required in drupal:10.0.0. See https://www.drupal.org/node/3277306', E_USER_DEPRECATED);

We still need a test to trigger this deprecation

@mikelutz, I don't have a good reference for this claim, but deprecations related to new arguments in the constructor are not generally covered by automated tests. There are a handful of tests for stuff like that, but mostly not. I think we should be ok.

Hmm, I haven't heard of that policy change. I've always written a test when I've added parameters to a constructor.

I don't see an exception listed in the how to deprecate instructions, which are right below the list of what can be deprecated, which includes constructor parameter additions. I don't see anything either way in the specifc constructor parameter addition instructions, which isn't unexpected, none of the other sections really mention testing either. Is this the most recent deprecation policy, or is there another policy that I'm missing here?

Unsure about any policy change on deprecations, but at the very least I think this issue needs a 10.0.x patch, with removal of the deprecations made in the 9.5.x patch.

Agreed with @Spokje, though there is a chance the RMs might want to put this in the new 10.1.x anyway, so we should probably find out.

Sense = made.

Here's a new patch for 9.5.x that includes a test for the deprecation in the constructor. I'm also including a patch for 10.0.x with the deprecations removed.

@mikelutzt, @Spokje, any ideas on the custom commands failure in the 10.0.x patch in #33?

--- Commands Executed ---
core/scripts/dev/commit-code-check.sh --drupalci
Return Code: 1
--- Output ---
Drupal's JavaScript development dependencies are not installed or cannot be resolved. Run 'yarn install' inside the core directory, or 'yarn check -s' to list other errors.
--- Errors ---

I've seen it before, but never found out what's exactly wrong. I always blame it on the particular GitLab runner doing the test being wonky and retry, hoping it gets run on another one.

In the other retry there are real dragons:

10.0.x-2919158-33.patch test with PHP 8.1 & MySQL 5.7

Created 29 Jun 2022 at 17:34 CEST. danflanagan8 added custom test. Updated 29 Jun 2022 at 17:42 CEST.

Custom Commands Failed - View results on dispatcher

----------------------------------------------------------------------------------------------------

Running PHPStan on *all* files.
 ------ ----------------------------------------------------------------------- 
  Line   core/modules/migrate/tests/src/Unit/TestSqlIdMap.php                   
 ------ ----------------------------------------------------------------------- 
  98     Call to an undefined static method                                     
         Drupal\migrate\Plugin\migrate\id_map\Sql::getMigrationPluginManager()  
         .                                                                      
 ------ ----------------------------------------------------------------------- 


 [ERROR] Found 1 error                                                          


PHPStan: failed


FILE: ...html/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 5 | WARNING | [x] Unused use statement
 7 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

That was all kinds of bad. This should be better. Apologies.

@mikelutz had too much faith in my abilities when he said in Slack:

If you took the time you’ve spent arguing and just written the test, you’d be done by now.

I was right to try to get around doing this!

Looking good, one nit:

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
@@ -996,12 +1013,16 @@ public function valid() {
   protected function getMigrationPluginManager() {
+    @trigger_error('deprecated in drupal:9.5.0 and is removed from drupal:10.0.0. Use $this->migrationPluginManager instead. See https://www.drupal.org/node/3277306', E_USER_DEPRECATED);
     return \Drupal::service('plugin.manager.migration');
   }
 

This should return $this-migrationPluginManager. We've ensured it's initialized in the constructor to the service in the container, so no need to go back to the container again here.

I think that's a worthy nit, @mikelutz. Here's the updated patch for 9.5.x. I'm also re-posting the 10.0.x patch from #37 but with a new name, just to try to keep the most up-to-date patches in the same place.

Thanks. I tweaked the CR, lets get this in front of committers and hope they will allow a small deprecation like this in 9.5.

That recent fail was clearly unrelated to this issue:

1) Drupal\Tests\quickedit\FunctionalJavascript\InlineFormErrorsIntegrationTest::testDisabledInlineFormErrors
WebDriver\Exception\JavaScriptError: javascript error: Drupal.quickedit.editors[editorName] is not a constructor
  (Session info: headless chrome=98.0.4758.102)
  (Driver info: chromedriver=98.0.4758.102 (273bf7ac8c909cde36982d27f66f3c70846a3718-refs/branch-heads/4758@{#1151}),platform=Linux 4.9.0-0.bpo.6-amd64 x86_64)

Resetting status to RTBC.

Unrelated failure in Drupal\Tests\book\Functional\BookTest::testGetTableOfContents. Resetting to RTBC.

Discussed with @catch as we agreed that deprecating in Drupal 9.5 but marking for removal for D11 is the way to go here.

diff --git a/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
index e648a8e586..4b76e4fbc6 100644
--- a/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
@@ -188,7 +188,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
     $this->messageTableName = mb_substr($this->messageTableName, 0, 63 - $prefix_length);
 
     if (!$migration_plugin_manager) {
-      @trigger_error('Calling Sql::__construct() without the $migration_manager argument is deprecated in drupal:9.5.0 and the $migration_manager argument will be required in drupal:10.0.0. See https://www.drupal.org/node/3277306', E_USER_DEPRECATED);
+      @trigger_error('Calling Sql::__construct() without the $migration_manager argument is deprecated in drupal:9.5.0 and the $migration_manager argument will be required in drupal:11.0.0. See https://www.drupal.org/node/3277306', E_USER_DEPRECATED);
       $migration_plugin_manager = \Drupal::service('plugin.manager.migration');
     }
     $this->migrationPluginManager = $migration_plugin_manager;
@@ -1017,13 +1017,13 @@ public function valid() {
    * @return \Drupal\migrate\Plugin\MigrationPluginManagerInterface
    *   The migration plugin manager.
    *
-   * @deprecated in drupal:9.5.0 and is removed from drupal:10.0.0. Use
-   * $this->migrationPluginManager instead.
+   * @deprecated in drupal:9.5.0 and is removed from drupal:11.0.0. Use
+   *   $this->migrationPluginManager instead.
    *
    * @see https://www.drupal.org/node/3277306
    */
   protected function getMigrationPluginManager() {
-    @trigger_error('deprecated in drupal:9.5.0 and is removed from drupal:10.0.0. Use $this->migrationPluginManager instead. See https://www.drupal.org/node/3277306', E_USER_DEPRECATED);
+    @trigger_error('deprecated in drupal:9.5.0 and is removed from drupal:11.0.0. Use $this->migrationPluginManager instead. See https://www.drupal.org/node/3277306', E_USER_DEPRECATED);
     return $this->migrationPluginManager;
   }
 
diff --git a/core/modules/migrate/tests/src/Kernel/Plugin/id_map/SqlDeprecationTest.php b/core/modules/migrate/tests/src/Kernel/Plugin/id_map/SqlDeprecationTest.php
index 369252deff..4bdab83ec4 100644
--- a/core/modules/migrate/tests/src/Kernel/Plugin/id_map/SqlDeprecationTest.php
+++ b/core/modules/migrate/tests/src/Kernel/Plugin/id_map/SqlDeprecationTest.php
@@ -23,7 +23,7 @@ class SqlDeprecationTest extends KernelTestBase {
    */
   public function testOptionalParametersDeprecation(): void {
     $migration = $this->prophesize('\Drupal\migrate\Plugin\MigrationInterface')->reveal();
-    $this->expectDeprecation('Calling Sql::__construct() without the $migration_manager argument is deprecated in drupal:9.5.0 and the $migration_manager argument will be required in drupal:10.0.0. See https://www.drupal.org/node/3277306');
+    $this->expectDeprecation('Calling Sql::__construct() without the $migration_manager argument is deprecated in drupal:9.5.0 and the $migration_manager argument will be required in drupal:11.0.0. See https://www.drupal.org/node/3277306');
     new Sql(
       [],
       'sql',
diff --git a/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php
index 83669708d0..f05ed19c78 100644
--- a/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php
+++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php
@@ -1190,7 +1190,7 @@ public function testGetMigrationPluginManagerDeprecation() {
     $container->set('plugin.manager.migration', $migration_plugin_manager);
     \Drupal::setContainer($container);
 
-    $this->expectDeprecation('deprecated in drupal:9.5.0 and is removed from drupal:10.0.0. Use $this->migrationPluginManager instead. See https://www.drupal.org/node/3277306');
+    $this->expectDeprecation('deprecated in drupal:9.5.0 and is removed from drupal:11.0.0. Use $this->migrationPluginManager instead. See https://www.drupal.org/node/3277306');
     $id_map = $this->getIdMap();
     $id_map->getMigrationPluginManager();
   }

Fixed this on commit :)

diff --git a/core/modules/migrate/tests/src/Unit/TestSqlIdMap.php b/core/modules/migrate/tests/src/Unit/TestSqlIdMap.php
index be99becdcc..433b3907ee 100644
--- a/core/modules/migrate/tests/src/Unit/TestSqlIdMap.php
+++ b/core/modules/migrate/tests/src/Unit/TestSqlIdMap.php
@@ -95,7 +95,7 @@ public function ensureTables() {
    * {@inheritdoc}
    */
   public function getMigrationPluginManager() {
-    parent::getMigrationPluginManager();
+    return parent::getMigrationPluginManager();
   }
 
 }

Fixed this PHPStan error on commit. Nice catch PHPStan!

Committed and pushed 275a32813b to 10.1.x and abea4c7b5d to 10.0.x and 409a884bd7 to 9.5.x. Thanks!