Upgrade path for blog module removal

Todo list looks complete to me, can't see any other glitches or tasks.

Was the plan to direct the user to the contrib version of the blog module?

https://drupal.org/project/blog

How will new individual users in a multiuser blog site be able to create individual blogs in a site upgraded from 6 to 8 - the contributed blog module will have "blog" content type as well as there will be existing "blog" content type? The users who were using individual themes in a multiuser blog site will be able to do so in the upgraded site?

@yeti22: I fear that all those things will be harder to create in D8, unless you follow this issue closely and help us out.

I just started working on this issue. I seem to have succeeded in handling the {node_type} table update. Now after running update.php, the node type is associated with the node module in the {node_type} table.

However, I am now struggling with the permission issue. After running update.php following update of the code from D7 to D8, all the blog node type related permissions are gone.

I'm guessing the following:
- node_permission() returns all the node-related permissions such as 'create foo content' and 'edit own foo content'.
- The strings such as 'create foo content' and 'edit own foo content' are generated by node_list_permissions().
- The list of node types which are passed to node_list_permissions() are obtained by node_permissions_get_configured_types().
- node_permissions_get_configured_types() calls node_type_get_types() to obtain the list of node types.
- node_type_get_types() calls _node_types_build() to find available node types. In fact, I guess _node_types_rebuild() is called here.
- _node_types_build() checks what modules calls hook_node_info().
- Because the node module doesn't define the blog node type, all the blog node type-related permissions are ignored, although these permissions in fact remain in the {role_permission} table.

How to address this issue? Any idea?

Here is my patch. I think it solved the two original problems.

