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

Files: 
CommentFileSizeAuthor
#20 1889328-20-query-options.patch6.97 KBgielfeldt
PASSED: [[SimpleTest]]: [MySQL] 53,319 pass(es).
[ View ]
#15 1889328-15-query-options.patch6.75 KBgumanist
PASSED: [[SimpleTest]]: [MySQL] 53,062 pass(es).
[ View ]
#13 1889328-13-query-options.patch6.75 KBgumanist
PASSED: [[SimpleTest]]: [MySQL] 52,990 pass(es).
[ View ]
#8 1889328-8-query-options.patch4.53 KBgielfeldt
PASSED: [[SimpleTest]]: [MySQL] 50,290 pass(es).
[ View ]
#4 1889328-1-query-options.patch1.82 KBgielfeldt
PASSED: [[SimpleTest]]: [MySQL] 40,380 pass(es).
[ View ]
#1 1889328-1-query-options.patch1.68 KBgielfeldt
FAILED: [[SimpleTest]]: [MySQL] 39,910 pass(es), 1 fail(s), and 21 exception(s).
[ View ]

Comments

StatusFileSize
new1.68 KB
FAILED: [[SimpleTest]]: [MySQL] 39,910 pass(es), 1 fail(s), and 21 exception(s).
[ View ]

Status:Active» Needs review

Status:Needs review» Needs work

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

StatusFileSize
new1.82 KB
PASSED: [[SimpleTest]]: [MySQL] 40,380 pass(es).
[ View ]

Resubmitting ...

Issue summary:View changes

Fixed example code

Status:Needs work» Needs review

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

Yes, tests passed!

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

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?

Version:7.x-dev» 8.x-dev
StatusFileSize
new4.53 KB
PASSED: [[SimpleTest]]: [MySQL] 50,290 pass(es).
[ View ]

And here's the D8 patch.

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

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.

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

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 :(

Status:Needs work» Needs review
StatusFileSize
new6.75 KB
PASSED: [[SimpleTest]]: [MySQL] 52,990 pass(es).
[ View ]

Added updated patch with tests

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

StatusFileSize
new6.75 KB
PASSED: [[SimpleTest]]: [MySQL] 53,062 pass(es).
[ View ]

Thank for your comments.

Status:Needs review» Reviewed & tested by the community

Looks good to go!

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.

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?

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.

Status:Needs work» Needs review
StatusFileSize
new6.97 KB
PASSED: [[SimpleTest]]: [MySQL] 53,319 pass(es).
[ View ]

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" :-)

Status:Needs review» Reviewed & tested by the community

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

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

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.

Status:Patch (to be ported)» Needs review

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

Issue summary:View changes

Updated text

Issue summary:View changes

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

Status:Needs review» Patch (to be ported)

coreyp_1: Someone needs to backport it.