Use db_select() in place of db_query() to work with Oracle DB driver.

Thank you for posting your patch, this will definitely be useful for other Oracle sites.

Please understand that committing this patch would invalidate almost two years of testing that went into ACL for D7, and this is a risk that I cannot take. We have 4500 known sites running ACL 7.x-1.0, and they expect to be able to update to a 1.1 that is just as solid as 1.0. The -dev versions receive very little testing out in the field, unfortunately, and I would not be able to release 1.1 and recommend upgrading to it.

This really needs to be fixed in the Oracle driver. There's no point in trying to "fix" thousands of contrib modules that aren't broken.

I'll leave this active until the Oracle driver is working.

If you (yes, YOU, the reader) try this patch, please leave feedback, so we can build some confidence for everyone in the Oracle boat.

Provided the new db_select() calls generate the same queries as the old db_query() calls, then the functionality is unchanged

We have no way to ascertain this, for all supported databases, all supported PHP versions, all supported versions of all databases, all hardware platforms, etc. Obviously, for Oracle, your patch does not produce the same SQL, which proves this point.

The fact that you have to post a revised patch proves that you're not infallible. I'm not infallible either. Every commit that I make carries a certain risk, and it's my job as maintainer to manage that risk and to weigh it against the benefit of the patch.

What it boils down to is that I need to be convinced that a patch is beneficial overall. With less than a 100 installations of the Oracle driver and maybe one or two wishing to use ACL, the benefit of working around the issues in the dysfunctional Oracle driver is, sorry to say this, very small, compared to the over 4500 installations of ACL 7.x-1.0 who would have to bear the risks of the patch.

What do you hope to gain if I'd commit your patch?

Ok, you've made a good case and I decided to spend my precious spare time to do a first review (see below). This has turned up two fatal errors.

As I wrote above, I can't claim to be infallible either, I may very well miss additional major errors, and I have no process to ensure that this patch will not break something.

