d6->d8 Blocks Migration: Visibility settings issues

Hrm - we have test coverage for this in MigrateBlockTest - looking into it now...

-mike

@micnap - it looks like that if the block is disabled, then the page visibility settings aren't migrated - is this the same behavior you're seeing?

I manually tested this twice - once with a disabled block with page visibility settings and once with an enabled block (assigned to a region) with page visibility settings. I'm pretty sure we want to migrate the visibility settings regardless of if the block is disabled or not.

Also, I noticed that we aren't currently testing the role-based visibility settings (nor do they appear to be working). We define the "block_roles" table in the Drupal6Block dump file, but don't include any data for testing. I went ahead and added a some data and a (currently failing) assertion for this in MigrateBlockTest (patch attached).

This is a good issue for someone looking to get their feet wet in the Migrate in Core issue queue...

-mike

Our story so far. The test successfully fails but there is a bigger issue we have discussed online. The migrate for blocks doesn't address importing user roles at all. I have added some functionality but the tests still fail and I am still not getting any good traction. So I offer a patch and a request for guidance.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/Block.php
@@ -75,18 +75,47 @@ public function fields() {
+      if ($croles = count($roles)) {
+        $row->setSourceProperty('settings/visiblilty/user_role/id', 'user_role');
+        foreach($roles as $role) {
+          $row->setSourceProperty('settings/visibility/user_role/roles/' . $role, $role);
+        }
+      }

This patch looks like a good start. Not sure why we you're setting this so deep, can't we just set the into the "roles" key? Like so: $this->setSourceProperty('roles', $roles);

I think maybe we are going to need a dependency on d6_user_role, even if it's only soft so we can get the new machine_name of each role.

Setting to NR for the bot.

Sorry for my lack of participation. Here's a re-roll. of 2341683-block_visibility-6. I tried $this->setSourceProperty('roles', $roles); but this still returned a negative test. Diving further into this tonight.

Hmm - there's a lot going on with this issue...

Did we lose the original intention for this issue? Is the "Show block on specific pages" issue still valid? It looks like most of the discussion of this issue is around the role visibility. If so, we need to update the issue summary.

It's pretty clear that we need to add some code in order to migrate role visibility settings, and it is not so simple to do. Here's what a sample role visibility setting for a D8 block config looks like:

visibility:
  user_role:
    id: user_role
    roles:
      anonymous: anonymous
    negate: false
    context_mapping:
      user: user.current_user

So, if role visibility is used on the source, then we need to build out this structure in the destination. Some questions/thoughts:

1. Is the best place to do this in the block source plugin or maybe the BlockSettings.php plugin? Based on the next few points, I think BlockSettings.php...
2. It seems like we're going to need to access the role migration map table in order to map all roles. This is where we get into a bit of "Inception": we're migrating rows of blocks, but some blocks may have role visibility settings that require us to look at one or more rows of the D6 source "block_roles" table. Mickey's code in patch 10 is a good start, but I think we have to assume that we're going to be migrating all block_roles, just just anonymous and authenticated. Therefore, we'll need to be able to map the source id of a role to the destination id (is there a simple way to do this?) If there are any records in block_roles for a given block, then we need to build out the above structure.
3. I don't think we want to build out any of the above "visibility/user_role" structure unless the source block actually has its visibility restricted by role.
4. Minor point - the patch has the word "visibility" spelled wrong in line 109 of Block.php

Thoughts?

-mike

Would be good if someone could retest this now that #2358269: Migration bugs in block visibility, field overrides, cron, maintenance settings and form modes found by configuration schema checking has been committed.

The page related visibility settings are all working as far as I can tell.

There's an issue with the role visibility settings, but I'll open a new issue for that.

-mike