this patch change 2 parts:

  1. write all field name instead of '*'
  2. use NOT NULL instead of '!='

the original query is functioning within mysql, but not oracle friendly. on the other hand, it change the query into a formal format, which also increase cross DB compatibility.

CommentFileSizeAuthor
#15 drupal-6.x-dev-menu_sql_friendly-3.0.diff1.22 KBhswong3i
#13 drupal-6.x-dev-menu_sql_friendly-2.0.diff1.13 KBhswong3i
#10 rollback_tree_data_sql_1.patch1.07 KBpwolanin
#7 rollback_1.patch803 byteschx
drupal-6.x-dev-menu_sql_friendly-1.0.diff1.48 KBhswong3i

Comments

dries’s picture

dries commented

Committed to CVS HEAD. Thanks.

dries’s picture

dries commented
Status: Needs review » Fixed
Frando’s picture

Frando commented
Priority: Normal » Critical
Status: Fixed » Reviewed & tested by the community

This needs to be rolled back. It breaks HEAD - it causes an infinite loop in menu.inc line 965. (debug credits go to chx).

eaton’s picture

eaton commented
Priority: Critical » Normal
Status: Reviewed & tested by the community » Active

use NOT NULL instead of '!='

This completely broke head and made it impossible to load any pages. The difference between '' and NULL is non-trivial in this case. if it's changed the change will have to include the logic in menu.inc that handles building paths, etc.

eaton’s picture

eaton commented
Priority: Normal » Critical
Frando’s picture

Frando commented
Title: update menu.inc for SQL friendly » HEAD broken (was: update menu.inc for SQL friendly)
Status: Active » Reviewed & tested by the community

Marking this RTBC so that the commiters see it. The original patch should simply be rolled back. We can think about an actual solution for this when HEAD is working again. Thanks.

chx’s picture

chx commented
StatusFileSize
new rollback_1.patch803 bytes

Only one hunk needs to be rolled back, here is the rollback patch.

Frando’s picture

Frando commented
Title: HEAD broken (was: update menu.inc for SQL friendly) » update menu.inc for SQL friendly
Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs work

This has been rolled back. Marking as CNW as the original issue remains unsolved.

pwolanin’s picture

pwolanin commented
Priority: Normal » Critical

One of the queries in menu.inc is still broken in menu_tree_all_data(), includes/menu.inc line 640

I included a fix as part of the patch here: http://drupal.org/files/issues/book_to_outline_23.patch

or I can roll a separate patch.

pwolanin’s picture

pwolanin commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new rollback_tree_data_sql_1.patch1.07 KB

separate menu.inc patch. Just reverts the other part - that function is used heavily by the patched book.module and was broken too.

mike2854’s picture

mike2854 commented

the way for MySQL treating IS NOT NULL differs from Oracle's
They give different results.

no records are resulted in Oracle
as null comparison always give FALSE in Oracle.

Could we have != ' ' instead of != ''
I found the result from MySQL and Oracle are same

Or any other database give different result?

hswong3i’s picture

hswong3i commented

my suggestion is: handle such different within oracle driver, but not changing SQL query in modules. this is because:

dc:/home/hswong3i/project/drupal/HEAD# find drupal-6.x-dev -type f | xargs fgrep -nH db_query | grep "!= ''"                
drupal-6.x-dev/modules/node/node.module:1600:  $count = db_result(db_query("SELECT COUNT(*) FROM {node} n WHERE language != ''"));
drupal-6.x-dev/modules/system/system.install:1073:  $result = db_query("SELECT nid, log FROM {book} WHERE log != ''");
drupal-6.x-dev/modules/path/path.module:314:  $count = db_result(db_query("SELECT COUNT(*) FROM {url_alias} WHERE language != ''"));
drupal-6.x-dev/includes/locale.inc:482:  $translations = db_query("SELECT COUNT(*) AS translation, t.language, s.textgroup FROM {locales_source} s LEFT JOIN {locales_target} t ON s.lid = t.lid WHERE translation != '' GROUP BY textgroup, language");
drupal-6.x-dev/includes/menu.inc:928:    $result = db_query("SELECT * FROM {menu_router} WHERE tab_root = '%s' AND tab_parent != '' ORDER BY weight, title", $router_item['tab_root']);

as you can see this is not the only problem of menu.inc, but quite normal among other DB... this is oracle only problem (http://www.techonthenet.com/oracle/isnotnull.php):

In other languages, a not null value is found using the != null syntax. However in PLSQL to check if a value is not null, you must use the "IS NOT NULL" syntax.

we may take some preg_replace, change != '' into IS NOT NULL... this may need to preform before mapping any %s/%d, etc. as db_query() become private function right now, we may handle this within it... not the best solution, but at least logically correct ;(

any other better suggestion?

hswong3i’s picture

hswong3i commented
StatusFileSize
new drupal-6.x-dev-menu_sql_friendly-2.0.diff1.13 KB

this v2.0 patch will handle the 1st invaild SQL query in line 637 of menu.inc. v1.0 face error just because the system.schema change the struct of table menu_links: rename from href to link_path.

i build the patch from CVS HEAD on last night, and test the patch with mysql version, and passed. patch is ready to recommit ;-)

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed. Thanks.

hswong3i’s picture

hswong3i commented
Status: Fixed » Reviewed & tested by the community
StatusFileSize
new drupal-6.x-dev-menu_sql_friendly-3.0.diff1.22 KB

mike just find another SQL which contain the similar problem, under current CVS HEAD. here is the patch.

mike test it with oracle; and i test it with mysql, which directly run menu_tree_page_data() and display correct result ;D

moshe weitzman’s picture

moshe weitzman commented

looks fine to me.

hswong3i’s picture

hswong3i commented

this patch is now depended on oracle driver version 5.3 for drupal-6.x-dev (please refer to #113), and should be committed before that ;D

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed. Thanks.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)