The attached patch does fix a few minor bugs in DBTNG..

  1.    public function having($snippet, $args = array()) {
-    $this->query->where($snippet, $args);
+    $this->query->having($snippet, $args);
     return $this;
   }

    Obvious, I'd say :)

  2. -    foreach ($count->tables as $alias => &$table) {
+    $tables = $count->getTables();
+    foreach ($tables as $alias => &$table) {
       unset($table['all_fields']);
     }

    SelectQueryExtender should not access tables directly but instead use getTables() instead. getTables() is by reference...

  3. -      if ($table instanceof SelectQuery) {
+      if ($table instanceof SelectQueryInterface) {

    Allows to use extended query objects as subqueries...

CommentFileSizeAuthor
#9 dbtng_fixes4.patch3.74 KBBerdir
#7 dbtng_fixes3.patch3.74 KBBerdir
#4 dbtng_fixes2.patch3.27 KBBerdir
dbtng_fixes.patch1.95 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Note that the missing & #2 exists in the patch...

Dries’s picture

Dries CreditAttribution: Dries commented

Good catches. Could you extend the DBTNG tests to provide coverage for the relevant bugs?

Crell’s picture

Crell CreditAttribution: Crell commented

Thanks, Berdir!

As for tests, that's a wee bit more complicated since there are so many tests. Ideally we'd refactor the tests to take an arbitrary select object, which we'd then subclass from and pass an arbitrary select object to. That is, we'd have a SelectParent test case, which is abstract and holds all the tests, and then a child class BasicSelect that just runs a normal select query through it, a PagerSelect child class that just runs a pager query through it plus some extra pager-specific tests, etc. I considered doing that for the original extenders patch but it was too much work to try and roll into the single patch at that point. I still think it's a good idea, but I'm not certain if this is the best place to do so.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
dbtng_fixes2.patch3.27 KB

Added a crazy query test to validate #1 and #3. I know there was a reason I changed #2 but I can't remember what it was exactly. But it should be changed anyway, the tables should not be accessed directly.

If you remove either #1 or #3 from patch, the test fails with PDO exceptions.

Dries’s picture

Dries CreditAttribution: Dries commented

Let's see what Crell has to say about the tests.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Needs work

This should really be 2 tests, one to test that having/count works properly and one to test that we can use an extended query as a subquery. We'll hold off on revamping the whole testing system. :-) Let's do that and then get this committed so that we unblock other issues.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Title: DBTNG bugfxes » DBTNG bugfixes
Status: Needs work » Needs review
FileSize
dbtng_fixes3.patch3.74 KB

Re-roll with tests split up.

chx’s picture

chx CreditAttribution: chx commented

I have a rather "serious" problem here -- trivial to fix of course: the & should stick to the variable name not the =. If this is not in the code style guide then it should be.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
dbtng_fixes4.patch3.74 KB

Oh, I actually checked how it should be but then it's wrong in the same method...

$ grep -R " =& " includes/ modules/ | grep -v CVS 
includes/database/select.inc:   * $fields =& $query->getFields();
includes/database/select.inc:   * $fields =& $query->getExpressions();
includes/database/select.inc:   * $fields =& $query->getOrderBy();
includes/database/select.inc:   * $fields =& $query->getTables();
includes/database/select.inc:    $fields =& $count->getFields();
includes/database/select.inc:    $expressions =& $count->getExpressions();
includes/database/select.inc:    $tables =& $count->getTables();
includes/database/select.inc:    $fields =& $count->getFields();
includes/database/select.inc:    $expressions =& $count->getExpressions();
includes/form.inc:      if ($batch =& batch_get() && !isset($batch['current_set'])) {
includes/form.inc:      if ($type == 'submit' && ($batch =& batch_get()) && !isset($batch['current_set'])) {
includes/form.inc:    $batch =& batch_get();
includes/form.inc:  $batch =& batch_get();
modules/simpletest/tests/database_test.module:    $conditions =& $query->conditions();
modules/simpletest/tests/database_test.module:    $fields =& $query->getFields();
modules/simpletest/tests/database_test.module:    $expressions =& $query->getExpressions();
modules/node/node.module:    $tables =& $query->getTables();

Changed it in this patch... we should change the other occurences in another issue.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

Agreed with chx. I've fixed the code style issue and committed this patch to CVS HEAD. Thanks all.

Crell’s picture

Crell CreditAttribution: Crell commented

I actually disagree with chx, and find =& to be much easier to read.

Status: Fixed » Closed (fixed)

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

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

I'm pretty sure the testInnerPagerQuery test is not testing anything useful. At least it is not testing what it pretends to test, because the "a pager query with inner pager query returns valid results" feature is broken. See #753084: Extenders and subqueries don't play nice.