This patch depends on http://drupal.org/node/147656 and part of the 'birthday patches' series :)

CommentFileSizeAuthor
#102 typo_1.patch648 bytesbdragon
#101 menu_147657.patch560 bytesdrewish
#99 menu_update_29.patch21.84 KBchx
#93 menu_update_28.patch20.94 KBchx
#89 menu.module.rej__1.txt3.13 KBpatrickfgoddard
#85 menu_update_27.patch22.23 KBpwolanin
#84 menu_update_26.patch25.54 KBwebernet
#82 menu_update_25.patch23.65 KBchx
#80 menu_update_24.patch17.38 KBchx
#79 menu_update_23.patch17.05 KBchx
#76 menu_update_19.patch17 KBchx
#75 m1.png7.58 KBchx
#74 menu_update_22.patch16.86 KBpwolanin
#73 menu_update_21.patch16.24 KBpwolanin
#72 menu_update_20.patch16.15 KBpwolanin
#71 patch_166.txt15.49 KBwebernet
#70 menu_update_18.patch14.77 KBchx
#69 menu_update_17.patch14.55 KBchx
#67 menu_update_16.patch15.61 KBwebchick
#64 menu_update_15.patch14.5 KBchx
#61 menu_update_14.patch14.59 KBchx
#59 menu_update_13.patch12.57 KBchx
#58 menu_update_12.patch12.76 KBpwolanin
#57 menu_update_11.patch12.4 KBchx
#56 menu_update_10.patch12.38 KBpwolanin
#55 menu_update_9.patch12.75 KBpwolanin
#54 menu_update_8.patch11.18 KBpwolanin
#52 menu_update_7.patch8.79 KBchx
#50 menu_update_6.patch9.24 KBchx
#49 menu_update_5.patch9.1 KBchx
#48 menu_update_4.patch9.09 KBchx
#47 menu_update_3.patch7.53 KBchx
#46 menu_update_2.patch5.98 KBchx
#44 menu_update_1.patch5.9 KBchx
#43 menu_update_0.patch5.97 KBchx
#39 menu_update.patch4.73 KBchx
#35 sysup_12.patch8.72 KBpwolanin
#29 Picture 6_4.png87.81 KBagentrickard
#27 Picture 4_2.png84.31 KBagentrickard
#26 sysup_11.patch27.07 KBpwolanin
#21 sysup_10.patch28.26 KBpwolanin
#19 sysup_9.patch28.13 KBpwolanin
#18 sysup_8.patch27.47 KBpwolanin
#17 sysup_7.patch26.33 KBpwolanin
#16 sysup_6.patch23.64 KBpwolanin
#15 sysup_5.patch23.44 KBpwolanin
#14 sysup_4.patch23.44 KBpwolanin
#13 sysup_3.patch21.21 KBpwolanin
#12 sysup_2.patch14.23 KBpwolanin
#8 graphics_on_links_gone.png113.33 KBmorphir
#4 sysup_0.patch8.52 KBchx
sysup.patch3.57 KBchx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

morphir’s picture

subscribe. Gonna test it early tomorrow morning.

pwolanin’s picture

subscribe

bjaspan’s picture

Status: Needs review » Needs work

The table creation part of this patch needs to be rewritten. Short form: You cannot assume that .schema files contain the right thing during update functions. Long form: http://drupal.org/node/144765#comment-245805.

chx’s picture

FileSize
8.52 KB

I can see your problem, though it does not apply now, it might apply later. The patch is same as before I just copied the schema instead of including it.

chx’s picture

Status: Needs work » Needs review
morphir’s picture

i simply get access denied. So I set $access_check = FALSE;

Now. during the upgrade I get an error:

Updating
An unrecoverable error has occured. You can find the error message below. It is advised to copy it to the clipboard for reference.
Please continue to the error page
Fatal error: Call to undefined function _menu_link_build() in /home/morphir/www/drupal5/modules/system/system.install on line 3416

that's how far I got :/

morphir’s picture

when I go back to the site. this error message occurs:



    * user warning: Table 'menu.menu_custom' doesn't exist query: SELECT * FROM menu_custom WHERE admin = 1 in /home/morphir/www/drupal5/includes/database.mysqli.inc on line 154.
    * warning: Missing argument 1 for watchdog_menu() in /home/morphir/www/drupal5/modules/watchdog/watchdog.module on line 33.
    * notice: Undefined variable: may_cache in /home/morphir/www/drupal5/modules/watchdog/watchdog.module on line 36.
    * notice: Undefined variable: output in /home/morphir/www/drupal5/includes/batch.inc on line 42.
    * notice: Undefined variable: output in /home/morphir/www/drupal5/includes/batch.inc on line 42.
    * notice: Undefined variable: output in /home/morphir/www/drupal5/includes/batch.inc on line 42.


morphir’s picture

FileSize
113.33 KB

I'm sorry for the poor review.

This time I did it properly. To get the update.php running I had to set $access_check = FALSE; on line 14.
It worked.

