Deprecate calling Drupal\block\Plugin\migrate\process\BlockTheme::_construct() with the $migration argument

I've added an inline comment in the MR

We could do

public function __construct(array $configuration, $plugin_id, $plugin_definition, ?MigrationInterface $migration = NULL, Config $theme_config = NULL, array $themes = NULL) {
  if ($migration !== NULL) {
    @trigger_error('Calling ' . __CLASS__ . '::_construct() with the $migration argument is deprecated in drupal:10.1.0 and is removed in drupal:11.0.0. See https://www.drupal.org/node/3323212', E_USER_DEPRECATED);
  }
  assert($theme_config instanceof Config);
  assert(is_array($themes));
  ...

so we don't lose anything in terms of strictness, PHPStan/PHPUnit will ensure type is respected. Then in D11 clean it all up.

Again, IMHO PHPStan will force us to reconsider some of our current standards...

You might temporarily change the deprecation trigger to an exception and look at the stack trace that gets out of the tests from running on DrupalCI

I found sometimes the test jobs execute against a repo that is not yet including the last MR commit. Tried to retrigger the test.

looks good to me now, let’s bring to committers’ eyes - there’s a different approach than standard re. deprecation of individual function parameters

The constructor arguments have to lose their typehints, or downstream callers cannot actually update the method call before Drupal 11? The constructor then has to handle arguments both with and without the $migration parameter present by checking the type of the 4th arg?.

(It seems unlikely there is a downstream caller, I wish we just made most plugin constructors internal and didn't have to do this dance)

Seems like a valid deprecation with no failures.

I reviewed the MR and left a comment that needs addressing.

Added the type back and left a comment.

Reviewing the MR

All the threads appear to be address. The one open one appears to be a comment

Ran just the tests locally and did get a failure

Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 @expectedDeprecation:
-%A  Calling Drupal\block\Plugin\migrate\process\BlockTheme::__construct() with the $migration argument is deprecated in drupal:10.1.0 and is removed in drupal:11.0.0. See https://www.drupal.org/node/3323212
-  Method "Behat\Mink\Element\ElementInterface::waitFor()" might add "mixed" as a native return type declaration in the future. Do the same in implementation "Drupal\Tests\DocumentElement" now to avoid errors or add an explicit @return annotation to suppress this message.
+  Method "Behat\Mink\Element\ElementInterface::getText()" might add "string" as a native return type declaration in the future. Do the same in implementation "Drupal\Tests\DocumentElement" now to avoid errors or add an explicit @return annotation to suppress this message.
+  Method "Behat\Mink\Element\ElementInterface::waitFor()" might add "mixed" as a native return type declaration in the future. Do the same in implementation "Drupal\Tests\DocumentElement" now to avoid errors or add an explicit @return annotation to suppress this message.
+  Method "Behat\Mink\Element\ElementInterface::getText()" might add "string" as a native return type declaration in the future. Do the same in implementation "Drupal\Tests\DocumentElement" now to avoid errors or add an explicit @return annotation to suppress this message.
+  Method "Behat\Mink\Element\ElementInterface::waitFor()" might add "mixed" as a native return type declaration in the future. Do the same in implementation "Drupal\Tests\DocumentElement" now to avoid errors or add an explicit @return annotation to suppress this message.
+  The "PHPUnit\Framework\TestCase::addWarning()" method is considered internal This method is not covered by the backward compatibility promise for PHPUnit. It may change without further notice. You should not extend it from "Drupal\KernelTests\KernelTestBase".
+  The "Drupal\Tests\Listeners\DrupalListener" class implements "PHPUnit\Framework\TestListener" that is deprecated.
+  The "Drupal\Tests\Listeners\DrupalListener" class uses "PHPUnit\Framework\TestListenerDefaultImplementation" that is deprecated The `TestListener` interface is deprecated.

Deprecation call is correct version
All appears good to me

Good work!

FYI, the policy for constructor parameter removal has been updated during the life of this issue.

Setting to NW for the questions raised in the MR.

Working on this.

I think the questions in the MR are superceded by latest changes.

From what I can tell the open threads have been addressed.

There's no need for the test to be a kernel test - it can be a unit test. Doing the following fixes this...

diff --git a/core/modules/block/tests/src/Unit/Plugin/migrate/process/BlockThemeDeprecationTest.php b/core/modules/block/tests/src/Unit/Plugin/migrate/process/BlockThemeDeprecationTest.php
index 327743f2ae..05eddff578 100644
--- a/core/modules/block/tests/src/Unit/Plugin/migrate/process/BlockThemeDeprecationTest.php
+++ b/core/modules/block/tests/src/Unit/Plugin/migrate/process/BlockThemeDeprecationTest.php
@@ -1,11 +1,11 @@
 <?php
 
-namespace Drupal\Tests\block\Kernel\Plugin\migrate\process;
+namespace Drupal\Tests\block\Unit\Plugin\migrate\process;
 
 use Drupal\block\Plugin\migrate\process\BlockTheme;
 use Drupal\Core\Config\Config;
-use Drupal\KernelTests\KernelTestBase;
 use Drupal\migrate\Plugin\MigrationInterface;
+use Drupal\Tests\UnitTestCase;
 
 /**
  * Tests the deprecation notices of the block theme.
@@ -14,7 +14,7 @@
  *
  * @group block
  */
-class BlockThemeDeprecationTest extends KernelTestBase {
+class BlockThemeDeprecationTest extends UnitTestCase {
 
   /**
    * Tests the deprecation in the constructor.

But actually I think there are some further things to do...

    assert($theme_config instanceof Config);
    assert(is_array($themes));

isn't the invalid arguments I asked for originally... but we have property types... and in this class we have...

  /**
   * Contains the system.theme configuration object.
   *
   * @var \Drupal\Core\Config\Config
   */
  protected $themeConfig;

  /**
   * List of themes available on the destination.
   */
  protected array $themes;

So the assert(is_array($themes)); is pointless and I would argue that we should take this opportunity to add the property typehint to protected $themeConfig; and whilst we're here we should remove protected $configFactory; because it is never set.

done #25 (dunno why the MR pushes are now displaced vs d.org comments), then some:

1) removed @coversDefaultClass from test since it is only testing deprecation, and we saw it is confusing if left in (see #3326239: Add phpstan/phpstan-phpunit as a dev dependency).
2) made the array declaration a bit more stringent string[].

Change looks fine to me but been wrong twice before

Committed f979f42 and pushed to 10.1.x. Thanks!