We have a few tests, but the coverage is far from complete (see #761696: WANTED: SimpleTests for ACL), unfortunately...
BTW, I had to push an update to the -dev version to enable the tests that we do have, and I moved the acl.test file to the tests subdirectory, which makes your patches inapplicable now.

Here are the review results:

All SelectStatementInterface::condition() calls need to fully qualify the column names, i.e. you must prefix the column names with their table aliases.

+++ b/acl.admin.inc
@@ -12,13 +12,20 @@ function _acl_edit_form($acl_id, $label = NULL, $new_acl = FALSE) {
+    $result = $query->fields('u', array('uid', 'name'))

The -> should go to the next line.

+++ b/acl.module
@@ -248,9 +258,14 @@ function acl_node_access_records($node) {
+    ->addField('n', 'acl_id', 'gid')
+    ->condition('n.nid', $node->nid);

addField() cannot be chained because it returns the field alias.

+++ b/acl.module
@@ -248,9 +258,14 @@ function acl_node_access_records($node) {
+  $query->join('acl', 'a', 'n.acl_id = a.acl_id')
+    ->addField('a', 'module');

join() cannot be chained because it returns the table alias.

+++ b/acl.module
@@ -248,9 +258,14 @@ function acl_node_access_records($node) {
+  $result = $query->execute()->fetchAssoc();

The FETCH_ASSOC should be passed to db_select().

+++ b/acl.module
@@ -334,9 +355,11 @@ function acl_node_access_explain($row) {
+      $query->join('users', 'u', 'au.acl_id = :acl_id AND au.uid = u.uid', array(':acl_id' => $row->gid));

The acl_id condition is not part of the ON clause, it's just a condition. The original code is not exactly clean because it has the ON condition in the WHERE clause. Your version puts it all into the ON clause, which would produce different SQL on all databases. DBTNG forces us to fix this, invalidating the testing.

+++ b/acl.test
@@ -31,11 +31,11 @@ class AclWebTestCase extends DrupalWebTestCase {
+    $records = db_select('acl', 'a')->fields('a', array('acl_id', 'name'))->condition('a.acl_id', $acl_id)->execute()->fetchAll();

The queries in the tests should be line-wrapped, too, to make them readable. I haven't looked at them yet.

I was following the first example at http://drupal.org/node/310075#joins with respect to that query, which seemed to indicate that was permissable. I've moved the acl_id condition off to a conditional.

I'm not sure, maybe there could be a benefit to putting the conditions for the joining table into the ON clause of its JOIN (the example that you cite). But in our case, the additional condition is for the joined table and it seems to me that these should go to the WHERE clause. OTOH, INNER JOIN is symmetric, so there shouldn't really be a difference...

http://dev.mysql.com/doc/refman/5.0/en/join.html says

The conditional_expr used with ON is any conditional expression of the form that can be used in a WHERE clause. Generally, you should use the ON clause for conditions that specify how to join tables, and the WHERE clause to restrict which rows you want in the result set.

That makes sense. A clear separation also helps query altering.

We're down to minor issues now:

+++ b/acl.admin.inc
@@ -146,4 +155,3 @@ function acl_save_form($form, $priority = NULL) {
       ->execute();
   }
 }
-

An empty line after every method (including the last one!) is helpful when you rearrange code. Don't remove it.

+++ b/acl.module
@@ -189,26 +191,34 @@ function acl_get_id_by_number($module, $number) {
-  return $query->execute()->fetchField();
+  return $query
+    ->execute()
+    ->fetchField();

$query->execute()->fetchField() is generally accepted — it occurs 9 times in core. execute() and fetchField() don't add anything to the $query anymore, so they don't need to follow the same rules.

+++ b/acl.module
@@ -227,7 +237,9 @@ function acl_get_ids_by_user($module, $uid, $name = NULL, $number = NULL) {
-  $acl_ids = $query->execute()->fetchCol();
+  $acl_ids = $query
+    ->execute()
+    ->fetchCol();

Same here, leave it as it is.

+++ b/acl.module
@@ -334,9 +360,12 @@ function acl_node_access_explain($row) {
+      $query->fields('u', array('name'))
+        ->condition('au.acl_id', $row->gid);

Break before the ->.

+++ b/acl.module
@@ -345,9 +374,16 @@ function acl_node_access_explain($row) {
+        $query->fields('an', array('acl_id'))
+          ->fields('a', array('module', 'name'))

Same here. The rule as I understand it is that query method calls that add something to a query should always be aligned on a line of their own, except those that break the chain: if they are by themselves, they should be merged with $query on the same line.

+++ b/acl.module
@@ -375,4 +411,3 @@ function _acl_get_explanation($text, $acl_id, $module, $name, $number, $users =
   }
   return $text;
 }
-

Don't.

+++ b/tests/acl.test
@@ -31,11 +31,19 @@ class AclWebTestCase extends DrupalWebTestCase {
+      ->fetchAll();

I'm not sure fetchAll() is valid here. It's not documented.

+++ b/tests/acl.test
@@ -31,11 +31,19 @@ class AclWebTestCase extends DrupalWebTestCase {
+      ->fetchAll();

Same as above.

+++ b/tests/acl.test
@@ -54,20 +62,32 @@ class AclWebTestCase extends DrupalWebTestCase {
+      ->fetchAll();

Same here.

+++ b/tests/acl.test
@@ -54,20 +62,32 @@ class AclWebTestCase extends DrupalWebTestCase {
+      ->fetchAll();

Same here.

+++ b/tests/acl.test
@@ -54,20 +62,32 @@ class AclWebTestCase extends DrupalWebTestCase {
+      ->fetchAll();

Same here.

+++ b/tests/acl.test
@@ -90,26 +110,51 @@ class AclWebTestCase extends DrupalWebTestCase {
-    $count = db_query('SELECT COUNT(nid) FROM {acl_node} WHERE acl_id = :acl_id', array(':acl_id' => $acl_id1))->fetchField();
+    $count = db_select('acl_node', 'n')
+      ->fields('n', array('nid'))
+      ->condition('n.acl_id', $acl_id1)
+      ->countQuery()
+      ->execute()
+      ->fetchField();

This is what we really want to have in all cases above, because they only test the count.

+++ b/tests/acl.test
@@ -90,26 +110,51 @@ class AclWebTestCase extends DrupalWebTestCase {
+      ->fields('n', array('nid'))

For all count queries, as long as we don't do distinct(), we don't need to specify a field. We're really looking for COUNT(*) but COUNT(primary_key) was supposed to be more efficient on MySQL. That's the only reason why it's there, but in DBTNG the MySQL driver should take care of that.

+++ b/acl.admin.inc
@@ -146,4 +155,3 @@ function acl_save_form($form, $priority = NULL) {
-

Your editor must not do that.

+++ b/acl.module
@@ -196,19 +198,25 @@ function acl_get_id_by_number($module, $number) {
+    ->fields('aclu', array('uid'))

Do we really need to specify fields() for the countQuery()s?

Thank you for the patches, brianV. I've rerolled #19 and fixed my comments in #20 as a service to the Oracle users.

However, https://www.drupal.org/node/310075 makes it clear that this is not the way to go. #1067802: Compare $connection->query() and $connection->select() performance shows that db_select() is considerably slower than db_query(). Node Access is a notorious performance killer, and blanket replacement of db_query() with db_select() would cause a performance hit for everyone, for the benefit of only a few Oracle installations.

If anyone wants to take this on, then we need to examine each db_query() that causes problems with Oracle and see how we can fix them. Are these just the ones linked in the OP? We may even have to special-case Oracle, if we don't find a way to support it with db_query().

Be warned though: this will require reviewers/testers on the MySQL side as well!

Tests passed.

I'm setting this to RTBC, but I won't commit it as explained in #23 above.

I would like to commit #2385183: Oracle Support though, if we can get that RTBC.

I am also against that patch replacing db_query with db_select, because of the performance impacts this would cause.

The node access system is already a bottleneck for performance and every performance bit saved in this layer has big effects on the whole.

Drupal 7 has reached its EOL, so I'm closing this as outdated.