Provide an upgrade path to the new Block architecture from Drupal 7

Actually, we can start work on this as soon as we finish the ConfigEntity conversion. At most I expect we will need to re-save exported block instances with a few additional plugin IDs, I think.

Given where the code in #1871772: Convert custom blocks to content entities is going, we'll need to write to field_data_field_body|field_revision_field_body too as block.body moves to a field, plus initialize the {custom_block} and {custom_block_revision} tables
We can follow the 6-7 node body upgrade easily enough for the field stuff.

or rather field_data_body|field_revision_body

In #1875260: Make the block title required and allow it to be hidden we're looking to make the block title required. For the upgrade path, we'll need to replace titles that are an empty string or <none> with the block description ({block_custom}.info for custom block instances and I suppose whatever the D8 plugin's default title is for other blocks).

Did we keep the same block type IDs for all the core blocks when we converted them?

Related issue: #1887688: locale_update_8001() references unused block tables

How long are we planning on postponing this? AFAICS, all of the architectural issues are done now, yeah?

FWIW the custom_blocks (ie data from {block_custom}) are upgraded, this was fixed in #1871772: Convert custom blocks to content entities so that just leaves their configuration (ie data from the {block} table)

Updating issue summary

I'm with @webchick on this
Now that #1871696: Convert block instances to configuration entities to resolve architectural issues is in, this can be active imo

Yeah even if it changes again there's no reason it can't be fixed now then updated later.

I was looking into what the upgrade path for blocks would be and stumbled upon this issue. After looking into code, it looks like the basic upgrade path would be to go through each block table row, create an config instance for the block, and then remove the block table at the end. The hard part of this would be to identify any changes from block.delta to the config entity id (as well as serialized data).

@webchick, @catch, I've asked several times for this to be unpostponed, but @EclipseGc and others posit that it will be a huge duplication of effort before displays are done. (I'm not saying I agree it should remain postponed; just that is the response I've gotten when I ask about this issue every week or two.)

Here's an incomplete patch that borks the site after block upgrade test, but I might as well post my approach. There are two issues I ran into:

1. Everything requires a plugin and there is no way to find out what a future plugin is going to be. I started out trying to create Block entity instances directly, but quickly found that to be a pain. I got most of the way with just creating Config objects directly, but still ran into the plugin issue. I think it would be possible to lookup core block plugins if the second issue below is solved, but contrib is a different matter. Maybe a dummy plugin for contrib or core modules that aren't consistent with Drupal 7, and then those modules can deal with the problem in a later update..?
2. Drupal 8 broke consistency with Drupal 7's block names/ids/whatever. An upgrade will need to have a matrix lookup to compare these as there does not seem to be a consistent pattern to upgrade the plugin names/ids/whatever (e.g. Compare the following: menu block, system powered by block, user login block, and language block). Ugh.

I am also having trouble finding docs on appropriate instantiation of configuration entities such as blocks. Maybe I'm missing something obvious..?

