D6->D7 update does not create required menus

How does this look? A lot of things happend in #273137: Split Navigation to User and Administration menu, and I'm not sure I captured all relevant changes in the patch.

I think this needs a little more work - to me this function looks broken also: http://api.drupal.org/api/function/system_update_7009/7

Reproducing this problem, it makes it rather hard to do upgrade testing from Drupal 6, which is important to keep patches working for the future upgrade.

Regarding the actual patch: the management and navigation menu blocks are now provided by system module, so I don't see why would we need to create the menus here. They seem to be already there. The issue, is that user-block-1 became system-block-navigation and there is this new system-block-management, which nobody enables. The upgrade path migrates user-block-1 to user-block-navigation, which is not true anymore. So we end up without navigation and management blocks.

Manually enabling both blocks after the upgrade gets me the management menu properly (create content + administer), but gets an empty navigation menu (block is not displaying). I bet the later is due to that I did not have any custom nav links, I just did a fresh Drupal 6.11 install to start the upgrade.

So the patch definitely needs work.

In #511258: Do not enable the management menu by default, we are discussing not enabling the management menu block by default, since toolbar module is already there. Once that is resolved, the upgrade path should do the same.

I can only guess this is still an issue. Horrible issue.

.

This patch fixes the user navigation block to the system navigation and changes all 5 default system menus to match what happens on menu_install(). I tested it on a fresh install of D6 upgraded to latest D7 HEAD and it properly configured the menus for me.

Reroll. Still figuring out patching and git.

Couldn't this use http://api.drupal.org/api/function/menu_save/7 otherwise looks good.

Didn't mean to change status.

Could use http://api.drupal.org/api/function/menu_save/7 for the new menu items but couldn't use it for updating the old menus since we are changing the menu_name entry in the database. If it is important I can switch the few inserts to menu_saves but otherwise I'll leave the patch as is.

Can't use menu_save() because it's a hook_update_N() anyway, RTBC.

It's late, so I might be mis-reading this, but won't this obliterate any custom title/description people may have given to the primary links/secondary links/navigation menu?

I'm not sure the upgrade path should be futzing with those...

No I think you're right, we only want the inserts here.

Re-roll with the db_update('menu_custom) calls removed.

Side note, is it a "feature" that you can not rename a system menu in D7? I just saw that while comparing the edit form/description on D6/D7....

Ups...

Committed to HEAD! Thanks!

And I know that HEAD-to-HEAD is not supported, but I get

The following updates returned messages
system module
Update #7053

    * Failed: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'user-menu' for key 'PRIMARY'

when running update.php.

Also, even though this update fails, admin/reports/status still insists that it needs to be done. What is the correct behavior when an update fails? Does it remain "Database updates: Out of date" forever? What's the correct way out of this situation?

The easiest way to prevent that from running is have your own update handler which sets system schema version manually. We may need to add that to head2head.

However I think that's a bug in the patch - if a D6 site happens to have a custom menu with that name, they'll get errors too, so we should do a menu_get_menus() and only create them if they don't already exists instead I think.

I know how to fiddle with the system schema version.... But that doesn't answer the question (perhaps not for this issue): What happens to an end user when an update fails? In D6 (I believe) the update was considered to be "done" even though it failed and was then ignored. In D7 it appears to me that if it fails it's always considered "not done". I think the D7 way is probably better, but we aren't really providing a way for the end user to deal with it.

I should also mention that the failure in #23 is *not* logged to dblog at all, and certainly not with details that would allow it to be debugged. Is that another issue that I should open? EDIT: I see #774882: update.php: errors are not logged

Ahh I see, yeah I think the current behaviour is as good as we'll get on that. In this case the upgrade itself has an error, so it not running should result in a fix, which would then allow it to proceed the next time the user updates their site. If the database being upgraded has corrupted behaviour for some reason, then that'd likely take manual intervention before the update could run, I'm not sure there's much else we can do (apart from getting that logging issue committed).

@Gabor in #6 and @David in #22:

Upgrading a Drupal 6 site that had the Navigation men block enabled, and finding no management menu is a pretty big issue. I just test the upgrade path and spent a good minute staring* and thinking about what had happened to all my menu links, before remembering the change. Gabor's comment about the toolbar doesn't apply since no upgraded site will get the toolbar enabled either.

* I may have just been in awe of how smoothly the upgrade went.

Well, as said, we could at least enable the toolbar module on upgrade. We do this with other modules. A notable exception in the D6 upgrade was the update module, but we chose to not enable that automatically, so people can get informed about the types of details it shares with drupal.org. That does not apply here, so we can just go ahead and enable it in the update.

Here is a patch that, as Gábor suggested, enables the toolbar on update.

Nice improvement. RTBC for me

+++ modules/system/system.install	20 Jun 2010 08:37:17 -0000
@@ -2415,6 +2415,14 @@ function system_update_7054() {
+ * Enable toolbar module.
+ */
+function system_update_7055() {
+  $module_list = array('toolbar');
+  module_enable($module_list, FALSE);
+}

small patch, code style looks ok.

based on #30, rtbc

59 critical left. Go review some!

Still needs work for #24 - D6 sites may have menus with the same name, and the current update in HEAD fails in that case.

User created menus are prefixed, so such a conflict should be rare or non-existent outside of a head2head scenario.

As catch says in IRC - we just need to make sure they exist - if it's a real problem, we can use a merge query instead of the API function to create them.

After discussion on IRC with catch an pwolanin, the D6 sites with the same name are highly unlikely, as custom menus are prefixed with 'menu-', and the head2head case is currently not supported, so that should not pose a problem.

I think all the comments from #22 are addressed in this version:

1) The toolbar module is enabled and so the administrator has access to the menus.

2) Check for table existance. If the table does not exist, the module is not installed; The insertion will occur when the module is enabled.

