D6 Block Mapping

Seems like a reasonable approach. The only other thing I'd recommend is a step between 2&3 where you attempt to match with the default region names that are defined in https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Extension!ThemeHa.... If you take the top ~10 base themes in D6 and see what they called those things, one additional step could map e.g. "left", "sidebar_left", "left_sidebar", etc. to "sidebar_first." Not sure if that's possible or not, but it'd be nice, since barfing things into the content region's definitely not going to be what people want (though certainly better than not showing up at all :D).

Thanks, this is looking quite great!

\Drupal::config('system.theme')

This should be injected. See for example core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/processor/DefaultProcessor.php on how to write an injecting plugin; that one uses $container->get('config.factory'), you could directly go for $container->get('config.factory')->get('system.theme') and inject the config object itself.

+ if (($this->isSourceCommonTheme($theme)) && ($theme == $d6_default_theme) && ($this->isDestinationCoreTheme($d8_default_theme))) {

I don't quite understand this logic. I thought we will always map default to default and admin to admin.

+ $this->adminTheme = $this->variableGet('admin_theme', $this->defaultTheme)

Shouldn't we just set the default to NULL and skip the second half of the mapping I just mentioned if the admin theme is empty?

Drupal 8 does not have a status but it doesn't matter; this is here the

Should be: this is here to. Yes, I wrote it. Wrong nonetheless.

+ private function

Never use private unless you have some really, really good reason to.

Please rename getRegionMap to getCoreRegionMap in anticipation of the followup which will be themeless region mapping.

This is way better than a proof of concept, really! I think it just needs to flesh out the block test and it's done.

> Is the way I set up the array in getCoreRegionMap() the best way to set up the region map?

Yes.

Now, theme logic. Your theme logic wants to map only if the region map will have a chance to match. But that's not relevant for theme mapping IMO. Even if the core migration does not have the necessary region mapping for the case when the user changes their theme from D6 to D8 it can easily be altered in. I think we should honor what the user wants and map default to default and admin to admin:

+  public function transform($value, MigrateExecutable $migrate_executable, Row $row, $destination_property) {
+    list($theme, $d6_default_theme, $d6_admin_theme) = $value;
+
+    $d8_default_theme = $this->config->get('default');
+    $d8_admin_theme = $this->config->get('admin');
+
+    if ($theme == $d6_default_theme) {
+      return $d8_default_theme;
+    }
+
+    if ($theme == $d6_admin_theme) {
+      return $d8_admin_theme;
+    }
+
+    return $theme;
+  }

Exactly! Because the region map will always return a valid region -- it's just going to fall back to content a lot of times in the uncovered cases.

1. return $destination_region = FALSE; is very weird while works just return FALSE.
2. mapping to content is always pointless. No mapping results in content too.
3. Looking at these results, if we were changing left and right to sidebar_first and sidebar_second respectively then check whether the results are in the region list of the target theme then we would kill a lot of birds with a single stone: core, contrib, all. Yes this is not what's in the issue summary but look at your mappings. There's some presumption here that I thought dangerous before but I'll boldly go where no man has gone before and claim that left, right, header and footer are such strings where merely the string is enough to derive a semantic meaning from without knowing the theme. https://api.drupal.org/api/drupal/core%21modules%21system%21system.modul... is your friend.

Yes, that's exactly what I had in mind, just map those names. When I wrote the IS , I was thinking too many contrib region names won't have a good match but I didn't realize core was so uniform and few regions.

This is becoming really great.

Sorry for being so slow in realizing things but as I see the code forming so do my thoughts answer to it. The default_region_map really ought to be a region_map in the YAML so that contrib can easily change it, don't you think so?

I still think the theme logic in #10 would be enough. if ((strtolower($theme) == strtolower($d8_default_theme)) || (strtolower($theme) == strtolower($d8_admin_theme))) this is the difference and it only makes a difference if there is a non-default theme in D6 which happens to have the same name as the d8 default. That doesn't seem too realistic to me? But we could simply check whether the theme from d6 is neither default nor admin and exists in d8 and if yes leave alone:

$themes = \Drupal::service('theme_handler')->listInfo(); // inject this.
if (isset($themes[$theme])) {
  return $theme;
}
// If the source block is assigned to a region in the source default theme, then assign it to the destination default theme.

etc.

I do not think you could easily get this into a multi step process with one step being the custom plugin and another being a static map because they are not pipeline-ish -- their sources are different and they are independent. I am not sure why is nestedvalue used, however. That's what static map is using, yes, but we have a very small source=>destination style map here, nothing on the level of staticmap. if (isset($map[$region])) return $map[$region] should suffice, i'd guess?

I will get back to the testing but

1. the block region mapping is seriously bad ass
2. the theme mapping already has the scaffolding in place for injection, please inject $container->get('theme_handler')->listInfo(). Makes (theoretical) unit testing quite easy cos that's just an array, isn't it.
3. On that note the two config gets call can be moved down after the theme exists check. Microoptimization, yes, but why not when it costs nothing?

Is the patch meant to be applied in the core root directory? I'm getting errors when I try, due to some non-existent file paths, like:
core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/Dump/Drupal6SystemTheme.php

Thanks @ultimike: I did a simple migration with the latest patch applied and this worked as advertised.

This is looking good ultimike, really great work.

Just a few super minor doc issues and then I think this is RTBC.

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/BlockRegion.php
   @@ -0,0 +1,44 @@
   +    // Theme is the same on both source and destination, we will assume they have the same regions.
   

   comments should be less than 80 characters.

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/BlockRegion.php
   @@ -0,0 +1,44 @@
   +    // If the source and destination theme are different, try to use the mappings defined in the configuration.
   

   comments should be less than 80 characters.

3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/BlockTheme.php
   @@ -0,0 +1,95 @@
   +   * @param \Drupal\Core\Config\Config $config
   +   *   The configuration factory object.
   +   */
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, Config $config, array $themes) {
   

   Missing @param comment for $themes.

> comments should be less than 80 characters.

on one line, you just need to break them up.

I just rerolled the patch against the latest HEAD and fixed comments in #32 and a couple others I found. I hope to see this committed soon.

Hmmm... It does not seem to want to test the patch. Maybe I should flip the status once?

Setting back to needs review to test the patch in #34.

UPDATE: Okay, it doesn't look it is working. Any ideas?

UPDATE 2: Okay, it finally worked (and the tests pass). It might be a delay.

Unfortunately the patch in #34 failed my manual testing just now.

I've attached a detailed manual test script to cover as many of the situations in this issue's description as possible. Please let me know if there are any situations it doesn't cover. It tries to make e.g. the manual equivalent of "fixtures" as explicit as possible. However, in the absence of a similar-named theme in D6 and D8, it's hard to test everything. Also it would be good to get some guidance on what happens to any e.g. D6 "Bluemarine" configuration: should that be silently dropped?

With that in mind, here's a report of the results of my testing.

I patched D8 with the patch from #34 and then ran the migration using the manifest inline in the test script, with d6_block_region and d6_block_theme explicit. However, when I did so, I didn't get any mention of those two migration plugins:

$ drush migrate-manifest --db-url=mysql://d6mig:d6mig@localhost/drupal6_migration /var/www/drupal8-migration-manifests/D6Manifest-Block-Theme.yml
Running d6_filter_format            [ok]
Running d6_custom_block             [ok]
Running d6_menu                     [ok]
Running d6_block                    [ok]
$ 

Here's what I saw in the D8 admin interface. I cleared caches to make sure I wasn't missing anything.

Test 1

Custom block C1:

• PARTIAL: D6 admin description became D8 block "title" (/admin/structure/block) or "description" (/block/1) - these are the same in D8?
• FAILURE: D6 block title itself not migrated
• SUCCESS: D6 body & text format migrated
• FAILURE: Visibility:pages not migrated
• FAILURE: Visibility:roles not migrated
• FAILURE: Garland "header" region not migrated

I'm not sure what metadata is currently supported by d6_block, but obviously the last item is the kicker for this particular issue.

Custom block C2:

As C1 above; the region "left"=>"sidebar_left" was not migrated either.

Tests 2-4

Can't test (see comments in test script re what should happen to Bluemarine, and how to test "theme with the same name".)

Here's an option for testing migration of a theme with the same name: a stub "Marvin" theme in D8, with just a "content" region (to also be able to test what happens when "left"/"sidebar_left" doesn't exist in the D8 equivalent theme.)