Then I did it a second time, but this time I set the the site in offline mode, made two nodes with path alias and linking to the primary menu.
I ran the update.php script, and it worked this time as well. But when I was going to visit the site, I'm unable to log in.

NB! Do not set site in offline mode while testing this patches yet. Run the update script in online mode.

On the third try I made 2 nodes, with path alias and put them under the primary links menu. It worked. Links and nodes worked perfect.

The only catch was the graphics on the primary links was gone. This I think is a theme issue on the garland. So I will file one there. See screenshot if you are curious.

pwolanin’s picture

I think also for this:

drupal_install_schema('menu');

You need instead to declare the schema again to future-proof the code.

bjaspan’s picture

pwolanin: I don't think drupal_install_schema('menu') in system.install is a problem there. It is installing the menu module schema for the first time. Aftwards, menu.install will handle updates. At the time the menu module is installed, its schema version in the database will be set to the most recent version number in menu.install, which is the same version that install_schema('menu') will create. So everything will line up at the most recent version supported by the code at the moment.

Or am I failing to understand my own warning on this topic?

pwolanin’s picture

My interpretation of the gist of the warning was that the update code can never rely on the .schema file being a particular version, so the update code should explicitly declare the schema for any update.

pwolanin’s picture

Status: Needs review » Needs work
FileSize
14.23 KB

incomplete patch - beginnings of the code to also handle links corresponding to router items that have been moved form their default place in the tree.

pwolanin’s picture

FileSize
21.21 KB

small steps - compared to the previous patch it's more towards getting the module working, than strictly the update code. Quite a number of bugs fixed. for example, function menu_parent_options is much simplified and should now show in the drop-down in the links in the correct order.

pwolanin’s picture

FileSize
23.44 KB

more small steps- add saving of $item['customized'] to menu_link_save().

pwolanin’s picture

FileSize
23.44 KB

missing comma in SQL

pwolanin’s picture

FileSize
23.64 KB

doh- SQL again - guess I should test before making the patch.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
26.33 KB

ok, ready for serious review and testing, with a number of improvements:

  1. fixed the menu elements in the node form so user input is not lost on preview
  2. only show "reset" links for router items that have been customized when viewing pages like admin/build/menu/navigation
  3. fixed code in menu.inc compared to last patch so that new links derived form router items are saved
  4. fixed menu.module code to accommodate changes in menu.inc so that resetting an idem does not duplicate it
  5. the customized flag is used to avoid overwriting user edits of menu links, including the description field of node links.
pwolanin’s picture

FileSize
27.47 KB

minor code style and form ordering fix

pwolanin’s picture

FileSize
28.13 KB

add menu module to the default profile

hunmonk’s picture

Status: Needs review » Needs work

we need to drop the old menu table after this conversion is complete -- it's pointless to have it in there. as for concerns about failed conversions, we've always clearly instructed to make backups prior to upgrading.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
28.26 KB

with DROP TABLE

Boris Mann’s picture

Title: Menu update from D5 -> D6 » Menu update from D5 -> D6 and fixes that make menus work in D6

Marking this for testing, changed title to reflect the fact that is needed for clean installs as well.

Boris Mann’s picture

Does not fix for fresh install. See http://drupal.org/node/151055 for screenshot.

pwolanin’s picture

Actually, that's the way Karoly coded it, so it's a feature, not a bug, that items stay in the same menu where they were created. You should be able to add the item to any other menu as well, right?

Also, this patch *does* make lots of other things work properly.

pwolanin’s picture

patch still applies cleanly.

pwolanin’s picture

FileSize
27.07 KB

minor re-roll to account for a commit to remove a comment in menu.inc

agentrickard’s picture

FileSize
84.31 KB

Checklist of fixes from pwolanin in IIRC, checked.

* pwolanin> every system-defined item has a "reset" link (even when not edited)
- corrected and verified.

* pwolanin> the drop-down for picking a parent isn't ordered right
- seems to be, but not all items appeared on first view (see below).

* pwolanin> the menu_otf form in the node form is very big and unwieldy, and located way at the bottom
- form is in the right place. entry field too large. I couldn't save nodes, though and got access denied errors probably unrelated to menu-otf. See SQL error below.

* pwolanin> there's also the update code
- did not test

Needs work:

* resetting an edited default item causes the "expanded" yes/no column to vanish in the list view -- see screenshot. But they came back after I created a new menu (seems like a menu rebuild issue).

* the first time I went to edit a menu item (under Navigation), I wasn't given the full list of menu items in the 'parent' dropdown, only the "parent" items from the Navigation menu were shown (not their children as well). This behavior changed after the menu was rebuilt when I created a new menu.

* title on pages like 'http://127.0.0.1/drupal/admin/build/menu/item/edit/10' is "Home" and breadcrumbs only show 'Home'

* Access denied pages still force strange behavior for primary links, as the top-level (parent) Navigation items appear as primary links.

Node creation error:

user warning: Column 'vid' cannot be null query: node_save INSERT INTO node (vid, title, type, uid, status, language, created, changed, comment, promote, sticky) VALUES (NULL, 'Abouy', 'page', 2, 1, '', 1181702765, 1181702765, 0, 0, 0) in

agentrickard’s picture

Testing again on clean install on PostGres 8.2.3, applying the patch before installing.

agentrickard’s picture

FileSize
87.81 KB

OK, my first review applied the patch to an existing 6-dev install. For the second try, I applied the patch to a new directory and then created a new Drupal site running on PGSQL 8.2.3.

Findings:

- Menu module not active by default.
- http://127.0.0.1/drupalpg/admin/build/menu and http://127.0.0.1/drupalpg/admin/build/menu/list do not return the same content.
The first link returns a list of default menus:

    Navigation
      The navigation menu is provided by Drupal and is the main interactive menu for any site. It is usually the only menu that contains personalized links for authenticated users, and is often not even visible to anonymous users.
    Primary links
      Primary links are often used at the theme layer to show the major sections of a site. A typical representation for primary links would be tabs along the top.
    Secondary links
      Secondary links are often used for pages like legal notices, contact details, and other secondary navigation items that play a lesser role than primary links      

The second link is incorrect and returns the parent menu items from Navigation.

      My account
      Create content
      Administer
      Log out

- menu breadcrumbs build normally.
- resetting a menu item destroys the expanded column in table view.
- node creation error vanished acting as user 1 (I was user 2 in the other test)
- successfully added menu-on-the-fly
- recreated case where full menu parent options did not appear when editing a default menu item (see screenshot).

agentrickard’s picture

Um, ignore that last screenshot. Obviously you cannot assign a parent item to its children. I just expected to see the entire menu list, and it was truncated drastically when editing the Administer item.

So I see two issues on the fresh install:

* http://127.0.0.1/drupalpg/admin/build/menu and http://127.0.0.1/drupalpg/admin/build/menu/list do not return the same content.

* resetting a menu item destroys the expanded column in table view.

pwolanin’s picture

the node creation error is unrelated to this patch (relates a change to the schema for {node})

There is a separate issue for fixing the link of all default tabs (so it won't point to admin/build/menu/list): http://drupal.org/node/115847

will check on other items - thanks.

pwolanin’s picture

@agentrickard one clarification. I was mistaken on IRC - the user access page does not force a menu rebuild, since all menu permissions are checked dynamically.

agentrickard’s picture

OK, so user access doesn't need to force menu rebuild. I encountered the menu permission / access control error when applying the patch to an existing D6 site, not when I ran from fresh rebuid, so I think it was a legacy issue.

pwolanin’s picture

@agentrickard - after looking in detail, the screenshot actually represents the correct behavior. The item you are trying to move is /admin. An item cannot become a child of one of its own children, so all the links under /admin are excluded in the drop-down

pwolanin’s picture

Status: Needs review » Needs work
FileSize
8.72 KB

At the request of Karoly,

splitting this patch into two: this issue just for the update code, and a separate issue for menu module fixes: http://drupal.org/node/151583

Note that the update code will depend on the "fixes" patch, so that should be applied before testing this one (until it's committed).

pwolanin’s picture

Title: Menu update from D5 -> D6 and fixes that make menus work in D6 » Menu update from D5 -> D6

this issue is only for the update path now

Gábor Hojtsy’s picture

Just to note: the system_update_6020 part of the patch is already committed as part of a schema API update/fix.

pwolanin’s picture

Priority: Normal » Critical
chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
4.73 KB

There. Works. Does not rely on cache.

We pick a menu item. If it's parent is a menu then we store that. If parent is an item and it is converted, we are happy. If not, then we replace the item with its parent and repeat. As we go towards the root, very soon this climbing ends. We fill the new fields and save. Repeat :)

Gábor Hojtsy’s picture

It would be great to see someone else test this patch (I know you tested it with a considerable dump, but it would be great to test it with another one anyway).

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review
webchick’s picture

Status: Needs review » Needs work

I ran into issues with this patch. The test site was Lullabot's internal blog thing.

We had two custom menus, one which listed all of the free-tagging vocabs in use on the site ("Free Tags"), and the other that had some manual-entered links to stuff ("Secrets"). During the ugprade path, two new items were added to the main "Navigation" menu:

Free Tags (Overview) => admin/build/menu-free-tags
Secrets (Overview) => admin/build/menu-customize/menu-secrets

Each of these takes me to a menu overview page, but all the items are missing.

chx’s picture

Status: Needs work » Needs review
FileSize
5.97 KB

fixed a) secondary links equal to primary links b) custom menu names are properly handled.

chx’s picture

FileSize
5.9 KB

mnus debug message.

webchick’s picture

I'm still getting the same issue... I've sent a sanitized dump of our menu table to chx for analysis.

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
5.98 KB