3) Adding the extra functionality to update_fix_d7_block_deltas() was pretty straight forward

4) The function was indeed incorrect (although it was merely a naming error), but should now be corrected.

Other idea: Would the user and management menu need to be enabled by default if the navigation menu was enabled in D6?

This works well, and created the management menu and enabled the toolbar module. No errors on a site with menu module previously installed.

Now it feels really weird for the site to have toolbar on, overlay off, and not using seven theme :(.

I'll reroll a patch to correct the typo after there is a decision which way to go here:

1) Enable the toolbar => need to hand out permissions: same roles as access administration pages? Might conflict with other "toolbars" such as admin_menu

2) Enable the management menu => looks more like the old situation. Could be in the same region as the navigation menu.

Either way, we should probably enable the menu-user block if the navigation block was previously active. Otherwise normal users can't log themselves out without knowing the path.

Also, in what order should the menu blocks show up?

I really think as long as the person doing the upgrade can find their way to /admin and /user without knowing the URLs, we shouldn't worry too much about the details of the blocks.

When you upgrade a major Drupal version, there is always configuration to change, blocks are a common part of that. In Drupal 5-6 you were lucky if your menu resembled the Drupal 5 one at all.

Having said that, the management block seems like a much less intrusive change if we have to auto-enable something.

#36: 410636-toolbar-and-22.patch queued for re-testing.

A new patch that enables the user-menu and management block on the same places navigation is shown.

#22.2 is fixed in #831732: system_update_7053 needs to check for existence of menu_custom table so that is removed from this patch.

Last patch had some code commented out for testing, but which should be enabled.

edit: I used block_flush_caches because otherwise the user-menu and management block weren't in the block table yet. This could have consequences for other modules who still need to migrate their blocks.

Should the block_flush_caches be replaced by inserting the blocks manually here, or should other modules depend on this update having run already?

+ * Example:
+ * $moved_deltas = array(
+ * 'user' =>
+ * array(
+ * 'navigation' => 'system',
+ * ),
+ * );

There is an @code tag for code in doxygen.
I think we have to use it in this case.

Reroll with code tags added.

Last patch had ANSI colors in it

forgot to change the status

+++ includes/update.inc	26 Jun 2010 07:25:30 -0000
@@ -228,8 +229,21 @@ function update_prepare_d7_bootstrap() {
+ *   An associative array. Keys are source module names, values an associative array
+ *   mappign the (possibly renamed) block name to the new module name.

Srry that I missed this.

1) line exceeds 80 characters
2) mappign? probably has to be mapping

