Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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... in the "Documentation Team links" menu.
Comment | File | Size | Author |
---|---|---|---|
#25 | 1613554.patch | 2.22 KB | drumm |
#21 | 1613554.patch | 1.06 KB | drumm |
#20 | 1613554.patch | 1.14 KB | BTMash |
#15 | 1613554.patch | 1.29 KB | drumm |
#13 | 1613554.patch | 1.29 KB | drumm |
Comments
Comment #1
drummHere is a patch. Anyone want to write a test?
Comment #2
BTMash CreditAttribution: BTMash commentedHmm, 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.
Comment #3
drummDrupal.org has
But you probably want a simpler example for a test.
Comment #4
BTMash CreditAttribution: BTMash commentedActually, 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.Comment #5
drummGreat. 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.
Comment #6
chx CreditAttribution: chx commentedThis 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.
Comment #7
webchickCommitted and pushed to 7.x. Thanks!
Comment #8
David_Rothstein CreditAttribution: David_Rothstein commentedHm, 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?
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, 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.
Comment #10
drummAttached is a patch just adding isset().
Comment #11
drummAttached is a patch which avoids the menu API.
Comment #13
drummUpdated patch avoiding the menu API.
Comment #15
drummFixed typo.
Comment #16
drummComment #17
chx CreditAttribution: chx commentedDiscussed 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.
Comment #18
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks for working on the followup! http://drupalcode.org/project/drupal.git/commit/99b2514
Comment #19
David_Rothstein CreditAttribution: David_Rothstein commentedSee #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?
Comment #20
BTMash CreditAttribution: BTMash commentedHmm, 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.
Comment #21
drummThe 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.Comment #22
drummThe patch in #21 should be ignored. I changed my thinking on it. #20 is okay.
Comment #23
drnikki CreditAttribution: drnikki commentedSecondary review. Looks fine. I agree with #20.
Comment #24
webchickTalked to chx, he pointed out we need a test for this. :\
Comment #25
drummRe-roll with BTMash's test that had been committed from #4.
Comment #26
drummComment #27
chx CreditAttribution: chx commentedThis 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".
Comment #28
webchickAwesome, thank you!
Committed and pushed to 7.x.