+++ b/core/modules/block/block.installundefined
@@ -336,6 +336,80 @@ function block_update_8008() {
+      // Remove the block table.
+      db_drop_table('block');

We don't drop tables in the upgrade path - see #1860986: Drop left-over tables from 8.x

Other than that, I had a quick look how to get this forward, and yes, we need some sort of mapping for the core blocks. Same would apply to contrib, so maybe we should create a helper upgrade function where core (and contrib) can pass on a list of old id's mapping to new id's ? A bit like how we handle the variables now as well.

Ah, if the block tables still exist until 9.x, then contrib can take care of itself.

For core blocks, a block/plugin map. Then I'd just go back to creating the block entity with createInstance() instead of manually creating a Config object.

I tried switching back to entity_create(), but still ran into controller class issues so I switched back to config(). I quickly (read: sloppily) made a mapping function, which works mostly, and there are some issues still with system menu, menu, and locale blocks. Currently ignoring poll, profile, locale, language, and blog blocks.

Everything else either fits the consistent pattern or has a mapping, but needs a test (aggregator, etc...).

I modified the block upgrade test to temporarily just look at admin/structure/block and admin/structure/block/list/block_plugin_ui:seven. If anything bad happens it will break there.

Also still need to filter out contrib blocks per my last comment.

I started out making the block mapping function fairly verbose, but cut back. It may just be a good idea to make it a complete map with properties as well as plugin ids.

Needs work patch attached. No interdiff as I forgot to commit locally last time.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BlockUpgradePathTest.phpundefined
@@ -8,9 +8,10 @@
+ * Loads a standard installation of Drupal 7 and runs the upgrade process on ¶

trailing whitespace http://drupal.org/coding-standards#indenting

I had some changes that I haven't posted yet and I am not sure when I'm going to get back to this issue so I'll post a new patch up.

I redid the mapping function so that it will be able to map other block properties instead of just id. Still trying to get menu and system menu blocks to upgrade properly. I think I need to stop relying on the block upgrade test so I can actually inspect config files that are being written and compare those to what they should be.

Okay, all core blocks except locale, profile, poll, and blog have configuration entities "written" with visibility settings although it is messy code.

However I still end up with corrupt block configuration for block instances that are disabled due to label settings. It's almost as if the configuration entity writes differently when it's created manually rather than via the block plugin container (which does not seem to be possible in the upgrade path).

Any ideas from anyone else?

However I still end up with corrupt block configuration for block instances that are disabled due to label settings. It's almost as if the configuration entity writes differently when it's created manually rather than via the block plugin container (which does not seem to be possible in the upgrade path).

That's probably exactly the case. Somehow the entity plugin manager is available for Views though (I think?) Maybe it becomes available later?

There's a lot of criticals in this problem space right now:
#1949724: Allow simpletest child sites to additionally load a test-specific settings.php to allow testing anonymous and configless updates
#1848998: Problems with update_module_enable()
#1856972: config() isn't safe to use during minor updates if there's a change to configuration storage

Well one thing is for sure - we can't use entity_* functions in updates... mad stuff happens!

This patch needs to update the block manifest file with the new entities created too.

Also should the block module be upgrading blocks provided by other modules?

Also should the block module be upgrading blocks provided by other modules?

This is an interesting question. Since the data was originally in {block}, I think it should?

This is an interesting question. Since the data was originally in {block}, I think it should?

But the implementation of hook_block_info is the modules... eg. http://api.drupal.org/api/drupal/modules%21system%21system.module/functi...

Thank you for the direction, xjm and alexpott.

Okay, new proposal after thinking about this on the plane.

• Modules that implement blocks are responsible for upgrading their own blocks wrt mapping, name changes, etc...
• Add (_block_)update_block_to_config
  1. Get/Make config entity using config() for the block name.
  2. Merge in changes like in _update_variables_to_config()?
  3. Get node type and role visibility settings for block id.
  4. Set properties necessary for block entities based on property map passed in?

Issue summary should be updated to specify that it should no longer be postponed

Updated issue summary.

Updated issue summary based on the latest approach proposed in #26. This will need more updates around reviews, steps to test, API changes - but should be OK for now?

@mradcliffe are you happy to continue working on this, in which case I will stop hounding other people to do it? :)

@pameeela, I'm planning on at least working on it before Friday, and then when I am not working as a mentor on Friday in Portland.

Edit: I don't have any additional code at the moment.

Okay, I made some progress on an alternate way of doing this. I added a function to handle creating config entities in update.inc, and then each module will need to implement this. I added user and menu module as a test and to see what's going to fail.

I still need to do some more actual testing, but wanted to post a WIP patch for some review on this new approach.

Questions:

• Should the (_block_)update_block_to_config() function be in update.inc, block.module, or block.install?

Okay, here's an added test and fixing the label issue. I think this is going to work.

Need to hammer out the rest of core.

@mradcliffe looks like you are getting close. Can you or anyone else across this issue post some steps to test for when we get to green?

@pameela. I think if we want full test coverage, then the upgrade database needs to be filled with content for all blocks, and then tests each block for specific content/configuration. I think the most important assert would be if the block is not corrupt (i.e. the code created config entities incorrectly via config()).

I have added book, comment, forum, node, search, system, and shortcut block upgrade paths in this next patch. I haven't had a chance to look at the one failure yet.

