Properly check for block content type in BlockPluginId process plugin

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Ran the test locally to make sure it failed and did get

Drupal\Component\Plugin\Exception\PluginNotFoundException : The "block_content" entity type does not exist.

Not entirely sure about adding a new file but could be wrong

Lets see what the committers say.

Looking at the process plugin, it does have a dependency on d7_custom_block. Therefore, all that needs to be done here is add that as a required dependency on d7_block. Refer to d6_block.yml for an example.

Shouldn't it be possible to run d7_block without d7_custom_block (and thus implicitly, without the Block Content module)? The constructor for the BlockPluginId tries to consider this fact, but I believe it was a simple programming error that needed to pass FALSE as a second argument for the ternary, otherwise this ternary would serve no purpose.

Could you provide a quick test that shows that?

Yep! The test is included in the MR.

On D10 uninstalled block_content
Ran ddev drush mim --tag "Drupal 7" d7_block
got the error in the IS
Applied the patch
And am able to run successfully.

Also curious of the block_content module is needed as these blocks coming over after custom right?

Will mark it if no one has any objection

I'm not sure this is the right fix

If you look at Drupal 7's codebase, the block_custom entries were in a database table defined by the block module - https://git.drupalcode.org/project/drupal/-/blob/7.x/modules/block/block...

In Drupal 8+ we split it into two modules (I worked on the patch 😀)

I think this implies that if you had the block module in D7, you need both the block and block_content modules in D9.

Is there a way we can install the module if its not enabled? That feels to me like the right approach.

This is what @quietone one said in #7 too

I looked into this today.

I do not know what the intention was in the ternary in BlockPluginID. In this issue that committed that process plugin, the d6_block migration was also added and that did not have a dependency on d6_custom_block. A required dependency on d6_custom_block was added later #2250429: d6_block migration not migrating all blocks → . The Drupal 7 block migration has an optional dependency on d7_custom_block. Since it is optional (and yes I tested) then d7_block should run without block_content.

That means that the change here is the right thing to do. It allows d7_block to run when block_content is not installed.

#13. I don't recall a case where we install a module to run a migration.

I've made a patch and fail path.

The last submitted patch, 15: 3331059-15-fail.patch, failed testing. View results →

Retested this.

On my Drupal10.1 site I uninstalled block_content
Connecting to my Drupal7 site and running the migrate:import for d7_block I do get the error reported
Applying patch #15 resolved the issue.

All questions have been answered. I have updated the title and added details to the issue summary.

The last submitted patch, 15: 3331059-15.patch, failed testing. View results →

Random ckeditor

I'm triaging RTBC issues → . I re-read the IS and the comments. I didn't find any unanswered questions or other work to do.

Leaving at RTBC.

The last submitted patch, 15: 3331059-15.patch, failed testing. View results →

Seems random. Restarted

Mostly nitpicks, but also a request for a followup issue.

