Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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 ...
Comments
Comment #1
gielfeldt CreditAttribution: gielfeldt commentedComment #2
gielfeldt CreditAttribution: gielfeldt commentedComment #4
gielfeldt CreditAttribution: gielfeldt commentedResubmitting ...
Comment #4.0
gielfeldt CreditAttribution: gielfeldt commentedFixed example code
Comment #5
gielfeldt CreditAttribution: gielfeldt commented#1: 1889328-1-query-options.patch queued for re-testing.
Comment #6
gielfeldt CreditAttribution: gielfeldt commentedYes, tests passed!
Is "needs review" the proper status for this ticket?
Comment #7
mcrittenden CreditAttribution: mcrittenden commentedYes, 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?
Comment #8
gielfeldt CreditAttribution: gielfeldt commentedAnd here's the D8 patch.
Comment #9
gumanist CreditAttribution: gumanist commentedIt 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'])) {
Comment #10
gielfeldt CreditAttribution: gielfeldt commentedThose 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.
Comment #11
gielfeldt CreditAttribution: gielfeldt commentedBtw, I agree on the tests part ... not sure if I'm up to it though :-). Any takers?
Comment #12
gumanist CreditAttribution: gumanist commentedOups, sorry, missreaded :(
Comment #13
gumanist CreditAttribution: gumanist commentedAdded updated patch with tests
Comment #14
andypostLooks awesome except few nitpicks
trailing white-space
should use dot not comma
Comment #15
gumanist CreditAttribution: gumanist commentedThank for your comments.
Comment #16
andypostLooks good to go!
Comment #17
Crell CreditAttribution: Crell commentedAlmost, but not quite. A few things need work still:
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.
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.
Comment #18
gielfeldt CreditAttribution: gielfeldt commentedNot 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?
Comment #19
Crell CreditAttribution: Crell commentedYou 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.
Comment #20
gielfeldt CreditAttribution: gielfeldt commentedI 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" :-)
Comment #21
Crell CreditAttribution: Crell commented"Should" is such a fun word in technology... :-)
Comment #22
xjm#20: 1889328-20-query-options.patch queued for re-testing.
Comment #23
catchCommitted/pushed to 8.x, thanks!
Looks like this should be backported.
Comment #24
DrColossos CreditAttribution: DrColossos commented#4: 1889328-1-query-options.patch queued for re-testing.
Comment #24.0
DrColossos CreditAttribution: DrColossos commentedUpdated text
Comment #25
coreyp_1 CreditAttribution: coreyp_1 commented#4 does not apply to D7. What is the status of this issue?
Comment #26
Crell CreditAttribution: Crell commentedcoreyp_1: Someone needs to backport it.
Comment #27
gielfeldt CreditAttribution: gielfeldt commentedBackported!
Comment #28
Crell CreditAttribution: Crell commentedI'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.
Comment #29
gielfeldt CreditAttribution: gielfeldt commentedYeah. The comment addresses the null/true duality which was fixed. I'll remove the comment.
Comment #30
gielfeldt CreditAttribution: gielfeldt commentedRemoved bad comment.
Comment #35
dsutter CreditAttribution: dsutter as a volunteer commentedRTBC+ patch #30