[DBTNG] revamp menu.inc with DBTNG syntax

Subscribing...

And the test server doesn't like this.

From visual inspection:

Do we really have to duplicate the array in the query? It looks like the array that is already built is what should be passed into db_update() and db_insert() anyway.

The menu rebuild db_insert() is exactly the sort of place we want to be using a multi-insert statement. That should give us a nice performance bump.

Let's go ahead and do a full menu.inc DBTNG conversion as long as we're here.

The fact that you can 'feel' the difference is interesting and most promising.

We should make Simpletest report its total running time in the test summary. We'd be able to quantify what you feel and we'd be able to figure out who is using the fastest machine ... I created an issue for this: #329407: Make simpletest report running time

I did some tests and I couldn't measure a significant performance impact when running the tests. Without this patch, running all tests took 21 minutes 18 seconds. With your patch it took 21 minutes 30 seconds.

I agree with sticking to db_insert()/db_update() here. In fact I'd like to go as far as eliminating drupal_write_record() entirely if possible. However, can we just unset the unneeded keys from the $items array and then pass it to insert?

Also, the fields() definition on the insert should just put one field per line. It's more lines that way, but is visually cleaner and more consistent with the rest of core, I think. (Dries & Angie can overrule me if they want there. I can see the argument to keep it as is.)

If this patch is not complete yet, please mark it 'CNW'. Thanks, and keep up the good work, hswong3i.

At first glance this looks good. Would be great to get Crell's approval. :-)

Probably mis-marked as CNW by the recent bot bug.

You can replace (twice) :

+        for ($i = 1; $i <= 9; $i++) {
+          $query->orderBy('p' . $i, 'ASC');
+        }

with :

+        for ($i = 1; $i <= MENU_MAX_DEPTH; $i++) {
+          $query->orderBy('p' . $i, 'ASC');
+        }

Wrong patch :-(

When I apply this patch, I get a WSOD (fatal error).

After another review, I decided to commit this patch. If necessary, we can follow-up with new patches. Thanks for your hard work hswong3i, and thanks to the many reviewers along the way. Committed to CVS HEAD.

I've got the following error now trying to install sqlite HEAD:

PDOException: UPDATE {menu_links} SET menu_name=:db_update_placeholder_0, plid=:db_update_placeholder_1, link_path=:db_update_placeholder_2, router_path=:db_update_placeholder_3, hidden=:db_update_placeholder_4, external=:db_update_placeholder_5, has_children=:db_update_placeholder_6, expanded=:db_update_placeholder_7, weight=:db_update_placeholder_8, depth=:db_update_placeholder_9, p1=:db_update_placeholder_10, p2=:db_update_placeholder_11, p3=:db_update_placeholder_12, p4=:db_update_placeholder_13, p5=:db_update_placeholder_14, p6=:db_update_placeholder_15, p7=:db_update_placeholder_16, p8=:db_update_placeholder_17, p9=:db_update_placeholder_18, module=:db_update_placeholder_19, link_title=:db_update_placeholder_20, options=:db_update_placeholder_21, customized=:db_update_placeholder_22 WHERE (mlid = :db_condition_placeholder_142) AND ( (menu_name <> :db_condition_placeholder_119) OR (menu_name IS NULL) OR (plid <> :db_condition_placeholder_120) OR (plid IS NULL) OR (link_path <> :db_condition_placeholder_121) OR (link_path IS NULL) OR (router_path <> :db_condition_placeholder_122) OR (router_path IS NULL) OR (hidden <> :db_condition_placeholder_123) OR (hidden IS NULL) OR (external <> :db_condition_placeholder_124) OR (external IS NULL) OR (has_children <> :db_condition_placeholder_125) OR (has_children IS NULL) OR (expanded <> :db_condition_placeholder_126) OR (expanded IS NULL) OR (weight <> :db_condition_placeholder_127) OR (weight IS NULL) OR (depth <> :db_condition_placeholder_128) OR (depth IS NULL) OR (p1 <> :db_condition_placeholder_129) OR (p1 IS NULL) OR (p2 <> :db_condition_placeholder_130) OR (p2 IS NULL) OR (p3 <> :db_condition_placeholder_131) OR (p3 IS NULL) OR (p4 <> :db_condition_placeholder_132) OR (p4 IS NULL) OR (p5 <> :db_condition_placeholder_133) OR (p5 IS NULL) OR (p6 <> :db_condition_placeholder_134) OR (p6 IS NULL) OR (p7 <> :db_condition_placeholder_135) OR (p7 IS NULL) OR (p8 <> :db_condition_placeholder_136) OR (p8 IS NULL) OR (p9 <> :db_condition_placeholder_137) OR (p9 IS NULL) OR (module <> :db_condition_placeholder_138) OR (module IS NULL) OR (link_title <> :db_condition_placeholder_139) OR (link_title IS NULL) OR (options <> :db_condition_placeholder_140) OR (options IS NULL) OR (customized <> :db_condition_placeholder_141) OR (customized IS NULL) ) - Array ( [:db_update_placeholder_0] => navigation [:db_update_placeholder_1] => 0 [:db_update_placeholder_2] => batch [:db_update_placeholder_3] => batch [:db_update_placeholder_4] => -1 [:db_update_placeholder_5] => 0 [:db_update_placeholder_6] => 0 [:db_update_placeholder_7] => 0 [:db_update_placeholder_8] => 0 [:db_update_placeholder_9] => 1 [:db_update_placeholder_10] => 1 [:db_update_placeholder_11] => 0 [:db_update_placeholder_12] => 0 [:db_update_placeholder_13] => 0 [:db_update_placeholder_14] => 0 [:db_update_placeholder_15] => 0 [:db_update_placeholder_16] => 0 [:db_update_placeholder_17] => 0 [:db_update_placeholder_18] => 0 [:db_update_placeholder_19] => system [:db_update_placeholder_20] => [:db_update_placeholder_21] => a:0:{} [:db_update_placeholder_22] => 0 [:db_condition_placeholder_142] => 1 [:db_condition_placeholder_119] => navigation [:db_condition_placeholder_120] => 0 [:db_condition_placeholder_121] => batch [:db_condition_placeholder_122] => batch [:db_condition_placeholder_123] => -1 [:db_condition_placeholder_124] => 0 [:db_condition_placeholder_125] => 0 [:db_condition_placeholder_126] => 0 [:db_condition_placeholder_127] => 0 [:db_condition_placeholder_128] => 1 [:db_condition_placeholder_129] => 1 [:db_condition_placeholder_130] => 0 [:db_condition_placeholder_131] => 0 [:db_condition_placeholder_132] => 0 [:db_condition_placeholder_133] => 0 [:db_condition_placeholder_134] => 0 [:db_condition_placeholder_135] => 0 [:db_condition_placeholder_136] => 0 [:db_condition_placeholder_137] => 0 [:db_condition_placeholder_138] => system [:db_condition_placeholder_139] => [:db_condition_placeholder_140] => a:0:{} [:db_condition_placeholder_141] => 0 ) SQLSTATE[HY000]: General error: 25 bind or column index out of range in menu_link_save() (line 2168 of /home/davereid/Projects/drupal-head/includes/menu.inc).

Rolling back this patch allows HEAD to be installed on sqlite. Marking as critical since this patch broke HEAD on sqlite.

Followup issue for broken sqlite HEAD in #342725: Broken HEAD - Super long update queries.

Tagging.