Note: the interdiff has two commits in it (didn't post patch #3).

Edit

1. Node and comment had variables that were not upgraded related to blocks. I added comment variable in the block upgrade and node in a previous upgrade. Not sure which is the best approach.

Oops. That patch was terrible. this won't solve the 1 fail, but that's next.

Sure let's go for a test.

Okay, this patch will fix some bad plugin ids that I had in the previous patches in the new approach. Again, I haven't looked at the one failing test yet to do with block title length. I am not sure what changed that would cause this.

Todo:

• Debug failing test.
• Come up with more tests of blocks (if necessary - @eclipsegc?).

Also screenshots added:

I am not saving the label correctly, which should account for the test failure.

I was confused by the title and thought that it meant that blocks had not yet been upgraded in D8. Hopefully the new title still says what is intended while removing the confusion.

Ignore comment #42. The test fail comes from switching to standard_all instead of minimal. The test assumes that sidebar region will be available on admin pages, but that's not true for the standard_all database.

- Switched back to minimal database for now.
- Improved docs a bit (not complete) including fixing a typo in node.install.

This patch is mainly cleanup with a fix for the exceptions in the last test run and incrementing the node update number.

Oh, heh, block_navigation != book_navigation. I also fixed not using array union correctly.

Let's give this patch a whirl while the queue is low.

Updated issue summary

New patch:

• Added aggregator category and feed block upgrade path.

Manual testing.

1. Download drupal 8 dev
2. Apply patch
3. Import Drupal 7.22 database from zip file (It's SQL w/ devel generated content, not a test-driven dump).
4. Create a settings file to point to the database with update access turned on.
5. Upgrade
6. Go through anonymous, authenticated, and admin users (use drush 6 to generate a login link), and verify blocks. I added some screenshots to work with.

• timplunkett: Try reworking the node/comment settings instead of adding them as permament settings in node/comment.settings.yml
• mradcliffe: Upgrade/delete variables that are used in blocks in the node/comment block update, and then create follow-up issues for forum/book as that's what already exists now in D8

Removing myself from the author field so I can unfollow. --xjm

Updated issue summary.

Re-roll and remove the node/comment settings additions per #50.

Whoops. I failed. This should be better.

Remove completed task.

Re-roll, patch no longer applies and changed update numbers for node, system, and user.

#52: 1871700-block-upgrade-take-two-12.patch queued for re-testing.

Ah, that's not my fault. There's a regression in core and that default local task no longer works. If the previous patch was in core already than tests would have caught the regression. :*(

Here's a patch that uses the working menu route for the block form controller.

Filed related issue #2052019: Fix block configuration default local task to use block_admin_edit route.

That test is not failing locally for me so rebase and re-roll.

Awesome work @mradcliffe. I have found some minor doc issues.

+++ b/core/includes/update.incundefined
@@ -1680,3 +1680,99 @@ function update_language_list($flags = Language::STATE_CONFIGURABLE) {
+ * @param $block_name
...
+ * @param $module
...
+ * @param $delta
...
+ * @param $properties
...
+function update_block_to_config($block_name, $module, $delta, $properties) {

Typehint missing.

+++ b/core/modules/aggregator/aggregator.installundefined
@@ -330,3 +330,77 @@ function aggregator_update_8001() {
+  $results = db_select('block')->condition('module', 'aggregator')->fields('block')->execute()->fetchAll();

+++ b/core/modules/book/book.installundefined
@@ -79,3 +79,43 @@ function book_update_8000() {
+  $results = db_select('block')->condition('module', 'book')->fields('block')->execute()->fetchAll();

+++ b/core/modules/forum/forum.installundefined
@@ -270,3 +270,51 @@ function forum_update_8000() {
+  $results = db_select('block')->condition('module', 'forum')->fields('block')->execute()->fetchAll();

+++ b/core/modules/menu/menu.installundefined
@@ -89,3 +98,41 @@ function menu_update_8004() {
+  $results = db_select('block')->condition('module', 'menu')->fields('block')->execute()->fetchAll();

+++ b/core/modules/system/system.installundefined
@@ -2250,6 +2250,78 @@ function system_update_8059() {
+  $results = db_select('block')->condition('module', 'system')->fields('block')->execute()->fetchAll();

+++ b/core/modules/user/user.installundefined
@@ -1090,5 +1090,43 @@ function user_update_8020() {
+  $results = db_select('block')->condition('module', 'user')->fields('block')->execute()->fetchAll();

Can we make these multi lines?

Thank you for the review, jibran.

I tested this using the database dump provided and the latest codebase and I get the following error when hitting update.php. Can anyone comment on what I might have done wrong, or replicate the same error?

WD menu: Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table [error]
or view not found: 1146 Table 'd8.router' doesn't exist: SELECT name, route FROM
{router} WHERE name IN (:names_0); Array
(
    [:names_0] => block_admin_display
)
 in Drupal\Core\Database\Connection->query() (line 571 of
/Users/bendougherty/Sites/d8/core/lib/Drupal/Core/Database/Connection.php).
WD php: Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S22]: Column not  [error]
found: 1054 Unknown column 'base.uuid' in 'field list': SELECT base.uid AS uid,
base.uuid AS uuid, base.name AS name, base.langcode AS langcode, base.pass AS pass,
base.mail AS mail, base.theme AS theme, base.signature AS signature,
base.signature_format AS signature_format, base.created AS created, base.access AS
access, base.login AS login, base.status AS status, base.timezone AS timezone,
base.preferred_langcode AS preferred_langcode, base.preferred_admin_langcode AS
preferred_admin_langcode, base.init AS init
FROM
{users} base
WHERE  (base.uid IN  (:db_condition_placeholder_0)) ; Array
(
    [:db_condition_placeholder_0] => 0
)
 in Drupal\Core\Database\Connection->query() (line 571 of
/Users/bendougherty/Sites/d8/core/lib/Drupal/Core/Database/Connection.php).

Hi benjy,

Do you have a drupal 7 settings.php? You need to make that writable and place it in sites/default. I think most likely Drupal "detected" that 8 was installed and tried to bootstrap Drupal 8. You can see this if you find a Drupal 8 config_* directory or Drupal 8 tables created in the database (ugh). I found that I needed to go directly to core/update.php instead of being redirected from /update.php to core/update.php.

Unrelated to the above...

I was able to run the update with and without the patch on the database dump from a while ago, but I was left with a broken / bugged installation. Next step is to re-do the manual database dump.

• PHP Fatal error: Call to a member function buildUrl() on a non-object in core/modules/rdf.module on line 341
• ... and similar errors in image.module

Added manual testing steps to issue summary.

OK I can confirm I got this patch to work my main problem been that I was using a D8 settings file I think.

It was a little flakey though, like afterwards I was getting quite a lot of random errors around the place. I'm going to try upgrade a an out the box D7 site populated with devel content and a live site sometime this week and i'll post back with some more steps and results.

Re-adding eaten tags.

Thanks SO much for working on this, folks!!

anyone know what causes tags to sometimes get destroyed? It's a bit annoying!

I blame gremlins.

:D

The database dump provided is blocked on #2049465: Upgrade of image styles and effects broken.

OK I did some more testing and the results are below. I've also attached a new patch which increments the hook_update() number.

• Install D8
• Import attached D7 db.
• Copy a D7 settings file into your D8 installation. http://paste2.org/4Lp1gWL5
• Make sure you have $settings['update_free_access'] = TRUE; (This is from the D8 config file and differs from D7)
• Run the update

Troubleshooting

• Make sure you delete everything in sites/default/files/* if you've been using the same D8 codebase previously.
• Try going straight to core/update.php if you don't get redirected.

Issues
1. My custom blocks were no longer "Placed" in their regions.

2. After the update I ran drush uli and got this error when submitting the user/edit page with my a new password.

InvalidArgumentException: Unable to set a value with a non-numeric delta in a list. in Drupal\Core\Entity\Field\Field->setValue() (line 105 of /Users/bendougherty/Sites/d8/core/lib/Drupal/Core/Entity/Field/Field.php)

Update for the bot

DB dump attached, mentioned in #69

Tagging all critical D8 upgrade path issues as "beta blocker."

1. My custom blocks were no longer "Placed" in their regions.

AFAICT this is because there is no test coverage for the upgrade path of custom blocks from D7 to D8. Neither #1871772: Convert custom blocks to content entities nor #1919910: Refactor custom_block upgrade path to create tables using hook_schema_0() instead of in block_update_N() added that.

Is it NW or NR because of #73?

This is going to need a re-roll due to #1888702: Use configuration selection instead of derivatives for some blocks which I'm about to commit.

To clarify, that issue removes derivates for aggregator feeds and categories and instead puts the item count into the block instance configuration, where it already kinda is.

I haven't check what this issue is doing, but it simply needs to create a block instance for each (enabled, not sure if you care about others here?) aggregator block and put the feed/category id into the configuration. Then drop the block column.

Remove use of deprecated function.

Yes, this is going to need work. Also adjusted status per remaining tasks section.

1. interdiff-1.txt: Took care of the config -> \Drupal::config change
2. interdiff-2.txt: Fixed update numbers for user and node.
3. interdiff-3.txt: Made an attempt at a fix for aggregator based on recent changes.

Hopefully this gets things up to speed for any Prague core sprinters.

This issue is no longer relevant now we're brining migrate into core. I'll post back with the new issue when it's created.

There is a group for the migrate efforts here: https://groups.drupal.org/imp

I wanted to mention it for those at Badcamp tackling this issue, but the patch can still come in handy so you don't have to duplicate a lot of work.

Yeah, I don't think an issue should be closed when the code in it will still need to be added in some form or another.

Maybe at most this could be marked "postponed"?

It depends on if the Migrate work should be split into a separate issue or not. I could not find any information on an existing tag for the Imp Initiative so I went ahead and added one as "Imp Initiative".

I'd commit this patch assuming it works + passes tests so there's a reference point to work from in core, however I don't want to encourage anyone to work on it when it's likely that a good chunk of the code will have to be done again for migrate. Assuming that doesn't happen 'postponed' feels like the right status - a lot of the code can likely be re-used or at worst shows what data needs to go to/from where.

Updated issue summary per latest comments.

No longer blocks release. But we need a way to track issues that block releasing a migration path at some point.

...parent/related issues now have their own place as issue metadata. Also hiding past patches and interdiffs.

We have an IMP issue for #2120629: Write a source plugin + tests for 6.x Blocks. Metadata also needs migrating (enabled/disabled, region, etc.). That one is now #2149179: Write a source plugin + tests for 6.x Block Metadata (status, region, visibility, ...) and has Related Issue and Summary links back to this one.

Marking this as a duplicate.