I've attached an info.yml file for comment, to be saved to sites/default/themes/marvin/marvin.info.yml.

Sorry, I think I was misunderstanding "everything else gets copied" up in the original issue description as meaning "all the block metadata" (visibility etc.): is it worth me opening separate issue(s) for that, to look at the issue(s) independently?

Unfortunately the main thrust of this issue - the theme/region copying - didn't work. This was buried a bit in my discussion of the block metadata but here are the salient bits of my comment #37 above:

C1: FAILURE: Garland "header" region not migrated
C2: the region "left"=>"sidebar_left" was not migrated (either)

This was trying to copy from Garland (D6 default) from Bartik (D8 default.) I didn't get much further than that as I had enough negative results that I wanted to make sure I was on the right track!

... Tell you what, for clarity's sake, I'll run through my tests again - including "D8 Marvin", but not including any discussion of block metadata - and report back when I've done so.

I've redrafted the testing script (and edited marvin.info.yml to use the right) and run it locally, and I can confirm a partial success.

I've a couple of queries which I think are worth clarifying:

* The manifest in the testing script currently states d6_block_theme and d6_block_region explicitly: is that necessary?
* When a theme doesn't exist in D8 (e.g. Bluemarine in the testing script) what's the result? Should the configuration be silently ignored? Is there any way of checking that at the database/config layer? This edge case is the final test in the testing script.

