[DBTNG] revamp menu.inc with DBTNG syntax
hswong3i - October 13, 2008 - 08:04
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | menu system |
| Category: | task |
| Priority: | critical |
| Assigned: | hswong3i |
| Status: | closed |
| Issue tags: | DBTNG Conversion |
Description
This patch update menu.inc with part of the TNGDB syntax: using db_insert and db_update. The complicated UPDATE query in _menu_link_move_children() is not included in this patch.
Tested with MySQL and PostgreSQL, pass Menu simpletest.
| Attachment | Size |
|---|---|
| tngdb-menu.inc-1223884710.patch | 7.86 KB |
| Testbed results | ||
|---|---|---|
| tngdb-menu.inc-1223884710.patch | failed | Failed: Failed to apply patch. a href=http://testing.drupal.org/pifr/file/1/tngdb-menu.inc-1223884710.patchDetailed results/a |
#1
Indentation issues following arrays.
#2
@earnie: I think this is our new coding standard? E.g. includes/actions.inc, line 329:
<?phpdb_merge('actions')
->key(array('aid' => $aid))
->fields(array(
'callback' => $function,
'type' => $type,
'parameters' => serialize($params),
'description' => $desc,
))
->execute();
?>
#3
Right, but look at your patch in the browser. Some of the lines after closing )) for the fields(array( are not indented correctly.
#4
@earnie: Yes, get it and fixed :D
#5
Oooh... Some don't need to be too fancy :S
#6
Patch reroll via CVS HEAD.
#7
Subscribing...
#8
And the test server doesn't like this.
#9
Well... Its strange. I fetch code from CVS HEAD, apply patch, run installer with expert mode, activate simpletest, run simpletest for "menu", and all pass and looks good :-S
Anyway, re-submit the patch and hopefully the test server would like it :-)
#10
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.
#11
Hmm... I try to revamp with using
$itemdirectly, but failed. I guess it is because$itemcontain some other required keys. db_insert() and db_update() will not reference to database schema so it will build the RAW SQL with unexpected result. In order to have blind-insert or blind-update, we should use drupal_write_record() instead.BTW, as drupal_write_record() is now implement within common.inc so sometime it may still not yet include during early stage of bootscript. Moreover, drupal_write_record() coming with additional abstraction which must result with performance degrade. If we hope to boost up our libraries as much as possible, using db_insert() and db_update() seems to be a better choice.
P.S. Patch revamp with multi-insert, and I can FEEL the different during simpletest! Amazing! Maybe we can first pass this issue and follow up the others later?
#12
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
#13
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.
#14
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.)
#15
Oh #329407: Make simpletest report running time is a good idea. Feeling is not accurate enough...
I give a try for this approach, but found that lines for explicitly unset unneeded keys is almost equal as that for explicitly use required keys. Moreover, those "unneeded keys" for db_insert() or db_update() maybe required for on going procedure, unset them may trigger other hidden issues. Finally, explicitly define keys seems much accurate and clear for debug and maintenance. (Please correct me if possible.)
Patch reroll based on above indent suggestion. Tested with MySQL simpletest and it looks fine.
#16
Keep on update with DBTNG syntax. Just half-done. Keep as record.
#17
If this patch is not complete yet, please mark it 'CNW'. Thanks, and keep up the good work, hswong3i.
#18
Latest progress. I try my best (simpletest during development for 100+ times...), and most queries are now in DBTNG syntax. Some missing:
while ($item= db_fetch_array($result)) {is not able to revamp asforeach ($result as $item) {style. Once change it error is triggered. I don't know why this happened.db_rewrite_sql().Tested with MySQL + simpletest and pass all menu test. I need some help for the rest of revamp :S
#19
At first glance this looks good. Would be great to get Crell's approval. :-)
#20
I add the following comment for those incomplete, so simplify tracing:
// @todo: Revamp with DBTNG syntax.Also minor fix some other syntax and coding style.
#21
Patch reroll based on code cleanup. Also integrate with progress of #328110: Recoverable fatal error: Argument 3 passed to l() must be an array.
Patch via CVS HEAD, tested with MySQL + menu simpletest. All pass without error message.
#22
According to commit of #328110: Recoverable fatal error: Argument 3 passed to l() must be an array, patch reroll, update, and focus on menu.inc only.
Patch via CVS HEAD, tested with MySQL + menu simpletest. All pass without error message.
#23
The last submitted patch failed testing.
#24
Ooops... "Module list functionality" is failed. It is because passing $item as object into
_menu_delete_item(), but we use it as array. Quick fix with following code snippet:<?php@@ -1852,17 +1968,19 @@ function menu_link_delete($mlid, $path =
* Forces deletion. Internal use only, setting to TRUE is discouraged.
*/
function _menu_delete_item($item, $force = FALSE) {
+ $item = (array) $item;
if ($item && ($item['module'] != 'system' || $item['updated'] || $force)) {
// Children get re-attached to the item's parent.
if ($item['has_children']) {
?>
Also handle some more revamp from
while (...db_fetch_array())toforeach (...).#25
#26
Object to array conversion should happen with http://php.net/get_object_vars instead.
<?phpfunction _menu_delete_item($item, $force = FALSE) {
if (is_object($item)) {
$item = get_object_vars($item);
}
?>
#27
Reference to #302207: DBTNG: book module, also handle db_query_rewrite() and some other minor stuff. Update "object to array" with get_object_vars(). Please refer to diff file for more information.
#28
The last submitted patch failed testing.
#29
Probably mis-marked as CNW by the recent bot bug.
#30
Crell: Not only about bot bug. forum.test coming with illegal (!?) statement, so the test fail. Patch updated, please refer to diff file for more information.
#31
Reroll via CVS HEAD.
#32
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');
+ }
#33
Yes, this looks more elegant :D
#34
Wrong patch :-(
#35
Ooops... How a silly mistake... Patch reroll :(
#36
The last submitted patch failed testing.
#37
I double check with CVS HEAD and my testbed, simpletest should pass. Let's reroll once again.
#38
When I apply this patch, I get a WSOD (fatal error).
#39
It shouldn't be... I test it with PHP5.2 CVS HEAD, and all simpletest pass. Or any Apache error log message as reference?
#40
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.
#41
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).#42
Rolling back this patch allows HEAD to be installed on sqlite. Marking as critical since this patch broke HEAD on sqlite.
#43
Followup issue for broken sqlite HEAD in #342725: Broken HEAD - Super long update queries.
#44
Automatically closed -- issue fixed for two weeks with no activity.
#45
Tagging.