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

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.

alexpott → credited xjm → .

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 📌 Add phpstan/phpstan-phpunit as a dev dependency Fixed ).
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!

Spokje → closed merge request !3008

Automatically closed - issue fixed for 2 weeks with no activity.