Anyway, here are my results in full.

Drush output

Here's the output from running my Drush command (whitespace edited slightly to stop it looking crazy):

Running d6_menu                                                             [ok]
Running d6_filter_format                                                    [ok]
Running d6_custom_block                                                     [ok]
Running d6_block                                                            [ok]
The block block_4 was assigned to the invalid region right and has been disabled.
                           [warning]
The block block_5 was assigned to the invalid region left and has been disabled.
                            [warning]

As you can see, it's treated both "right" and "left" as invalid regions (probably for Marvin; see below.)

Test 1: Migration Garland=>Bartik (because default theme)

SUCCESS: C1 migrated Garland header=>Bartik header
SUCCESS: C2 migrated Garland left=>Bartik sidebar_first

Test 2: Migration Marvin=>D8 Marvin

FAILURE: C1 migrated Marvin right=>D8 Marvin disabled (should be migrated into content as per issue description: 'blocks will be dumped in the "content" region in D8.'
FAILURE: C2 migrated Marvin left=>D8 Marvin disabled (should be in sidebar_first as per mapping)

Test 3: Migration Bluemarine=>No candidate in D8

I'm assuming this configuration should be dropped, but maybe we need a way of knowing whether or not it's been unintentionally saved in D8 against a nonexistent "bluemarine" theme.

@ultimike: the Marvin behaviour makes sense, but maybe the description of this issue doesn't make this behaviour clear? Perhaps when we get round to writing a change report (are those happening for IMP?)then we can make this all absolutely explicit.

On that note I'm happy for at least one of the test results to be a negative e.g. actually test that not-same-region-if-same-theme-name gets dropped. It's part of the complex behaviour anyway. So maybe let's have a marvin.info.yml with one region name the same, and one different (but a valid D8 mapped region, so we make explicit what's happening - or not - with those.)

If we're testing behaviour for non-core themes, is it a valid test, to use the D8 "with-core" test themes? Don't they still count as "basically core"? Would it be better to just create a D6 noncore.info and a D8 noncore.info.yml and add a fourth test to the script? (Do we indeed expect non-core themes to behave differently from e.g. Marvin in our tests? Just checking!)

And I've thought of a potential (negative) test to ensure Bluemarine is being "forgotten about": grep the config directories for Marvin, and then for Bluemarine? It would ensure good housekeeping (that config isn't accidentally being saved, just in an "invisible" way because the theme doesn't exist.)

(@ultimike PS I won't do any more testing until I hear your thoughts re using an in-core test theme rather than a mocked up noncore.info. But then I'll do so, and tidy up marvin.info.yml and the test script and hopefully have something final!)

@ultimike,

Your three points sound fine, but I might rephrase them as:

1. They're migrating from a contrib or custom theme (set to default) into a core D8 theme (set to default) (otherwise nothing will migrate if the theme names are different)
2. They're migrating from a contrib or custom theme into a the same named-theme (and we assume they have the same regions).
3. They're migrating from a core D6 theme into a core D8 theme (both set to default, as core D6 and D8 themes have no namespace overlap now Garland's gone)

This is mostly what I've been testing anyway, but I'm happy to formalize it like this. With my caveats added, a valid test of our point 2. entails a D8 theme with the same name as a D6 theme. Otherwise nothing will migrate, unless they're both set to default (in which case, we're just testing point 1 again.) If we're able to add such a test candidate then let's definitely do that with e.g. Marvin in core/modules/system/tests/themes/marvin.

To elaborate on that final point, one of the key problems with D5->D6 and D6->D7 upgrades has been cruft: stuff that isn't understood by the next major Drupal version but, given the database remains in situ, sits around and can sometimes cause later work to trip over itself (phantom database tables etc.)

However, this does seem to be the case with the D6->D8 migration: bluemarine config ends up in the database:

mysql> SELECT uuid FROM block_content;
+--------------------------------------+
| uuid                                 |
+--------------------------------------+
| 4674f1dd-4154-4f6f-b000-b03aca226642 |
| 759e8b3e-9aa6-4397-9b00-e613e3b27767 |
+--------------------------------------+

mysql> SELECT collection, name FROM config WHERE data LIKE '%4674f1dd-4154-4f6f-b000-b03aca226642%' AND data LIKE '%bluemarine%';
+------------+-------------------+
| collection | name              |
+------------+-------------------+
|            | block.block.block |
+------------+-------------------+
1 row in set (0.01 sec)

I don't know whether this is an issue or not. I imagine most people would expect bluemarine config to not be migrated if there's no D8 bluemarine theme, though. Would this config get cleared out later, or persist?

@ultimike I wouldn't want to propose Migration-in-core policy off the top of my head, but what I would say is that my experience of D6->D7 migrations using migrate_d2d is that it's very restricted in what it will migrate: obviously, you only ever migrate the nodes that you declare in your Migration::query() run on the D6 database. And if a field didn't exist on the D7 node, then your code to migrate it would be ignored: it's not as though the field would get implicitly created. But at times it was even more brutal than that: if you don't enable modules like Date Migration, then as far as I could tell certain Date API fields or functionality would get quietly left behind in D6.

I suppose it comes down to user expectations. Given how Migrate in D7 behaves, and what people will be used to or expect, then what do we do with configuration that has no valid home in D8, at the point of migration? I'm happy to be guided by voices more knowledgeable than me, though, especially as this is an odd edge case, where the block exists, and the concept of a block<->themeregion relationship can be said to exist, even if the theme (bluemarine) does not.

(I'd also add my experience as someone taking over a D5 site that we upgraded - not migrated - to D6. Every once in a while we'd turn a module on and the site would go bananas, because of cruft arising from functionality that other developers never fully uninstalled from the original D5 site. A D5->D6 or D6->D7 upgrade could arguably be said to be in that sense too inclusive, as it leaves any unrecognized tables in the database.)

Cruft is better than leaving data behind. That's my two cents. You might want to slice this policy off into a meta issue and write a gdo/core post about it and get community buy in.

I think the way the D7 migrate module worked by quietly leaving stuff behind was probably a downfall which was much harder to overcome for D7 migrate.

We now have the ability to migrate everything and I think bringing everything across, even if that means a few extras is probably the right behaviour. It's not doing any harm and it would be a nice surprise to enable your theme and have all the block placements "just work".

Well, I'm happy to go for the consensus opinion within the bounds of this specific issue here - and as chx says, if anyone wants to fight it out, there's always the possibility of opening another issue for it.

@ultimike, regarding your three items: here's what I think we require, to test them validly:

1. Non-core D6 theme (could just be a .info file.) set to default; core D8 theme set to default.
2. Non-core D6 theme and non-core D8 theme, of the same name, not set to default.
2. Garland (D6) and Bartik (D8), both set to default (would need to be run on a clean build, to avoid results confusion with 1.)

Does that sound right?

The D7 migrate_d2d's primary use case was to perform Drupal-to-Drupal migrations where the site architecture was being changed along the way - say, reducing 128 content types to a mere two dozen (true - the first project leading to migrate_d2d), it was never intended to be a substitute for the upgrade process and thus never attempted to migrate configuration/metadata. The D8 core migrate_drupal module, on the other hand, is meant to be the primary upgrade mechanism for upgrades to Drupal - this use case, of course, does require preserving everything on the original site.

@ultimike, I've run out of time today (although I've got dev sites set up on my work laptop now.) I'm travelling tomorrow but I hope I can look at this on Friday when I'm co-working remotely. I can provide the files but I might need advice on exactly where to roll them into the patch for #43.

One thing I did just notice - doing `drush si` on a patched codebase, I got the error:

PHP Fatal error:  Call to a member function getDirectoryPath() on a non-object in /var/www/personal/drupal8-migration/core/modules/image/src/Routing/ImageStyleRoutes.php on line 29

I had to switch back to the 8.x branch to install, and then patch afterwards. I'll try to repeat this Friday but if you're able to check in the mean time that'd help.