+++ b/modules/system/system.installundefined
@@ -1626,7 +1626,26 @@ function system_update_last_removed() {
+  $table = '{node_type}';
+  $query = "SELECT type FROM $table WHERE module = :module";

I think this isn't necessary, just use {node_type} directly.

Arguments are usually passed directly to the function too, but this is fine too I guess.

+++ b/modules/system/system.installundefined
@@ -1626,7 +1626,26 @@ function system_update_last_removed() {
+  $result = db_query($query, $arg)->fetchField();

$result usually indicates that this is a result resource, not an actual result. So it's better to use something like $blog_type, or whatever.

+++ b/modules/system/system.installundefined
@@ -1626,7 +1626,26 @@ function system_update_last_removed() {
+  ¶
+  // If there is a blog node type, change its module field and base field values
+  //  respectively.

Trailing spaces on the line above the comment, one space too much on the second comment line after the //.

+++ b/modules/system/system.installundefined
@@ -1626,7 +1626,26 @@ function system_update_last_removed() {
+  if ($result !== FALSE) {
+    db_update($table)
+      ->fields(array(
+          'module' => 'node',
+          'base' => 'node_content',
+      ))
+      ->condition('module', 'blog', '=')
+      ->execute();

Actually, all you need to do is execute this query unconditionally. If there is no blog content type, it simply will not do anything.

Also, the '=' is the default argument and it is not necessary to make this explicit.

10 days to next Drupal core point release.

@11
Berdir, thank you for your comments. As you point out, I understand we don't need to "check" if a blog node type record exists in the node_type table, because after all my update function has a condition which does the same thing basically. Thus, I shortened my patch almost by half.

Yes, exactly.

Long break since my last update. My next goal is to write a test for the upgrade, and I tried to start working on it from scratch.

It turned out that the patch at #12 was not applicable, so I rewrote it. Basically I wrapped my code in a new system update function.

But because /includes/bootstrap.inc calls includes/entity.inc, which doesn't exist on D8, on the line 3009, we cannot easily upgrade D7 site to D8 at this moment. I would love to come back when the entity.inc issue is addressed.

+++ b/modules/system/system.installundefined
@@ -1642,6 +1642,22 @@ function system_update_8001() {
 /**
+ * Convert Blog module nodes.
+ */

This comment reads a bit cryptic. Otherwise this looks good to me.

+  db_update('{node_type}')

Why are there curly braces { } in the table name string? Is this new for Drupal 8? It definitely isn't used for DBTNG queries in 7...

+ db_update('{node_type}')
Why are there curly braces { } in the table name string? Is this new for Drupal 8? It definitely isn't used for DBTNG queries in 7...

My understanding is that the curly braces are used to allow users to use table name prefix. With curly braces, Drupal's Database abstraction layer checks if the current Drupal installation is using any table name prefix. If it is, then the db abstraction layer adds the prefix to the table name just provided as a parameter. If it is not, then the db abstraction layer does nothing and execute db related functions.

But I can be wrong. Hope someone with more knowledge of Drupal programming adds his/her comments.

The curly braces are only needed when doing plain SQL statements with db_query(). When using abstracted DBTNG functions and methods, they need to be omitted.

I'm adding the 'Novice' tag because taking the existing patch and implementing the feedback shouldn't be too hard.

So question - how the heck is http://drupal.org/project/blog supposed to work if this lands?

We're not dropping the blog type, we just change it so that node.module is reponsible for rendering the view, forms and stuff so that it is possible to view and change existing blogs on your site.

If you then install blog.module from contrib, it should be able to easily set module and base back to blog for that type in update hook (assuming that we're keeping the info in the system table if you had blog.module installed and the install/enable hook if not).

Updates per #15 and #18 in addition to d8 /core update.

#22 has been re-attached with correct comment # in patchname.

@xjm asked me to chime in on getting a populated dump and the shell script I use is the following (please note that you need drush installed on your system)

    rm -rf drupal-dumps/drupal
    drush dl drupal-7.x --destination=drupal-dumps --drupal-project-rename=drupal
    cd drupal-dumps/drupal
    drush site-install -y standard --account-name=admin --account-pass=drupal --db-url=sqlite:./.ht.drupal.sqlite
    # install all modules
    drush pm-list --status="not installed" | sed 's/.*(\(.*\)).*/\1/p' | xargs drush en -y
    php scripts/dump-database-d7.sh > ../d7.standard.bare.php
    php scripts/generate-d7-content.sh
    php scripts/dump-database-d7.sh > ../d7.standard.filled.php

This will help speed up the process in generating dumps. However, what needs to be edited is the generate-d7-content.sh file so we need to save another type of content which we can then test against. I think the change would be

diff --git a/core/scripts/generate-d7-content.sh b/core/scripts/generate-d7-content.sh
index e65c099..36df0b4 100644
--- a/core/scripts/generate-d7-content.sh
+++ b/core/scripts/generate-d7-content.sh
@@ -158,12 +158,20 @@ for ($i = 0; $i < 24; $i++) {
 $node_id = 0;
 $revision_id = 0;
 module_load_include('inc', 'node', 'node.pages');
-for ($i = 0; $i < 24; $i++) {
+for ($i = 0; $i < 36; $i++) {
   $uid = intval($i / 8) + 3;
   $user = user_load($uid);
   $node = new stdClass();
   $node->uid = $uid;
-  $node->type = $i < 12 ? 'page' : 'story';
+  if ($i < 12) {
+    $node->type = 'page';
+  }
+  else if ($i < 24) {
+    $node->type = 'story';
+  }
+  else if (module_exists('blog')) {
+    $node->type = 'blog';
+  }
   $node->sticky = 0;
   ++$node_id;
   ++$revision_id;

Yes, it needs to be rolled into the patch and tested.

> So question - how the heck is http://drupal.org/project/blog supposed to work if this lands?

Well that would put you in the situation at #468976: Module-defined bundle is not installed when a custom with same name already exists.

This doc should help:
http://drupal.org/node/1429136

Also, for comparison:
#1280792-55: Trigger upgrade path: Node triggers removed when upgrading to 7-dev from 6.25

Ok, here we go. Removing Novice tag, it wasn't that easy :p

The attached patch updates the gzipped filled database structure and adds a simple check to verify that the upgrade did what it's supposed to. Also updates the generate script to generate some blog nodes. I noticed that there seem to have been made a bunch of cleanup changes to that script which are only in the 7.x branch. No idea why.

The patch has been generated with "git diff 8.x -c > upgrade_blog_with_tests.patch", never made a patch with binary changes before, tell me if I need to change anything.

Thanks Berdir! Unfortunately the patch seems to be full of gobbletygook. :) Did you use --binary?

Let's try again with binary.

Looking at the diff, I guess the problem is that my filled dump installed *all* (the previous one did not) modules and then the language dump tries to install locale on top of it.

Will look into it later.

Ah, I see. In #1352000: Forward-port upgrade test clean-ups from 7.x to 8.x, the goal was for its structure to be similar to how things were set up for 7.x. We'd have 2 sets of dump (one on minimal profile with bare/filled dumps, the other with a standard profile + all modules also with bare/filled dumps). That issue needs to get fixed up and hopefully it can help out with this one as well (I imagine you might be able to use your dumps with a minimal profile?)

Unassign myself until the blocking issue is dealt with :)

I don't think it's blocking this one... we just need to look a bit more closely in what we're putting in the dumps.

I'll try to look at it this weekend as well; not assigning to myself though in case I don't. :)

I haven't looked at this patch or tried to roll anything with this. But I did make some changes to the language.database.php.patch file so it doesn't act so rudely. It either needs to get rolled in here or in the forward-port issue (but posting here for now so someone will take a look).

Setting to needs review.

Ack, this should get combined with #33. Try again.

Changing the update number so its not conflicting with a function update number in core.

Patch in #44 forgot to add the database that the #42 had. That's why the files sizes are so dramatically different.

Rebased, updated upgrade function counter, redumped the database. Not sure if I did the latter right.

This time really with "updated upgrade function counter".

+++ b/core/modules/system/system.installundefined
@@ -1847,6 +1847,22 @@ function system_update_8007() {
+  db_update('node_type')

Since Node module is now optional, should we check the table exists, in case someone does this update D8 to D8?

Oh dear: old patch, wrong patch, accidantely changing component, cross posting with testbot. Right patch, this time.

Ok. We're back to #35. Not sure how to solve it, but the patch applies again. This is the error when running the test locally:

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'bartik-locale-language' for key 'tmd': INSERT INTO {block} (module, delta, theme, status, weight, region, custom, visibility, pages, title, cache) VALUES (:db_insert_placeholder_0, [...]) in require() (line 53 of /home/niklas/Projekte/drupal-8/core/modules/system/tests/upgrade/drupal-7.language.database.php)./

Alright, yes. Instead of enabling all modules, enable only those that were enabled for the old dump. Now were getting real fails for this blog module upgrade test. Or real fails because I forgot to enable the Blog module before dumping.

Ok. With that it passes locally.

What else needs to happen?

1. Point 1 - updating the node_type table - is done. Should we check that the table actually exists, since Node module is optional?
2. Point 2 - updating the role_permissions table - doesn't seem to be nescessary. Even the permissions for the Blog entry content type are already provided by the node module, giving records like this:
   

   mysql> select * from role_permission;
   +-----+-------------------------+--------------+
   | rid | permission              | module       |
   +-----+-------------------------+--------------+
   |   1 | access content          | node         |
   |   2 | access content          | node         |
   |   2 | create blog content     | node         |
   |   2 | delete any blog content | node         |
   |   2 | delete own blog content | node         |
   |   2 | edit any blog content   | node         |
   |   2 | edit own blog content   | node         |
   |   2 | edit own test content   | node         | <-- Custom content type for testing
   ...
   

Notes to myself: Rewrite this

+++ b/core/modules/system/system.installundefined
@@ -1847,6 +1847,22 @@ function system_update_8007() {
+  // Alter {node_type} table.
+  // If there is a blog node type, change its module field and base field values
+  // respectively.

and remove the empty line:

+++ b/core/modules/system/tests/upgrade/upgrade_filled.testundefined
@@ -27,5 +27,11 @@ class FilledUpgradePathTestCase extends UpgradePathTestCase {
+    $this->assertEqual('node_content', $blog_type->base);
+
   }

Regarding #58.1, I would recommend that a version of the upgrade tests should be run with a profile that includes "only" the required Drupal modules enabled (ie with the node module disabled since it is intended to be optional).

Thanks to @xjm for helping with the doxygen.

Edit: Crosspost with #60: I asked in IRC: Since Node module was required in D7 the question is basically: Do we support D8 -> D8 upgrades yet? The answer is no. (Once we have an unstable release that answer would depend on #1333898: Support unstable2unstable upgrades? [policy, no patch].) So for now, we can go with the patch as is.

@Lars Toomre, what you asked for belongs in http://drupal.org/node/1352000 which requires 4 dumps to be created.

#61: 1263208-blog-upgrade-60.patch queued for re-testing.

Reroll after #1352000: Forward-port upgrade test clean-ups from 7.x to 8.x.

None of the module combinations are working for me. New instructions to create the dumps needed ;)

#!/bin/sh

sudo rm -rf drupal
drush dl drupal-7.x --destination=. --drupal-project-rename=drupal
cd drupal
drush site-install -y standard --account-name=admin --account-pass=drupal --db-url=sqlite:./.ht.drupal.sqlite

drush en --yes block color comment contextual dashboard dblog field field_sql_storage field_ui file filter help image list menu node number options overlay path rdf search shortcut system taxonomy text toolbar update user standard
#drush en --yes blog

#drush pm-list --status="not installed" | sed 's/.*(\(.*\)).*/\1/p' | xargs drush en -y

#drush en --yes block aggregator blog book color comment contact contextual dashboard dblog field field_sql_storage field_ui file filter help image list locale menu node number openid options overlay path php poll rdf search shortcut simpletest statistics syslog system taxonomy text toolbar tracker translation trigger update user forum

patch -p1 < ../blog-patch.patch

php scripts/dump-database-d7.sh > ../drupal-7.bare.standard_all.database.php
php scripts/generate-d7-content.sh
php scripts/dump-database-d7.sh > ../drupal-7.filled.standard_all.database.php
cd ..
gzip *.php

Sorry I did not take a look at this sooner.

The changes from the last 'forward port D7 upgrade path test changes to D8' is likely to have messed around with this issue. I'll make some time for it this and next week to see what needs to change back.

Sorry for not being able to get to this sooner than I would have liked. First, I want to separate out the patch from the dump. So in the event we need to do a reroll in the future, it is easier to work with.

I'm revising the patchonly portion as there were a few fixes needed in there:

1. I've set for a default node type of 'page'; this is because when the blog module is not enabled (for minimal install), the node type is not set and things get messy.
2. node_type_get_type no longer exists. The most suitable equivalent seems to be node_type_load.
3. I'm now placing the check in both the bare and the filled db. I don't know what other tests will be necessary for the blog content type.

And I am attaching the combined patch with the new db dump based on the new generate-d7-script (which will also need to be updated for D7 when we create any dumps in the future. As such, we will need to create a backport patch only for the script once this is in D8).

Finally, unassigned :)

Wow, that is a pretty messed up failure - I wonder if adding the new nodes had anything to do with it? Also...I see it asks for uid 6 in the testbase...I'll take a look into when that uid gets added in.

I tried by changing the uid to 1 in drupal-7.language.database.php and it seems to work correctly. So it does seem to be tied to the uid being non-existent in the new dump. I'll do some further debugging in the dumps if a user with uid 6 exists.

Another update - the tests were failing because I had not generated the content. I found that even after I had created the content, however, the tests failed. That is because of conflicting nids. When poll module exists, nodes of type 'poll' get created as well - since we are now creating blog posts, the same happens. I've adjusted the nids of the i18n content. But I do see there is some other fail happening. I need to figure it out but I'm uploading the updated patch on its own + the zips.

Aha! It had to do with not updating the tnid to the new values. Attaching new patchonly version (which has the language.php change) and the combined.

I read some of the comments above and it is not entirely clear to me why we need to change the nids etc. around. It seems at first glance that only the first and last hunks are needed for this patch. Can you elaborate, please?

+++ b/core/scripts/generate-d7-content.shundefined
@@ -160,12 +160,21 @@ for ($i = 0; $i < 24; $i++) {
-for ($i = 0; $i < 24; $i++) {
+for ($i = 0; $i < 36; $i++) {
   $uid = intval($i / 8) + 3;

The nids are probably incremented because of a longer for loop (12 instances).

Dunno why the vid is incremented by 20

22 days to next Drupal core point release.

Yep, the nids are incremented due to the longer loop - same goes for the vid. I'm not completely sure on what the vid increases by (atleast 12 is certain, but it doesn't seem to be harmful given that we are increasing the vids by 5 in between each nid). I can try and figure out what the vid should be though if that would be better.

I'm not sure which order this issue should be in, but perhaps we should wait for any further work on this until #1619898: Convert upgrade tests to PSR-0 gets in?

Well this one is marked critical, so when it's ready I think it should have first priority.
So far, the patch without tests has never successfully failed. Otherwise this would be RTBC.

(BTW thanks for #77/#78, that makes sense.)

This can go in first. If it's done before the other one. I can reroll.

Hmm, that would mean our tests need to be more robust :)

The patch that would just be the test would be all the stuff without the update in system.install. Looking at this...it might be more worthwhile to have another test case so we can flesh out testing for the blog move. How does that sound?

Well, as far as I can see, the tests-only patch so far never successfully applied. I don't know if it would actually pass (which would be bad in that case).

Are you referring to the patch-only version? My guess is that would be because of the binary diff chunks that are in there. I'll look into this sometime tomorrow in any case :)

#75: 1263208.combined.patch queued for re-testing.

Ok, re-rolled.

The test-only patches now also contains the binary diff of the dump files and should therefor apply and the language tests should pass. I could leave them out but then would the language tests fail.

The complete patch also contains the changes in the generate script and system.install.

I don't know if there have been any changes to these dumps in other issues. I hope not.

Does this need manual testing? If yes please provide me with a test plan and I'll do it :)

Manual testing should involve something like this I guess:

- Create Drupal 7 site
- Enable blog module
- Create some blog posts
- Upgrade to Drupal 8
- Make sure that you can still view and edit your blog posts.

OK I tested the upgrade path and it works :)
I reviewed the changes and they are prety simple, so marking this rtbc!

I am not removing the RTBC yet but why do we patch the base dump? In D7 we used to add small dumps for invidual tests.

FWIW: The only actual upgrade path change in this patch is:

+++ b/core/modules/system/system.install
@@ -1925,6 +1925,19 @@ function system_update_8010() {
 /**
+ * Update the module and base fields for the blog node type.
+ */
+function system_update_8011() {
+  db_update('node_type')
+    ->fields(array(
+      'module' => 'node',
+      'base' => 'node_content',
+    ))
+    ->condition('module', 'blog')
+    ->execute();
+}

This looks correct to me.

We do not seem to provide a upgrade path for the remainder of the Blog module functionality; e.g., per-user blog views, also containing a per-user link to create a new blog post on that view. But of course, there's hope that Views in core will be able to replace + provide just that.

And yes, the upgrade path dump diffs are still a PITA. But for now, I couldn't care less. We have a completely different approach in the pipeline for some time already in #1364798: Impossible to generate meaningful diffs of upgrade db dumps, but even though I initiated that approach myself, I deferred/ignored it recently, because it involves quite some advanced usage of our current test framework, and I want/need to figure out the future of our test framework first, before possibly entering a path/direction that might be incompatible (or not).

In short: Another +1 on this. Time to finally get rid of this issue.

Thanks everyone who has worked on this! :)

Committed to 8.x. Thanks. I think this is done now.

Fixing tags per http://drupal.org/node/1517250

From comment #70:

And I am attaching the combined patch with the new db dump based on the new generate-d7-script (which will also need to be updated for D7 when we create any dumps in the future. As such, we will need to create a backport patch only for the script once this is in D8).

If that is still true then there is something to backport?