db_rewrite_sql does NOT work if $query doesn't include "WHERE"

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) {

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.

Goba does not like me.

Heads up on a conflict with #170656: db_rewrite_sql() breaks with GROUP_CONCAT().

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

This patch should be set as RTBC

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.

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

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. =) )

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

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);
  }

}

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.

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.

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

catch: it's in :)

Subscribing

Robin

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.

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

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

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