Posted by drumm on June 2, 2012 at 2:15am
9 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | menu system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | drumm |
| Status: | closed (fixed) |
| Issue tags: | Drupal.org D7, Needs tests, porting |
Issue Summary
menu_links's options column is a serialized array, sometimes including a query key. In Drupal 6 to 7 it changed from a string to associative array. If the string makes it to url(), it fails being constructed.
Drupal.org triggers this with the "API documentation issues" link to http://drupal.org/project/issues/search/drupal?text=&assigned=&submitted=&participant=&status[]=Open&component[]=documentation&i in the "Documentation Team links" menu.
Comments
#1
Here is a patch. Anyone want to write a test?
#2
Hmm, I'd be happy to help figure out a test for this. I'll check to see what the column looks like in D6 (this'll take some time) so I can replicate the issue for the upgrade test.
#3
Drupal.org has
mysql> SELECT * FROM menu_links WHERE mlid = 19554\G*************************** 1. row ***************************
menu_name: menu-documentation-t
mlid: 19554
plid: 0
link_path: project/issues/search/drupal
router_path: project/issues/search/%
link_title: API documentation issues
options: a:2:{s:5:"query";s:81:"text=&assigned=&submitted=&participant=&status[]=Open&component[]=documentation&i";s:10:"attributes";a:1:{s:5:"title";s:0:"";}}
module: menu
hidden: 0
external: 0
has_children: 0
expanded: 0
weight: -46
depth: 1
customized: 1
p1: 19554
p2: 0
p3: 0
p4: 0
p5: 0
p6: 0
p7: 0
p8: 0
p9: 0
updated: 0
1 row in set (0.00 sec)
But you probably want a simpler example for a test.
#4
Actually, this is great. My test is basically going to have link go to 'node' and the options is
a:3:{s:5:"query";s:14:"page=1&node=10";s:8:"fragment";s:6:"nid-10";s:10:"attributes";a:1:{s:5:"title";s:0:"";}. I tried adding another entry to drupal-6.menu.database.php and the test failed (quite badly) meaning it didn't need any extra work. Tried with your patch and it seems to work well on the menu upgrade test. Attaching test and combined patch.#5
Great. I can confirm this is working well for Drupal.org's 7 staging site. Doesn't need forward porting to 8.x since it is an update that 7 can do.
#6
This is not worth even a reroll cos it's an update function but really it could've been db_query instead of db_select. But again:still commit worthy.
#7
Committed and pushed to 7.x. Thanks!
#8
+ $menu_link = menu_link_load($mlid);+ if (is_string($menu_link['options']['query'])) {
+ $menu_link['options']['query'] = drupal_get_query_array($menu_link['options']['query']);
+ menu_link_save($menu_link);
+ }
Hm, are menu_link_load() and menu_link_save() actually safe to call from an update function? They both are API functions and can invoke hooks, and I thought we don't like doing that while updates are running...
Especially to the extent that this patch is simply fixing incorrect data in the database (rather than actually saving new information), I'd think it would be better to just load and save this via direct database queries, rather than re-saving (and potentially triggering other unexpected changes to) menu links on a Drupal 7 site that may already be live.
Any thoughts on that?
#9
Also, I think we might want an isset() before the is_string() call?
It's not too likely given the surrounding code, but I think we could wind up triggering some PHP notices there otherwise.
#10
Attached is a patch just adding isset().
#11
Attached is a patch which avoids the menu API.
#12
The last submitted patch, 1613554.patch, failed testing.
#13
Updated patch avoiding the menu API.
#14
The last submitted patch, 1613554.patch, failed testing.
#15
Fixed typo.
#16
#17
Discussed the necessity of an _update_7000_menu_link_save with drumm but we have agreed that'd be a massive overkill. So, this is good to go. Tests are already in.
#18
Committed to 7.x - thanks for working on the followup! http://drupalcode.org/project/drupal.git/commit/99b2514
#19
See #1706788: system_update_7074 fails on postgresql - I'm going to be rolling this back (or, well, pseudo-rolling-it-back) today before the Drupal 7.15 release, because apparently it broke PostgreSQL.
The underlying issue here is apparently #1575790: Update #7002 fails on postgres - ILIKE operator on bytea not supported, so it seems like we could just add this back (as a new update function) later once that issue is fixed?
#20
Hmm, one other option I see that we can use is to not use the like operator and use strpos to see if the word 'query' is in the options column after getting all the links?
I'm putting it as 'needs review' for now though waiting on #1575790: Update #7002 fails on postgres - ILIKE operator on bytea not supported to get in #15 may be the best route nonetheless.
#21
The LIKE is there to cut down the work in PHP; skipping records that have no chance of passing the real check,
is_string($menu_link['options']['query']).Drupal.org has 10,440 rows in the table, only 1 of which matches, so this does save a lot of work. #20 is better than nothing, saving the work of unserialize. Of course it would be ideal to just solve ILIKE for Postgres.
We actually don't even need this to be case-insensitive, "
query" is an array key that will always be lowercase.#22
The patch in #21 should be ignored. I changed my thinking on it. #20 is okay.
#23
Secondary review. Looks fine. I agree with #20.
#24
Talked to chx, he pointed out we need a test for this. :\
#25
Re-roll with BTMash's test that had been committed from #4.
#26
#27
This issue is insanely confusing! How can something be committed and then not be in 7.x and why do we want to re-add?
So what happened is that #1706788: system_update_7074 fails on postgresql actually rolled back this issue as committed in #7 (followed up by #18). This rather crucial info is missing. Here's the edited, understandable version of #25: "while we rolled back the fix alongside with the test, the test itself in #4 was good, so adding it back".
#28
Awesome, thank you!
Committed and pushed to 7.x.
#29
Automatically closed -- issue fixed for 2 weeks with no activity.