Handle block migration for modules moved to contrib

I've been testing with d6_block.yml. When aggregator is not installed the migration fails when saving the entity with a SchemaIncompleteException. Presumably, the same will happen with forum, although I have not tested that. (Not having drush is inconvenient). So, I thought I would put a wrapper around the import in a \Drupal\block\Plugin\migrate\destination\EntityBlock to catch that exception and rethrow it as a MigrateException. I did that and the migration will continue, it just creates another broken block, in this case one for aggregator. I have updated \Drupal\Tests\block\Kernel\Migrate\d6\MigrateBlockTest and added a similar test to aggregator.

Other things I thought about,
1) Preventing the import of aggregator blocks in the destination. The source data has the module that provides the block so maybe that could work. That would require more code and only prevent the creation of a broken block. Is it worth is?
2) Change the block migration to skip aggregator blocks and create a new block migration in aggregator. There is more that one way to achieve that but singling out aggregator is not the way to go. It would be saying that the block migration will only migrate blocks for modules in core.

In other words, this is implementing #2 in the proposed resolution.

Silly me, I didn't return a value from the import() method.

And again.

Yes, if you are using the UI, /upgrade, and Aggregator is not installed on the destination, then Aggregator is listed in the 'will not be upgraded' section of the Review page.

I see aggregator getting installed as part of Drupal\Tests\block\Kernel\Migrate\d6|d7\MigrateBlockContentTranslationTest

Should that get removed? Or should there be a @todo to remove it?

That's the only thing I saw that looked like a problem. I had a few other comments/questions though that may or may not be valuable.

1. I'm wondering if you have any clever ideas about how asserting the messages will scale as more modules (forum, maybe book) get moved to contrib? Will we just update the count, add assertions, and update indexes as needed?

+++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
@@ -306,6 +305,11 @@ public function testBlockMigration() {
+
+    // Check migrate messages.
+    $messages = iterator_to_array($this->getMigration('d6_block')->getIdMap()->getMessages());
+    $this->assertCount(2, $messages);
+    $this->assertSame($messages[1]->message, 'Schema errors for block.block.aggregator with the following errors: block.block.aggregator:settings.block_count missing schema, block.block.aggregator:settings.feed missing schema');
   }

2. The new test in aggregator is very similar to Drupal\Tests\block\Kernel\Migrate\d6\MigrateBlockTest obviously. Why does it not extend that test? Is it because it's unwise to extend a test that isn't marked as a TestBase? Is it because it just doesn't really matter and we just need to move this issue along?

I'm setting back to NW because I think those @todos need to be added to the MigrateBlockContentTranslationTest classes. The comment/questions don't necessarily need action. But I like to try to steal knowledge when I can. Thanks!

@danflanagan8, thanks!

I think I intended to come back to the BlockTranslation and then it completely slipped my mind. Aggregator can be removed from the translation test because it is a user block that has the translations to test.

1. From memory only a few migrations check the migrate messages. Yes, I just did a grep and found only 8 migration Kernel tests that do this so we will just deal with them one at a time, as needed.

2. It does not extend the core test because so it is independent of the core fixtures. That will avoid changes in the future, if, and when, the core fixtures change. Ideally, there would be new d6 and d7 test fixtures in aggregator that contained only the things that needed to be tested for aggregator. But making a fixture is time consuming and we already have ones that can be used, plus changing the fixtures most likely means changing the tests. And all that for no extra test coverage.

@quietone, thanks for the thought answers to my questions. I really appreciate it.

aggregator also gets enabled in the d7 version of MigrateBlockContentTranslationTest. So unfortunately I have to throw this back to NW. But then we should be good!

I am sorry for taking up your time one my silly mistakes, I am just having an 'off' day, presumably because I woke with a bit of cold.

Crossing my fingers!

1. +++ b/core/modules/aggregator/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
   @@ -0,0 +1,107 @@
   +use Drupal\Tests\migrate_drupal\Kernel\d6\MigrateDrupal6TestBase;
   

   In #3265424: Move migrate related aggregator tests to the module in preparation of removal in d10 we are adding this class to the aggregate module. Can we add a @todo to change to using that one.

2.     // Check aggregator block.
       $settings = [
         'id' => 'aggregator_feed_block',
         'label' => '',
         'provider' => 'aggregator',
         'label_display' => '0',
         'block_count' => 7,
         'feed' => '5',
       ];
       $this->assertEntity('aggregator', [], 'sidebar_second', 'bartik', -2, $settings);
   

   Can we remove this code from the test modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php? To me, it is a bit silly to keep it. Now we are only half removing it.

@daffie, Regarding 13.2, it looks to me like leaving the (modified) assertions about the aggregator block is reasonable. The assertion checks that the aggregator block, which is still very much a part of the fixture, is broken as expected after migration.

@daffie, thanks for reviewing,

13.1. Should be fixed in this patch.
13.2. That assertion is the only place in a Kernel test that the migration of the aggregator feed block is tested and it should remain.

in a rush...

Both my points have been addressed.
The change for comment #18 is going to be fixed in #3260592: Change Bartik-specific tests to Olivero (or appropriate alternative) in preparation for making Olivero the default.
For me it is RTBC.

#18, Even though I don't know much about the block migration that feels like too big a change. There are 10 migration Kernel tests with the word 'bartik'. I did try it, and after the migration there are 25 blocks instead of 14 and the aggregator feed block is in the 'content' region and not 'sidebar_second'. Based on that, I think considering the impact on Migrate when the default destination theme changes should be it's own issue.

@daffie, thanks for the link - I'll make an issue for migrate as a child of that one, and the RTBC.