Merge and Count do not respect query option "throw_exception"

MergeQuery::execute()
SelectQuery::countQuery()

The following code does not work as excpected:

<?php
   $options['throw_exception'] = FALSE;
   $query = db_merge('test', $options)
              ->key(array('some_field_that_doesnt_exist' => 1))
              ->fields(array(
                'name' => 'test' . rand(1,5)
              ))
              ->execute();
?>
<?php
   $options['throw_exception'] = FALSE;
   $query = db_select('some_table_that_doesnt_exist', 't', $options)
             ->fields('t')
             ->countQuery()
             ->execute();
 ?>

Both of the queries above will produce an exception, even though they shouldn't according to the API.

patch coming up ...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gielfeldt’s picture

gielfeldt’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1889328-1-query-options.patch, failed testing.

gielfeldt’s picture

Resubmitting ...

gielfeldt’s picture

Issue summary: View changes

Fixed example code

gielfeldt’s picture

Status: Needs work » Needs review

#1: 1889328-1-query-options.patch queued for re-testing.

gielfeldt’s picture

Yes, tests passed!

Is "needs review" the proper status for this ticket?

mcrittenden’s picture

Yes, needs review is the proper status. Can someone smarter than me say whether this is alright to add to a D7 release or if there's a possibility for breakage in contrib here?

gielfeldt’s picture

Version: 7.x-dev » 8.x-dev
FileSize
4.53 KB

And here's the D8 patch.

gumanist’s picture

Status: Needs review » Needs work

It would be great to have a tests on this.
I guess it makes sense to replace:
+ if (!isset($this->queryOptions['throw_exception']) || $this->queryOptions['throw_exception']) {
with
+ if (empty($this->queryOptions['throw_exception'])) {

gielfeldt’s picture

Those two expressions are not similar. Undefined equals true, since this function is not called with the default options where throw_exception is true by default.

gielfeldt’s picture

Btw, I agree on the tests part ... not sure if I'm up to it though :-). Any takers?

gumanist’s picture

Those two expressions are not similar. Undefined equals true, since this function is not called with the default options where throw_exception is true by default.

Oups, sorry, missreaded :(

gumanist’s picture

Status: Needs work » Needs review
FileSize
6.75 KB

Added updated patch with tests

andypost’s picture

Looks awesome except few nitpicks

+++ b/core/lib/Drupal/Core/Database/Query/Merge.phpundefined
@@ -403,42 +403,53 @@ public function __toString() {
+        return NULL;
+      }    ¶

trailing white-space

+++ b/core/modules/system/lib/Drupal/system/Tests/Database/SelectTest.phpundefined
@@ -376,4 +376,39 @@ function testSelectDuplicateAlias() {
+      // This query should die because table doesn't exists,

should use dot not comma

gumanist’s picture

Thank for your comments.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to go!

Crell’s picture

Status: Reviewed & tested by the community » Needs work

Almost, but not quite. A few things need work still:

+++ b/core/lib/Drupal/Core/Database/Query/Merge.php
@@ -403,42 +403,53 @@ public function __toString() {
+      // If throw_exception is not set at all, we assume that exceptions are wanted.
+      if (!isset($this->queryOptions['throw_exception']) || $this->queryOptions['throw_exception']) {
+        throw $e;

This is a poor way of handling missing values. Instead, default it to TRUE in the constructor of the class with a += on the array. See Connection::query() for how it's done there.

+++ b/core/modules/system/lib/Drupal/system/Tests/Database/SelectTest.php
@@ -376,4 +376,39 @@ function testSelectDuplicateAlias() {
+      // This query should die because table doesn't exists,
+      // but without exception, because of 'throw_exception' option.
+      $options['throw_exception'] = FALSE;

The grammar in this comment is quite awkward. Try:

"This query will fail. Normally it would throw an exception but we are supressing it with the throw_exception option."

I think we'd need a test to ensure that exceptions are still thrown if throw_exception is true.

gielfeldt’s picture

This is a poor way of handling missing values. Instead, default it to TRUE in the constructor of the class with a += on the array. See Connection::query() for how it's done there.

Not that I don't agree, but the !isset() approach seemed the more viable option at the time, since adding the default values would also populate $options['return'], which seemed to mess things up a bit. I didn't look any further into it. Any takers?

Crell’s picture

You don't need to reuse Connection->getDefaults() necessarily. But in general when you have a value that may or may not exist and has a clear default behavior when it does not, it's better to normalize it to that value first before you use it so that the later code that uses it is more readable. That could be a ?: operation, or an array += or whatever else makes sense in context.

gielfeldt’s picture

Status: Needs work » Needs review
FileSize
6.97 KB

I added a += array() "sanitizer" in the beginning of execute() and fixed some of the comments as you described. I did not add explicit tests for throw_exception = TRUE though, if that was what you meant. There is a test for when throw_exception is not set, which should be equal to throw_exception = TRUE ... of course the operative word being "should" :-)

Crell’s picture

Status: Needs review » Reviewed & tested by the community

"Should" is such a fun word in technology... :-)

xjm’s picture

#20: 1889328-20-query-options.patch queued for re-testing.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 8.x, thanks!

Looks like this should be backported.

DrColossos’s picture

Status: Patch (to be ported) » Needs review

#4: 1889328-1-query-options.patch queued for re-testing.

DrColossos’s picture

Issue summary: View changes

Updated text

coreyp_1’s picture

Issue summary: View changes

#4 does not apply to D7. What is the status of this issue?

Crell’s picture

Status: Needs review » Patch (to be ported)

coreyp_1: Someone needs to backport it.

gielfeldt’s picture

Status: Patch (to be ported) » Needs review
FileSize
6.5 KB

Backported!

Crell’s picture

+++ b/includes/database/query.inc
@@ -1606,43 +1606,59 @@ class MergeQuery extends Query implements QueryConditionInterface {
+    catch (Exception $e) {
+      // If throw_exception is not set at all, we assume that exceptions are
+      // wanted.
+      if ($this->queryOptions['throw_exception']) {
+        throw $e;
       }

I'm pretty sure throw_exception cannot be not-set by this point, as a default value is provided higher in the method. Even then, the comment is backward from what the code would actually do.

gielfeldt’s picture

Yeah. The comment addresses the null/true duality which was fixed. I'll remove the comment.

  • catch committed ee96cf6 on 8.3.x
    Issue #1889328 by gielfeldt, gumanist: Fixed Not all objects respect the...

  • catch committed ee96cf6 on 8.3.x
    Issue #1889328 by gielfeldt, gumanist: Fixed Not all objects respect the...

  • catch committed ee96cf6 on 8.4.x
    Issue #1889328 by gielfeldt, gumanist: Fixed Not all objects respect the...

  • catch committed ee96cf6 on 8.4.x
    Issue #1889328 by gielfeldt, gumanist: Fixed Not all objects respect the...
dsutter’s picture

RTBC+ patch #30