The pattern that is used to split the query for injecting added where, join clause returns 'null' if the main query does not include a where clause.

This results in generating a wrong SQL query. the code from version 5 works properly but this has been changed in version 6.

(line 318 database.inc)

CommentFileSizeAuthor
#5 nowhere.patch1.95 KBchx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cburschka’s picture

Would the following have the desired effect?

@@ -325,7 +325,7 @@ function db_rewrite_sql($query, $primary
         |
           \( (?P>anonymous_view) \)          # an open parenthesis, more "anonymous view" and finally a close parenthesis.
         )*
-      )[^()]+WHERE)
+      )[^()]+(WHERE)?)
     }x';
     preg_match($pattern, $query, $matches);
     if ($where) {
madli@radio.astral.com’s picture

No, it doesn't help!

for testing:

- the main query:
SELECT n.*, u.name FROM core_node n INNER JOIN core_users u ON n.uid = u.uid ORDER BY n.changed DESC LIMIT 0, 50

- $join and $where are retun values of a module implemented hook_db_rewrite_sql

the original code in drupal 6.0 creates the following wrong query:

$join WHERE $where AND ( SELECT n.*, u.name FROM core_node n INNER JOIN core_users u ON n.uid = u.uid )ORDER BY n.changed DESC LIMIT 0, 50

and your suggestion

SELECT n.*, u.name FROM core_node n INNER JOIN core_users u ON n.uid = u.uid ORDER BY n.changed $join WHERE $where AND ( ) LIMIT 0, 50

madli@radio.astral.com’s picture

I made the following changes to fix the issue and it works (the only problem is that it wouldn't work properly if the main query includes subqueries )

Index: database.inc

===================================================================

--- database.inc	(revision 68)

+++ database.inc	(working copy)

@@ -315,45 +315,32 @@

   }
 
   if (!empty($where) || !empty($join)) {
-    $pattern = '{
-      # Beginning of the string
-      ^
-      ((?P<anonymous_view>
-        # Everything within this set of parentheses is named "anonymous view"
-        (?:
-          [^()]++                   # anything not parentheses
-        |
-          \( (?P>anonymous_view) \)          # an open parenthesis, more "anonymous view" and finally a close parenthesis.
-        )*
-      )[^()]+WHERE)
-    }x';
-    preg_match($pattern, $query, $matches);
-    if ($where) {
-      $n = strlen($matches[1]);
-      $second_part = substr($query, $n);
-      $first_part = substr($matches[1], 0, $n - 5) ." $join WHERE $where AND ( ";
-      // PHP 4 does not support strrpos for strings. We emulate it.
-      $haystack_reverse = strrev($second_part);
-      // No need to use strrev on the needle, we supply GROUP, ORDER, LIMIT
-      // reversed.
-      foreach (array('PUORG', 'REDRO', 'TIMIL') as $needle_reverse) {
-        $pos = strpos($haystack_reverse, $needle_reverse);
-        if ($pos !== FALSE) {
-          // All needles are five characters long.
-          $pos += 5;
-          break;
-        }
+    if (!empty($where)) {
+      $new = " WHERE ($where) ";
+    }
+    $new = " $join $new ";
+
+    $insertPos = strpos($query, 'WHERE');
+    if ($insertPos) {
+      $query = substr($query, 0, $insertPos) . $new .' AND ' . substr($query, $insertPos+5);
+    }
+    else {
+      $insert = $new;
+
+      $insertPos = strrpos($query, 'LIMIT');
+      if ( strpos($query, 'ORDER') ) {
+        $insertPos = strrpos($query, 'ORDER');
       }
-      if ($pos === FALSE) {
-        $query = $first_part . $second_part .')';
+      if ( strpos($query, 'GROUP') ) {
+        $insertPos = strrpos($query, 'GROUP');
       }
+      if ( $insertPos > 0 ) {
+        $query = substr($query, 0, $insertPos) . $new . substr($query, $insertPos);
+      }
       else {
-        $query = $first_part . substr($second_part, 0, -$pos) .')'. substr($second_part, -$pos);
+        $query = $query . $new;
       }
     }
-    else {
-      $query = $matches[1] ." $join ". substr($query, strlen($matches[1]));
-    }
   }
 
   return $query;
chx’s picture

Status: Active » Closed (won't fix)

Add WHERE 1 = 1. This code is fragile enough that I simply urge core developers not to touch it. Let's do something better in D7.

chx’s picture

Status: Closed (won't fix) » Needs review
FileSize
1.95 KB

Goba does not like me.

cburschka’s picture

marcingy’s picture

That seems to work and also works when no grants are set on a node.

This patch should be set as RTBC

Gábor Hojtsy’s picture

Version: 6.0-rc3 » 7.x-dev
Status: Needs review » Reviewed & tested by the community

Yes, I also run this through some manual calls on queries with and without WHERE, with and without subqueries and all run fine, so it looks fine. Committed to 6.x. RTBC for 7.x.

chx’s picture

Status: Reviewed & tested by the community » Closed (won't fix)

Just let db_rewrite_sql die. It ran its course, let the new database layer take over.

cburschka’s picture

Version: 7.x-dev » 6.x-dev
Status: Closed (won't fix) » Fixed

This was already fixed in D6, however. Let's just leave it there.

(Okay, I just like it when bugs are set to Fixed instead of Won't Fix. =) )

Gábor Hojtsy’s picture

Version: 6.x-dev » 7.x-dev
Status: Fixed » Reviewed & tested by the community

Guys, let's allow 7.x to work as 6.x until this new database layer is introduced. Do not introduce regressions knowingly IMHO.

vladimir.dolgopolov’s picture

Here is the test for the issue for Simpletest module.
Patch #5 pass the test.


class TestCase219380 extends DrupalTestCase {
  function get_info() {
    return array(
      'name' => t('[219380] db_rewrite_sql does NOT work if $query doesn\'t include "WHERE"'),
      'desc' => t('The pattern that is used to split the query for injecting added where, join clause returns \'null\' if the main query does not include a where clause.'),
      'group' => t('Drupal 7 Tests'),
    );
  }
  
  // The test create a simple module with hook_db_rewrite_sql().
  // Then it checks a SQL query transformation without WHERE clause and with WHERE clause.
  // The former fails, the second pass for now.
  function testIssue() {

    $table_name = $this->randomName();
    $module_name = $this->randomName();

    $module_condition = "$table_name.field_module = 'module'";
    $original_condition = "$table_name.original_field = 'original'";

    $module_text = <<< EOF
<?php
function {$module_name}_db_rewrite_sql(\$query, \$primary_table, \$primary_field, \$args) {
  \$array = array();
  if (\$primary_table == '$table_name') {
    \$array['where'] = "$module_condition";
  }
  return \$array;
}
EOF;

    $this->createModule($module_name, $module_text);

    $query_without_where = "SELECT field FROM {$table_name}";
    $query_with_where = "$query_without_where WHERE $original_condition";

    // replace 'space' with '\s' and brackets (db_rewrite_sql() can add some extra space into a query)
    $expected_regexp_query_without_where = str_replace(' ', '[\s\(\)]*?', "$query_without_where WHERE $module_condition");
    @$result_without_where = db_rewrite_sql($query_without_where, $table_name); // @ avoids 'Undefined offset' notice
    $this->assertWantedPattern("|$expected_regexp_query_without_where|", $result_without_where, 'Check db_rewrite_sql without "where"');

    $expected_regexp_query_with_where = str_replace(' ', '[\s\(\)]*?', "$query_without_where WHERE ( $module_condition AND $original_condition )");
    $result_with_where = db_rewrite_sql($query_with_where, $table_name);
    $this->assertWantedPattern("|$expected_regexp_query_with_where|", $result_with_where, 'Check db_rewrite_sql with "where"');

  	$this->deleteModule($module_name);

  }

  function createModule($module_name, $module_text, $module_core = '7.x') {
    $module_dir = "sites/all/modules/$module_name";
    $module_file = "$module_dir/$module_name.module";
    $module_file_info = "$module_dir/$module_name.info";
    if (file_check_directory($module_dir, FILE_CREATE_DIRECTORY)) {
      $info = <<< EOF
name = "$module_name module"
description = "$module_name module for test. Delete it when you see it."
package = Temporary
core = $module_core

EOF;
      if ($fp = @fopen($module_file_info, 'w')) {
        fwrite($fp, $info);
        fclose($fp);
      }
      if ($fp = @fopen($module_file, 'w')) {
        fwrite($fp, $module_text);
        fclose($fp);
      }
      $this->drupalModuleEnable($module_name);
    }
  }

  function deleteModule($module_name) {
    $module_dir = "sites/all/modules/$module_name";
    $module_file = "$module_dir/$module_name.module";
    $module_file_info = "$module_dir/$module_name.info";
    @unlink($module_file);
    @unlink($module_file_info);
    @rmdir($module_dir);
  }

}

Gábor Hojtsy’s picture

Test looks good, thanks for doing it! (Dries might hold off waiting for PDO, so we will see whether this patch will need to be committed.). Keep this RTBC unless it becomes irrelevant.

Dries’s picture

I think we might want to revisit this after the PDO / database abstraction patch landed. In fact, I'd encourage you to participate in that issue: see http://drupal.org/node/225450.

catch’s picture

Status: Reviewed & tested by the community » Needs work

No longer applies, almost a dup/fixed by #225450: Database Layer: The Next Generation but let's kill it once that's in.

Robin Monks’s picture

catch: it's in :)

Subscribing

Robin

Crell’s picture

Version: 7.x-dev » 6.x-dev

I'm assigning this back to 6.x, as db_rewrite_sql() will be removed from D7 shortly so there's no point in trying to make it work there. Whether or not this is worth looking into further for D6 I leave up to Gabor.

catch’s picture

Status: Needs work » Fixed

It's already done in 6, so marking fixed.

Damien Tournoud’s picture

Great. I enjoy already that we have a plan to kill db_rewrite_sql() :)

catch’s picture

For those following along at home, that plan is here: #299176: Replace db_rewrite_sql() with hook_query_alter().

Anonymous’s picture

Status: Fixed » Closed (fixed)

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