There are two issues with webchick's dump, one is that menus came earlier than admin/build/menu thus the overview-per-menu got ts parents wrong, it's fixed by an early menu_rebuild and reloading the page. The second, you have a URL like taxonomy_menu/* and that gets removed because it has no router_path . I changed it something that exists and it was working. So now even webchick's dump is behaving as intended. For this reason I deem this RTBC again.

However, we have a problem in here -- UPGRADE.txt says disable contribured menus. Your custom menu items pointing to a path defined by a disabled module are removed on menu_rebuild, That's how we deal with module disable. menu_edit_item_validate and menu_link_save are working hard to ensure you can't enter / save a link which has no router path. From an architecture point of view, this is just good -- why would you want to display a link which leads to nowhere?

When selecting from {menu_links} we already LEFT JOIN on {menu_router} because of external paths so empty router paths are not a problem on use. if we want to, it's not hard to patch menu.inc to remove the safeguard from menu_link_save and change the final steps of _menu_navigation_links_rebuild so that it does not remove dead links. However, this is not something I would advise to do because this would mean links in the menu that lead to 404.

Another solution to have some user switchable "update mode" -- this would switch the behaviour described between "dead links allowed" and "dead links removed". Once all your contribs are in place you would switch this off -- and we would not let it to be switched back.

This is however a totally another followup issue. The menu update works as it should.

chx’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.53 KB

Enhancing Gabor's idea about marking updated links and not deleting them on the next router rebuild, comes this patch.

  • While inside the menu update, we save every link, with or without a router_path.
  • After menu update, we check whether the menu table is there and do not delete customized links which came from there.

This still means that after your site upgrade is finished, you are better off with moving the menu table aside because if you disable a module which existed in five then the links will remain -- but at least there is no variable to set.

chx’s picture

FileSize
9.09 KB

Let's fix broken router_paths on _menu_navigation_links_rebuild .

chx’s picture

FileSize
9.1 KB

I meant this :)

chx’s picture

FileSize
9.24 KB

The design rationale behind this is that when teh modules get enabled and the menu gets rebuild we go and fix router_paths -- and as more and more modules come into place, we need to fix less and less, eventually the new fixer query will become empty. Knowing core committers, I added a code comment :)

webchick’s picture

Status: Needs review » Needs work

We're getting there. This removed those weird entries in the Navigation table, and the 'Secrets' menu looks to be untouched.

However, there's still some weirdness. Here's my current menu from the 5.x site:
* new blog entry
* new book page
* new gig sheet
* new bookmark
* My blog
* Bot
* Create content
* My account
* News aggregator
* Administer
* Log out

Here's my menu after the upgrade:
* new blog entry
* new book page
* new gig sheet
* new bookmark
* My account
* Create content
* Recent posts
* News aggregator
* news aggregator
* Administer
* administer
* Log out
* log out

Weirdness:
- "My blog" is missing. I wonder if this has to do with the fact that in 5,x, the parent item "Blog" is disabled in its child "My blog" is "locked" At any rate, it ought to be there on the new site.
- There are doubles for Administer / administer, Log out / log out. The capitalized version of Administer actually contains the child menu items, the lowercased one does not. chx inferred on IRC that there's no way around this, but IMO that's unacceptable from a usability standpoint. In this database, I have only 3 links to fix manually, but I purposely started with this one because it's the most bare-bones Drupal install with actual data that I have access to. I can only imagine what will happen when a site like MTV UK goes through this transition. :P
- Bot is gone, but that's a contrib module, so no surprises there.
- Recent posts got added. Before, this was part of the "Secondary Links" menu, now it's in both places.
- "My account" got moved in order; it's now above Create content, rather than below.

Other weirdness:
- When I go to admin/build/menu, the only two menu items listed in the sub-nav there are:
* Free Tags (overview)
* Secrets (overview)
It doesn't include Navigation, Primary/Secondary Links, which are also on the page.
- The Secrets menu used to contain a link to 'call', a blog entry containing our conference call #. This menu item got removed somehow, although the 'call' page is still there and I can access it manually.
- All of the disabled links in the 5.x site are gone. For example, in our secondary links

There might be others, but this should do for now.

chx’s picture

FileSize
8.79 KB

still needs work.

pwolanin’s picture

just noticed something interesting/unfortunate: since cache_set doesn't work during install/update, every single call to menu_link_save() will result in a emptying {menu_router} and an expensive call to function _menu_router_build($callbacks). I think we can get around this easily enough - and we should use the same in book.install.

pwolanin’s picture

FileSize
11.18 KB

ok this patch gives you the idea, not well-tested yet.

Rather than relying upon the session to alter the behavior of menu_link_save(), we define a valid, but inaccessible router path in system menu as a placeholder.

So, in the install code we get around the problem of the {menu_router} being rebuilt repeatedly and assign this special router path to the item if it's not external and we can't find a router path.

pwolanin’s picture

FileSize
12.75 KB

Ok this also rolls in some code that chx e-mailed me to remove menu links on uninstall - but I put it in install.inc, and tried to clean out the names of load function.

Seems to work, but leaves behind the help links like: 'admin/help/book' which, I guess, really belong to the help module?

also, this now include a key bit: db_query("DROP TABLE {menu}");

pwolanin’s picture

FileSize
12.38 KB

oh wait, cache_get() doesn't work at all during install, so the attempt to not rebuild the router is pointless.

revised patch.

chx’s picture

FileSize
12.4 KB

Very nice job. A few remarks (and a reroll with them):

+ $callback = isset($item['access_callback']) ? trim($item['access_callback']) : 0; you wanted empty() here.

+ return array('#finished' => FALSE); that's not FALSE that's 0 because it's a number indicating how much is done. Yes, 159 has FALSE as well, and yes, that's also incorrect.

You could rebuild in the setup stage and store $menu in the session. menu_router_build might take the menu in the second argument (currently it has only one) to preseed its static storage.

+ // Skip over external links or links to the front page. <= Huh, we do nto entirely skip them... Maybe "Find router path for internal links (which do not point to the front page)"

1 + (!empty($item['customized'])) Just 1. this was only needed for my special join to menu table.

We still need to tackle somehow the duplicates that Angie saw.

Though there is new code in menu.inc to deal with update issues, most of it is just factoring out _menu_find_router_path, there are like eight lines of coded added.

pwolanin’s picture

FileSize
12.76 KB

progress, of a sort, but as noted with Karoly on IRC items whose router path is missing are not getting the "special" callback as expected.

and I had links links 'admin/build/generate' during the update - the router path assigned is 'admin/build'. This is very BAD news.

chx’s picture

Status: Needs work » Needs review
FileSize
12.57 KB

pwolanin asked me how D5 determines callback. Right. This solves many of the problems and it's not a fragile solution because the menu callbacks array is the same for every user so we can pick any item from the cache menu. I now remove duplicates and uncustomized items.

menu_rebuild is not costly enough to make hacking it worthwhile.

pwolanin’s picture

no time to look now- but does this work for non-cached menu items: e.g. node links?

chx’s picture

FileSize
14.59 KB

Nope, it won't . Seems there is no other way than keeping a flag for updated elements and setting their router_path on navigation links rebuild. As we do not keep items that are not customized, this won't be a horror because generated links are not kept.

chx’s picture

Please note that it is indeed a must to recheck router path on every rebuild because if you have a link pointing to foo/bar/baz and foo and foo/bar are defined in different modules (user/* paths are like that for example) then switching on foo would set the router_path to 'foo' which is wrong. Wrong, but not insecure -- router_path is used only for access checking the menu links, so you do not gain access to anything just because router_path is mis-set.

chx’s picture

Let me sum up the problems.

The smallest is that when you add the custom menus, their parent, admin/build/menu needs to be already defined, so we need to do a menu_rebuild before doing the actual update.

The current system is designed so that when a link is about to be displayed in the navigation block, we do an access check. The same access check that would run when you click on the link. In order to do this we need to have the relevant router entry for the menu link. This is very good for security -- one access check to rule them all (including menu overview). However, during upgrade the router entry is missing because you need to disable non-core modules to upgrade. Contributed modules might assume that Drupal database is like it would be if you'd installed it fresh, so you can't keep 'em on because it's possible that Drupal won't even boot.

Therefore, we need to do something with links which has missing router paths. The solution is that we store an empty router path for these entries, deny access for entries with an empty router_path (external items are not a problem as they are not access checked) and keep a flag on them forever which indicates they come from Drupal 5. Whenever menu_rebuild is run, like on module enable, we try to refresh the router path for these entries. So for example if you had an entry pointing to node/1234 then on the first run it'll get the node/% router_path. However, there is no way to predict whether you have a module waiting to be enabled which defines a special page for node/1234 so we need to recheck the router path again and again. As we only keep customized links from Drupal 5 -- there is no need to keep the generated ones -- this is not likely to be a problem, as it's unlikely that humans enter too many entries. The documentation might mention running a simple SQL UPDATE to remove this flag if your site is done, to speed up your menu_rebuild calls but it's not a crucial thing to do.

This means that if you have just updated your site it might seem like that some links are missing because the menu overview screen also uses the same access check. We might add some specific access tricks for this place. Or we can display a message "Your customized D5 item links pointing to paths which do not exist might not show up here, in this case you just need to re-enable modules and they will appear".

Another problem are duplicates -- unlike the old system, now you can have more than one link to the same path and if that happens to be on the same level with the same title, users will find it strange. For example you get a lower case entry 'administer' besides 'Administer' from Drupal 6 system_menu. We specifically check for these entries and skip them.

chx’s picture

FileSize
14.5 KB

Minor fixes: compared to earlier, minor speedup when resetting router_paths on navigation rebuild, do not delete uncustomized links with children, and skip entirely saving of duplicates.

webchick’s picture

This seems to have addressed all of the critical data loss/duplication bugs, except for the conf call # => call menu item in the Secrets menu.

We still have this business, too:

     * new book page
     * new gig sheet
     * new bookmark
-    * My blog
-    * Create content
     * My account
+    * Create content
+    * Recent posts
     * News aggregator
     * Administer
     * Log out

And this business:

...
Administer
    * Content management
    * Site building
          o Blocks
          o Menus
                = Free Tags (overview)
                = Secrets (overview)
          o Modules
...
webchick’s picture

Oh, also there's a:

notice: Undefined variable: items_to_delete in /Users/webchick/Sites/dailyreport/modules/system/system.install on line 3444.

during update.php, but I alerted chx to this, and I think he fixed it already. :)

webchick’s picture

FileSize
15.61 KB

This includes the fix for the notice, and the conf call # link is back. This was caused by pointing the menu item at an alias rather than something like node/123. One-line fix.

chx says that the admin > build > menu path bugs are a separate issue, and should be covered in a different patch.

The weighting issues are not worth holding this patch up.

We'll want more testing on this by more people before rolling a beta, but to my estimation it solves all of the critical issues originally defined. Therefore, I'm tentatively marking this RTBC. The sooner this is committed the sooner people who don't know how to patch can start testing our 6.x-dev tarball, and then we should start getting some really useful feedback.

webchick’s picture

Status: Needs review » Reviewed & tested by the community
chx’s picture

FileSize
14.55 KB

The only difference here is that we now skip pid 0 entries with a path. these are weird entries from Drupal 4.7 which do not appear in Drupal 5 anyways. Thanks for the report webernet.

chx’s picture

FileSize
14.77 KB

Extra safeguards against infinite loops. Again, webernet has a very confused menu table -- we found an entry where the pid points to an entry which does not exist. We skip those entries.

webernet’s picture

FileSize
15.49 KB

The patch in #70 successfully updated the (apparently) "buggy" menu table I've been testing with.

Minor reroll to add a missing period to a comment.

pwolanin’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
16.15 KB

chatting with webernet on IRC - I just realized this update is missing one important component: menu module's blocks.

The deltas are changing from the mid to the menu_name, so we need to update the {blocks} and {block_roles} tables too.

This little patch is also needed to see all the blocks: http://drupal.org/node/171019

There is also a preference for menu OTF that has a name change - we need to update that and maybe also think about whether the current D6 behavior is right or a regression, and whether we should delete this and other menu module variables that are no longer used?

I've tested this once - but more thorough/edge testing needed.

pwolanin’s picture

FileSize
16.24 KB

existing patch doesn't create links for the sytem-defined menus (navigation, etc). Here's a new version that uses menu_enable() to do it.

pwolanin’s picture

FileSize
16.86 KB

Webernet noted that user-defined titles for the primary and secondary links menus are not saved.

Here's a go at fixing that too. Can D5 have descriptions for menus?

chx’s picture

FileSize
7.58 KB

No description for menus in D5.

chx’s picture

FileSize
17 KB

I would call this a cleanup version. There is no menu description so I removed that. There is no menu item with a mid of 0 so I stripped the check from save user-defined titles, the SQL will come back empty, and two selects really does not matter with this update. I now record bogus menu entries and cleanse the table afterwards -- much easier than trying to not save these which would confuse the carefully built tree in session. Running menu_enable is a brilliant stroke, thanks.

pwolanin’s picture

Status: Needs review » Needs work

almost brilliant - menu_enable should be called with module_invoke() (just in case). Also, menu_enable needs to be re-written since it assumes that we always need to save new links when the module is enabled (this patch means that assumption is not true).

pwolanin’s picture

Also, the discussion on the devel list about block deltas just pointed out another problem - we're using the menu name as the delta:

the 1ist is 64 chars

'menu_name'    => array('type' => 'varchar', 'length' => 64)

the latter, 32 chars:

'delta'      => array('type' => 'varchar', 'length' => 32, 'not null' => TRUE, 'default' => '0'),

So, should we limit menu_name to 32 chars? We need to check the length also when doing the update, since we could get duplicate names by mistake (the update code appends an integer to the end, but that may not help for long names).

chx’s picture

Status: Needs work » Needs review
FileSize
17.05 KB

No need to touch menu_enable. I presume that if you have entries in menu then menu module is enabled. If it's not, then the module_invoke will protect from a fatal error.

chx’s picture

FileSize
17.38 KB

I now cut the menu name to 20 chars. As this is not really a user facing value -- we have title for that -- it'll be fine. If you have more than 10^12 menus with the almost same name, or even, any name in Drupal 5, then you can write your update as you surely are running your own fork as D5 dies around 10^5 menu items anyways.

pwolanin’s picture

Status: Needs review » Needs work

Look at the current code, it's no longer right: http://api.drupal.org/api/function/menu_enable/6

It assumes the admin links will be in 'navigation', and always saves the links (this will cause duplicate links if the module is disabled and re-enabled)

menu_enable needs to look something like this:


function menu_enable() {
  menu_rebuild();
  $result = db_query("SELECT * FROM {menu_custom}");
  $link['module'] = 'menu';
  list($link['plid'], $link['menu_name']) = db_fetch_array(db_query("SELECT mlid, menu_name from {menu_links} WHERE link_path = 'admin/build/menu' AND module = 'system'"));
  $link['router_path'] = 'admin/build/menu-customize/%';

  while ($menu = db_fetch_array($result)) {
    $link['mlid'] = NULL;
    $link['link_title'] = $menu['title'];
    $link['link_path'] = 'admin/build/menu-customize/'. $menu['menu_name'];
    if (!db_result(db_query("SELECT mlid FROM {menu_links} WHERE link_path = '%s' AND plid = %d", $link['link_path'], $link['plid']))) {
      menu_link_save($link);
    }
  }
  menu_cache_clear_all();
}

Also, the {menu_links} and {menu_custom} schemas need to change the length of menu_name to 32 chars.

chx’s picture

Status: Needs work » Needs review
FileSize
23.65 KB

All done. The stop condition was wrong. osinet created a 4.7 install, added custom stuff, upped to 5.0 and then 6.0 , now that works as well.

pwolanin’s picture

function system_update_6020 and function system_update_6021 also need to have the tables definitions for menu_name revised.

webernet’s picture

FileSize
25.54 KB

Minor changes:
- Puts case 'mysql': and case 'mysqli': on seperate lines
- Changes length to 32 for menu_name

pwolanin’s picture

FileSize
22.23 KB

the whitespace changes in system.admin.inc probably should not be part of this patch.

Also, changed menu_enable() to module_invoke('menu', 'enable') to guard against menu module being off.

is this supposed to return a fractional value? If so, one of the 2 values needs to be cast to float. If not, it's pointless, I think.

$ret['#finished'] = $_SESSION['system_update_6021'] / $_SESSION['system_update_6021_max'];
chx’s picture

Status: Needs review » Reviewed & tested by the community

I intended to that module invoke stuff. There is no problem with the divison. If neither of you (webernet, pwolanin) can find significant problems with the patch then it's ready, finally. It's high time.

patrickfgoddard’s picture

Hi chx,

Last night via IRC, I promised I'd test this patch on a site I have with custom menus, but honestly, I have no idea what I'm doing. I'm confused about this thread and which file I'm actually supposed to run. (not the first? sysup.patch?)

If you still need help, I'm happy to stumble through. Maybe someone could PM me and walk me through the process.

chx’s picture

to test, make a copy of your database , apply the latest patch and try running update.php against that copy. check whether your custom menus and items are in place.

patrickfgoddard’s picture

FileSize
3.13 KB

Results from running: patch -p0 < ../patches/menu_update_27.patch attached.
Got to a point where it asked me to "file to patch" and I was lost. Like I said before, I have no idea what I'm doing (don't worry, though, test db, test files... ;) ), but I'm more than happy to help out where I can.

patching file includes/install.inc
Hunk #1 succeeded at 360 (offset 6 lines).
patching file includes/menu.inc
Hunk #1 FAILED at 363.
Hunk #2 FAILED at 1457.
Hunk #3 FAILED at 1547.
Hunk #4 FAILED at 1580.
Hunk #5 FAILED at 1620.
Hunk #6 succeeded at 1359 with fuzz 2 (offset -297 lines).
5 out of 6 hunks FAILED -- saving rejects to file includes/menu.inc.rej
patching file modules/menu/menu.module
Hunk #1 FAILED at 130.
Hunk #2 FAILED at 462.
Hunk #3 FAILED at 562.
3 out of 3 hunks FAILED -- saving rejects to file modules/menu/menu.module.rej
can't find file to patch at input line 215
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: modules/menu/menu.schema
|===================================================================
|RCS file: /cvs/drupal/drupal/modules/menu/menu.schema,v
|retrieving revision 1.3
|diff -u -p -r1.3 menu.schema
|--- modules/menu/menu.schema 27 May 2007 20:31:13 -0000 1.3
|+++ modules/menu/menu.schema 28 Aug 2007 15:25:24 -0000
--------------------------
File to patch: <-- my tiny brain = lost.

pwolanin’s picture

patch applies cleanly for me.

@thund3rbox - you must not have done a cvs update, or if you're not using cvs, download the very latest tarball: http://drupal.org/node/97368

ChrisKennedy’s picture

Status: Reviewed & tested by the community » Needs review

Patch applies fine for me.

I'm wondering if Dries will be cool with the preg_match in drupal_uninstall_module(). You could just as easily check if the part begins with % ($part[0] == '%') and not mess with regex. This will automatically add support for numbers and uppercase letters too (not necessary, but a bonus imo - although I suppose the rest of the menu subsystem probably doesn't support this). Yes, it will catch entries that were already '%', but this doesn't seem like a problem.

Also, in the db_query() of that function, does MSSQL/Oracle/DB2 compatibility necessitate pulling the external = 0 constant out via a %d? That's the impression I get from hswong3i's post at http://edin.no-ip.com/html/?q=sql_naming_conventions but maybe he only means when inserting or updating?
* Same for the first modified query in _menu_navigation_links_rebuild() in menu.inc - ml.updated = 1.
* Same for the first modified query in menu_enable() in menu.module - WHERE link_path = 'admin/build/menu' AND module = 'system'.
* Same for db_query_range() in system.install.
* Some for the query with has_children = 0 AND customized = 0.

If we're set on making a maximum menu name length of 27 characters, we might as well make a constant for it from the beginning and not have to worry about updating the code and translations if we want to change the length. And should that be drupal_strlen() in menu_edit_menu_validate()? Just checking. The translation on that line should also be changed from "can not" to "cannot" imo.

menu.schema - why is the db length set to 32 even thought UI maxlength is 27? Plus if it's a varchar isn't 255 better since it's all the same for the db anyway? I've read the comments in the issue but they don't explain the reasoning.

system.install - can't we use db_add_field() rather than the separate mysql & pgsql code? If not, "default" and "unsigned" should be in caps (for mysql/i). The secondary links description needs a period at the end.
* Minor: extra space before the comma at preg_replace('/[^a-zA-Z0-9]/' , '-',
* $item['menu_name'] = substr($item['menu_name'], 0, 20); - should that 20 be a 27 (or 32... or a constant)?

chx’s picture

Assigned: chx » Unassigned

If people prefer nitpicking then I am done with this issue. I am seriously sick with it anyways. (btw it's 27 vs 32 because of menu- prefix)

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
20.94 KB

Of course the patch applies cleanly, the kid above tried it with DRUPAL-5, no wonder it failed.

"You could just as easily check if the part begins with % ($part[0] == '%') and not mess with regex. This will automatically add support for numbers and uppercase letters too (not necessary, but a bonus imo - although I suppose the rest of the menu subsystem probably doesn't support this)."

Then what's the point of your comment? Indeed menu system does not support those as a wildcard as it'd be have been apparent if you'd taken the time to search into menu.inc (there are two preg_matches altogether) instead of needlessly bashing this poor patch.

"Also, in the db_query() of that function, does MSSQL/Oracle/DB2 compatibility "

You are kidding me, aren't you? Let's worry about that in Drupal 7.

"menu.schema - why is the db length set to 32 even thought UI maxlength is 27? "

Answered above.

system.install - can't we use db_add_field()

Aye we can. At least something useful comes out of this.

* Minor: extra space before the comma at preg_replace('/[^a-zA-Z0-9]/' , '-',

I am so happy that people have time to hunt these. Care to extensively test this !@#$%^&*( beast instead?

* $item['menu_name'] = substr($item['menu_name'], 0, 20); - should that 20 be a 27 (or 32... or a constant)?

No it should not and it's covered above why.

ChrisKennedy’s picture

I had an hour to spare between eating dinner and going to bed. I'll use it for something else in the future.

pwolanin’s picture

@Chris - I somewhat blindly copied the preg_match from elsewhere in the menu code. You're right that we could (for this) us something simpler - even strpos. That uninstall bit in generally could probably use refinement but it's pretty tangential to the thrust of the patch.

chx’s picture

Sorry but this issue kills me. I really spent more time than I ever wanted and I am sorry if I offended you -- it's nothing personal it's more at the whole community -- why noone properly reviews this?

webernet’s picture

Tested the latest patch - everything is still working properly.

Gábor Hojtsy’s picture

- Please put this to phpdoc comments, there are no comments on the function, but this looks like a function comment actually:

+function _menu_find_router_path($menu, $link_path) {
+ // Find the router path which will serve this path.

- Please introduce a constant for the 27 magic number, ie. the max menu name length. Use it in both the code and in hlep text, like t("... %number of chars", array('%number' => ...)).

- Please also document the 32 limit on the menu name in the schema

- typo: Migrate the menu items from the old menu ystem to the new menu_links table.

- it would be great to document what isset($_SESSION['system_update_6021']) is used for (it is visible from the patch, but not by looking at either the menu code or the update code)

These does not seem like showstoppers :)

chx’s picture

FileSize
21.84 KB

Addressed Gabor's concerns.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

OK, this issue seems like well tested and even well documented now, so thanks for the hard work. Committed!

drewish’s picture

Status: Fixed » Needs review
FileSize
560 bytes

notice: Use of undefined constant MENU_MAX_MENU_NAME_LENGTH_UI - assumed 'MENU_MAX_MENU_NAME_LENGTH_UI' in drupalhead\modules\menu\menu.module on line 13.

Added in the single quotes.

ps I apologize for the patch that's not relative to the root. Just updated to the new version of TortoiseCVS and it ignores the paths. I haven't had a chance to figure out how to fix it.

bdragon’s picture

FileSize
648 bytes

IIRC there shouldn't be a space either.

webernet’s picture

Status: Needs review » Reviewed & tested by the community

Tested the patch in #102 - it fixes the warning.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed!

Anonymous’s picture

Status: Fixed » Closed (fixed)