1. +++ b/core/modules/block/src/Plugin/migrate/process/BlockPluginId.php
   @@ -41,12 +41,12 @@ class BlockPluginId extends ProcessPluginBase implements ContainerFactoryPluginI
   -  public function __construct(array $configuration, $plugin_id, array $plugin_definition, EntityStorageInterface $storage, MigrateLookupInterface $migrate_lookup) {
   +  public function __construct(array $configuration, $plugin_id, array $plugin_definition, EntityStorageInterface $storage = NULL, MigrateLookupInterface $migrate_lookup) {
   

   Technically, the @storage typehint is now incorrect. It should be ?EntityStorageInterface.

   However, that causes a PHP deprecation I wasn't aware of:
   https://3v4l.org/XAAhc

   If we keep the incorrect typehint, there's no error:
   https://3v4l.org/ANmin

   However, I think we at least need a followup issue to reorder the parameters so that we can eventually both have correct typehinting and order the arguments as PHP now wants us to.

2. +++ b/core/modules/block/tests/src/Kernel/Migrate/d7/MigrateBlockNoBlockContentTest.php
   @@ -0,0 +1,161 @@
   + * Tests migration of blocks without block content installed.
   

   This is missing the word "the". Also, Block Content should be title-cased.

3. +++ b/core/modules/block/tests/src/Kernel/Migrate/d7/MigrateBlockNoBlockContentTest.php
   @@ -0,0 +1,161 @@
   +  public function testBlockMigration() {
   

   Missing return typehint.

4. +++ b/core/modules/block/tests/src/Kernel/Migrate/d7/MigrateBlockNoBlockContentTest.php
   @@ -0,0 +1,161 @@
   +    $this->assertEntity('bartik_system_main', 'system_main_block', [], '', 'content', 'olivero', 0, '', '0');
   +    $this->assertEntity('bartik_search_form', 'search_form_block', [], '', 'content', 'olivero', -1, '', '0');
   +    $this->assertEntity('bartik_user_login', 'user_login_block', [], '', 'content', 'olivero', 0, 'User login title', 'visible');
   +    $this->assertEntity('bartik_system_powered_by', 'system_powered_by_block', [], '', 'footer_bottom', 'olivero', 10, '', '0');
   +    $this->assertEntity('seven_system_main', 'system_main_block', [], '', 'content', 'claro', 0, '', '0');
   +    $this->assertEntity('seven_user_login', 'user_login_block', [], '', 'content', 'claro', 10, 'User login title', 'visible');
   

   This kind of looks like it wants to be a data provide rarther than a helper assertion method. Or is it important that all these things are true in the same test setup?

#24
1. working on it
2. Done
3. Done
4. This is not something for a dataprovider. Like most migration kernel tests it is testing 'something' or 'somethings' after the selected set of migrations run using the source test fixture. We definitely want to confirm that the 'set' of things we are migrating are done so correctly, no just an individual item.

Follow up is 📌 Correct type hint and order of parameters in BlockPluginId Active . Removing tag.

Points have been addressed.

Actually didn't know we need :void for test methods but good to know going forward.

Thanks @quietone! I agree with your explanation on point 4.

For point 1, when I was discussing the new PHP 8.1 deprecation related to it, @alexpott pointed out that we had past coding standards cleanups for this: #2901790: Fix 'PEAR.Functions.ValidDefaultValue' coding standard →

If that's so, and it looks like it was, how is this patch passing Custom Commands? Did the rule not get enabled?

If it's okay I'm going to tag this for FM review since it's sort of a code smell -- we cleaned all the examples of this sort of thing up for a reason. Plus alexpott is kind of the leading expert on the history of our coding standards cleanups (although I am sure @quietone or @smustgrave could also do the research to provide more info as well; go for it if so inclined).

Cheers,
Jess

xjm → credited alexpott → .

Also, if patches are canonical on this issue, can we close the old MR?

Adding credit for @alexpott for background info in Slack.

+++ b/core/modules/block/src/Plugin/migrate/process/BlockPluginId.php
@@ -41,12 +41,12 @@ class BlockPluginId extends ProcessPluginBase implements ContainerFactoryPluginI
-  public function __construct(array $configuration, $plugin_id, array $plugin_definition, EntityStorageInterface $storage, MigrateLookupInterface $migrate_lookup) {
+  public function __construct(array $configuration, $plugin_id, array $plugin_definition, EntityStorageInterface $storage = NULL, MigrateLookupInterface $migrate_lookup) {

This should be
public function __construct(array $configuration, $plugin_id, array $plugin_definition, ?EntityStorageInterface $storage, MigrateLookupInterface $migrate_lookup) {
You have to intentionally pass a NULL in and we should not be providing a default value because $migrate_lookup is not optional.

#31 will make 📌 Correct type hint and order of parameters in BlockPluginId Active obsolete.

We should also improve the documentation for the param.. something like

   * @param \Drupal\Core\Entity\EntityStorageInterface|null $storage
   *   The block content storage object. NULL if the block_content module is not installed.

Also

+++ b/core/modules/block/src/Plugin/migrate/process/BlockPluginId.php
@@ -61,7 +61,7 @@ public static function create(ContainerInterface $container, array $configuratio
-      $entity_type_manager->getDefinition('block_content') ? $entity_type_manager->getStorage('block_content') : NULL,
+      $entity_type_manager->getDefinition('block_content', FALSE) ? $entity_type_manager->getStorage('block_content') : NULL,

Let's use useDefinition() here - it make more semantic sense. Ie:

$entity_type_manager->hasDefinition('block_content') ? $entity_type_manager->getStorage('block_content') : NULL,

alexpott → closed merge request !3197

@alexpott, thank you.

Appears feedback for NULL has been addressed.

Committed and pushed 49ecf1d7e85 to 11.x and d670582a0cd to 10.2.x and bfa78acfe04 to 10.1.x. Thanks!

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