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.

Files: 
CommentFileSizeAuthor
#25 1613554.patch2.22 KBdrumm
PASSED: [[SimpleTest]]: [MySQL] 39,299 pass(es).
[ View ]
#21 1613554.patch1.06 KBdrumm
PASSED: [[SimpleTest]]: [MySQL] 39,346 pass(es).
[ View ]
#20 1613554.patch1.14 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 39,334 pass(es).
[ View ]
#15 1613554.patch1.29 KBdrumm
PASSED: [[SimpleTest]]: [MySQL] 39,261 pass(es).
[ View ]
#13 1613554.patch1.29 KBdrumm
FAILED: [[SimpleTest]]: [MySQL] 39,087 pass(es), 157 fail(s), and 50 exception(s).
[ View ]
#11 1613554.patch1.23 KBdrumm
FAILED: [[SimpleTest]]: [MySQL] 39,247 pass(es), 53 fail(s), and 50 exception(s).
[ View ]
#10 1613554.patch620 bytesdrumm
PASSED: [[SimpleTest]]: [MySQL] 39,251 pass(es).
[ View ]
#4 1613554.testonly.patch1.07 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] 39,102 pass(es), 53 fail(s), and 24 exception(s).
[ View ]
#4 1613554.combined.patch1.91 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 39,101 pass(es).
[ View ]
#1 1613554.patch1.18 KBdrumm
PASSED: [[SimpleTest]]: [MySQL] 39,101 pass(es).
[ View ]

Comments

Status:Active» Needs work
Issue tags:+Needs tests, +Drupal.org D7, +porting
StatusFileSize
new1.18 KB
PASSED: [[SimpleTest]]: [MySQL] 39,101 pass(es).
[ View ]

Here is a patch. Anyone want to write a test?

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.

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.

Status:Needs work» Needs review
StatusFileSize
new1.91 KB
PASSED: [[SimpleTest]]: [MySQL] 39,101 pass(es).
[ View ]
new1.07 KB
FAILED: [[SimpleTest]]: [MySQL] 39,102 pass(es), 53 fail(s), and 24 exception(s).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

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.

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.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 7.x. Thanks!

Status:Fixed» Needs review

+    $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?

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.

StatusFileSize
new620 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,251 pass(es).
[ View ]

Attached is a patch just adding isset().

StatusFileSize
new1.23 KB
FAILED: [[SimpleTest]]: [MySQL] 39,247 pass(es), 53 fail(s), and 50 exception(s).
[ View ]

Attached is a patch which avoids the menu API.

Status:Needs review» Needs work

The last submitted patch, 1613554.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.29 KB
FAILED: [[SimpleTest]]: [MySQL] 39,087 pass(es), 157 fail(s), and 50 exception(s).
[ View ]

Updated patch avoiding the menu API.

Status:Needs review» Needs work

The last submitted patch, 1613554.patch, failed testing.

StatusFileSize
new1.29 KB
PASSED: [[SimpleTest]]: [MySQL] 39,261 pass(es).
[ View ]

Fixed typo.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs tests

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.

Status:Reviewed & tested by the community» Fixed

Committed to 7.x - thanks for working on the followup! http://drupalcode.org/project/drupal.git/commit/99b2514

Status:Fixed» Needs work

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?

Status:Needs work» Needs review
StatusFileSize
new1.14 KB
PASSED: [[SimpleTest]]: [MySQL] 39,334 pass(es).
[ View ]

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.

StatusFileSize
new1.06 KB
PASSED: [[SimpleTest]]: [MySQL] 39,346 pass(es).
[ View ]

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.

The patch in #21 should be ignored. I changed my thinking on it. #20 is okay.

Status:Needs review» Reviewed & tested by the community

Secondary review. Looks fine. I agree with #20.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

Talked to chx, he pointed out we need a test for this. :\

Status:Needs work» Needs review
StatusFileSize
new2.22 KB
PASSED: [[SimpleTest]]: [MySQL] 39,299 pass(es).
[ View ]

Re-roll with BTMash's test that had been committed from #4.

Status:Needs review» Reviewed & tested by the community

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".

Status:Reviewed & tested by the community» Fixed

Awesome, thank you!

Committed and pushed to 7.x.

Automatically closed -- issue fixed for 2 weeks with no activity.