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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drumm’s picture

Status: Active » Needs work
Issue tags: +Needs tests, +drupal.org D7, +porting
FileSize
1.18 KB

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

BTMash’s picture

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.

drumm’s picture

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.

BTMash’s picture

Status: Needs work » Needs review
FileSize
1.91 KB
1.07 KB

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.

drumm’s picture

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.

chx’s picture

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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

David_Rothstein’s picture

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?

David_Rothstein’s picture

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.

drumm’s picture

FileSize
620 bytes

Attached is a patch just adding isset().

drumm’s picture

FileSize
1.23 KB

Attached is a patch which avoids the menu API.

Status: Needs review » Needs work

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

drumm’s picture

Status: Needs work » Needs review
FileSize
1.29 KB

Updated patch avoiding the menu API.

Status: Needs review » Needs work

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

drumm’s picture

FileSize
1.29 KB

Fixed typo.

drumm’s picture

Status: Needs work » Needs review
chx’s picture

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.

David_Rothstein’s picture

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

David_Rothstein’s picture

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?

BTMash’s picture

Status: Needs work » Needs review
FileSize
1.14 KB

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.

drumm’s picture

FileSize
1.06 KB

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.

drumm’s picture

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

drnikki’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

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

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

drumm’s picture

Status: Needs work » Needs review
FileSize
2.22 KB

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

drumm’s picture

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

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

webchick’s picture

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.