45 critical left. Go review some!

Rerolled with comments from #51

#52: 410636-fix-comment.patch queued for re-testing.

Added update dependency to make sure the renamed the block tables exist.

add beta blocker tag

+++ includes/update.inc	10 Jul 2010 18:25:12 -0000
@@ -252,6 +266,23 @@ function update_fix_d7_block_deltas(&$sa
+          $block_exists = db_query("SELECT COUNT(*) FROM {" . $table . "} WHERE module = :module AND delta = :delta", array(
+            ':module' => $old_module,
+            ':delta' => $delta,
+          ))
+          ->fetchField();

Following lines after $block_exists... need one more level of indentation.

+++ modules/system/system.install	10 Jul 2010 18:25:12 -0000
@@ -2419,6 +2425,25 @@ function system_update_7053() {
+  // Show the new menus on themes and places the navigation block is shown

Missing period. Sentence also reads a bit hard for me, but perhaps it's only me.

+++ modules/system/system.install	10 Jul 2010 18:25:12 -0000
@@ -2419,6 +2425,25 @@ function system_update_7053() {
+  $blocks = db_query("SELECT theme, status, region, weight, visibility, pages FROM {block} WHERE module = 'system' AND delta = 'navigation'");
...
+    db_update('block')
...
+      ->condition(db_or()->condition('delta', 'user-menu')->condition('delta', 'management'))
+      ->execute();

What happens if the Navigation menu block is not enabled? Do we expect that people have modules/toolbars like admin_menu installed + upgrading them at the same time? I'd be ok with that, just mentioning the possibility.

Powered by Dreditor.

Also the db_or() conditions should be placed on separate lines, one indentation level deeper. Or, probably even better, it should be generated once outside the foreach and then be reused.

Something like

  $menu = db_or()
    ->condition('delta', 'user-menu')
    ->condition('delta', 'management');
  foreach () {
  // ....
    ->condition($menu)
  }

Rerolled with comments from sun and Berdir.

I don't think we can't expect people to have modules/toolbars installed, as the upgrade docs say you need to disable all contrib modules/themes before upgrading.
So that only leaves 2 options before the upgrade starts: people having a visible navigation, and people not having visible navigation. Anyway, after the upgrade has ended the navigation visibility will be the same as before. This looks acceptable to me.

RTBC afterwards:

+++ includes/update.inc	12 Jul 2010 06:53:29 -0000
@@ -239,10 +253,10 @@ function update_fix_d7_block_deltas(&$sa
+            ->fetchField();
           if ($block_exists) {

@@ -252,6 +266,23 @@ function update_fix_d7_block_deltas(&$sa
+          ->fetchField();
+          if ($block_exists) {

Seems like this DBTNG code exists twice ;)

+++ modules/system/system.install	12 Jul 2010 06:53:30 -0000
@@ -1632,6 +1632,17 @@ function system_update_last_removed() {
+}
+/**

Missing blank line.

50 critical left. Go review some!

Another reroll with the comments from #60

I would like to see a summary of this. I see lots of patches and reviews but very little info on the actual problem and solution.

Here are the changes made with this patch:
- includes/update.inc: Allow moving blocks between modules.
- modules/system/system.install:
-- system_update_7004: Move the navigation block from the user to the system module.
-- system_update_7009: Fix the rename of the primary menu variable.
-- system_update_7053: Enable the 'user menu' and 'management' menu blocks on places where the navigation block is shown.

So there are actually three things being fixed in this issue :( Would probably have been better when this patch was splitted up instead of adding everything here.

I think what would be helpful for yoroy (and core committers) are before/after screenshots of the upgrade path with this patch.

There's little to see here. This issue went into different directions before, now we're doing the most simple solution, just ensuring that any previously existing menus and blocks are still displayed (some have been renamed and splitted) after upgrading.

I can't imagine any usability issue derived from this patch, which would require screenshots.

Thx sun for updating the issue title, forgot about it

I've added some screenshots, but there really isn't that much to see here.

Looks good, and I agree that there are no UX implications here. Committed to CVS HEAD. Thanks.