(Re bug above: nvm, I think this was owing to doing `drush si` more than once; not sure if all the config folders or tables get cleared out properly, so I'm just DROPping the db each time now.)

@ultimike,

I did "drush si testing" but in the admin interface I couldn't see any of the themes in core/modules/system/tests/themes. So while they can be used for automated testing, I'm not sure they'll help with manual testing.

Here's a summary of the files needed, broken down for each manual test:

1. D6 theme info file (attached to this comment); D8 uses a core theme.
2. Can we just do this with Marvin? In which case, D6 just uses core marvin and D8 uses marvin.info.yml: neither of them set to default. Otherwise we'll need a new D8 info.yml file to go with the D6 theme info file.
3. No new files required.

If we're able to use Marvin for test 2 (I'm still trying to think what using non-core themes will gain us) then that's all we need: a new D6 .info and the marvin.info.yml.

(Note that each test now need to each be run on fresh D6 and D8 installs: the block config in D6 theme X is likely to get conflated with the block config in D6 theme Y. So the testing script I used above isn't really valid any more.)

@jp.stacey, I can't download the file you attached. I think it must be d.org blocking .info files. Maybe give it a .txt extension.

I don't see why we have to use a non-core theme for testing.

Note that each test now need to each be run on fresh D6 and D8 installs

I don't really understand this?

I've worked with @ultimike tonight on a few bits, hence all the re-rolls. I've done a final review and I think this patch is a good clean-up. It's also going to make testing a bit more realistic as you're likely to end up with a site that resembles your D6 website.

@jp.stacey, I think you were trying to make a few other points that probably shouldn't hold this issue up. Do you want to create a follow-up issue with any further concerns?

Should we document somewhere that if your blocks all end up disabled, enabling a theme with the same name as your D6 theme before or after the migration will pick-up the block configuration?

Maybe we could add it as a new child page here? https://www.drupal.org/node/2228547

Just a quick one as I'm in transit again, so sorry if the following is in any way brusque. The only reason I've held this up is that when I came to it was in "Needs Review". I wanted to review it, but it wasn't clear from either the issue description, or the comments following:

(a) what the work was actually meant to do (this has been clarified a bit as we've gone on) or
(b) what steps were required in order to review it and say "yes, I agree, this works as described"

So I've been trying to clear up both (a) - which is certainly clearer, but you'd have to read the whole comment thread to be sure, and (b), where I've tried to put together a manual test script - a set of "steps to review" that might enable RTBC, but I don't think anyone else is following it. As an example of clarity, see e.g. #2289223: Manual Testing: D6 Menu (which definitely isn't my work as I copied the structure from another ticket.)

My feeling is that from a PM background it still isn't particularly clear how "the community" - as opposed to the developers who've been closely involved in it and already know how it works - is meant to review this issue, nor whether such a review should only ever rely on running some automated test (which again there are no instructions on how to run) or whether it should follow a manual test script.

But given my slow rate of response at the moment I don't particularly want to be the only blocker on this issue, given others seem convinced it works as designed.

(Edited since posting, hopefully for my *own* clarity!)

The change looks good - agree with @jp.stacey that documentation here is important since people might expect migrate to try to enable and set up the default and admin theme - however I think it perfectly reasonable to expect some sort of minimal configuration before migrate runs - and having the default theme and admin theme set and migrating to those choices seems sensible.

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/BlockTheme.php
   @@ -0,0 +1,107 @@
   +      return $d8_default_theme;
   ...
   +      return $d8_admin_theme;
   

   return $this->themeConfig->get(''); then no $d8_* variables which are easy to confuse with the $d6_* variables.

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/BlockTheme.php
   @@ -0,0 +1,107 @@
   +    // Oh well, we tried.
   

   I think this comment could be improved.

Re-rolled with the changes from #68.

It was agreed that modules will need to be enabled before you can migrate. It's not unreasonable to say 'everything contrib/custom' needs to be enabled cos what if a theme wants to add / change a migration (like this one)?

git ac https://www.drupal.org/files/issues/2248407-69.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 23550  100 23550    0     0  15915      0  0:00:01  0:00:01 --:--:-- 22154
error: patch failed: core/modules/migrate_drupal/src/Tests/d6/MigrateSystemThemeTest.php:1
error: core/modules/migrate_drupal/src/Tests/d6/MigrateSystemThemeTest.php: patch does not apply

Rerolling... Conflicts from #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit).

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/BlockTheme.php
   @@ -0,0 +1,103 @@
   + * @file
   + * Contains
   

   Missing class comment here.

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/BlockRegion.php
   @@ -0,0 +1,46 @@
   + * @file
   + * Contains
   

   And here.

I hope this gets queued for testing as it's already Needs review. I have attached the interdiff too. I took example from other files in the directory and used the completely qualified class name in the file comments.

Looks good to me.

Committed 5032755 and pushed to 8.x. Thanks!

Updated the IS it was woefully outdated. Edit: alexpott committed it.Oh well we need it for archive purposes.