Follow-up from #2118245: [meta] Source classes for config entities.

Write a D6 source plugin + tests for the block table.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mpgeek’s picture

Assigned: Unassigned » mpgeek
mpgeek’s picture

Assigned: mpgeek » Unassigned
Status: Active » Needs review

I'm wondering if we can't just leave the "block" table behind for this. The table is really about placement configuration that is theme dependent (theme, region, visibility, pages, etc). I would contend that any 6-8 migration will involve a new theme, so each record in the "block" table doesn't contain much value outside of knowing the legacy configuration.

For blocks generated by code--say Views blocks for example--would be upgraded/migrated by virtue of the code upgrade. All that's left for those is placement. I could imagine a scenario where a migration developer writes a custom update hook (or other script) to place those blocks into the migrated site's theme, but that seems like it's outside the scope of what we are trying to accomplish here.

For "custom" content blocks, the content itself is stored in the "boxes" table, which probably has value... it's custom content that should be migrated. But you are back to figuring out how to place the blocks in the new theme.

So in both cases, *placement* is likely cruft to a migration, but migrating custom content is not. If we agree, i would think we can close this task and focus on the boxes task: #2120643: Write a source plugin + tests for 6.x Boxes.

mradcliffe’s picture

@mpgeek: I think the assumption that the block table doesn't matter is too narrow. Some users are going to want the same look and feel between Drupal versions, or as close as possible.

I have a working upgrade path for blocks in #1871700: Provide an upgrade path to the new Block architecture from Drupal 7. I propose for extending or writing a block plugin destination class that can set the necessary properties. The trickiest bit in the issue when I worked on it was looking for all of the changes to delta -> id and name from D7 to D8. After that it was not difficult to save the configuration entities in their proper regions.

chx’s picture

Status: Needs review » Active

There's no patch here for review.

As for delta -> id we faced the same problem with filters and the decision was made to write a two-column to single column process plugin (or mess with ProcessMap so it can do it) but more importantly: The mapping will be in the configuration of said plugin which is stored in the migration entity and then contrib modules can use hook_migration_load to change the migration entity. For filter formats, the system already has a way to deal with missing filter plugins -- it will happily return filter_null for missing ones. For blocks, we need to look at the problem ourselves, but this is hardly the responsibility of the source.

In short: we still need this written.

mradcliffe’s picture

Yes, agreed. That was all done by each module's implementation of blocks in the patch.

eliza411’s picture

Assigned: Unassigned » mpgeek
Issue summary: View changes
Issue tags: +Needs tests

mpgeek says he'll take this one :)

mpgeek’s picture

Here is a patch that rectifies a @TODO in Block.php and implements a test. Right now this test fails with

Drupal\migrate_drupal\Tests\BlockSourceTest::testRetrieval reset() expects parameter 1 to be array, null given

I belive this stems from the foreach ($iter as $data_row) call on line 85 in MigrateTestCase.php, which to me smells like a bug in Block.php itself. Providing the patch here in case a reviewer has insight while i debug further.

mpgeek’s picture

Status: Active » Needs review
benjy’s picture

+++ b/core/modules/migrate_drupal/tests/Drupal/migrate_drupal/Tests/source/d6/BlockSourceTest.php
@@ -0,0 +1,115 @@
+    $this->databaseContents['blocks'] = $this->expectedResults;

You also need to setup the block_roles table which the source uses.

mpgeek’s picture

Thanks, benjy. Added the blocks_roles table and now the tests are passing. Are we doing interdiff's on these issues? If so, that is attached as well as the working patch.

eliza411’s picture

Assigned: mpgeek » Unassigned
chx’s picture

Status: Needs review » Fixed

Committed, thanks!

Status: Fixed » Closed (fixed)

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