Not a bug Bug in SqlConstraint.php - Only takes first value from possible values.

[Rasmus Vind]

Affected version

2.1.7

I hope I am not wasting anyone's time by reporting a bug in 2.1.7. It's niche enough that I expect it still exists in the latest version.
I was passing an array of values to a SqlConstraint and the result was unexpected.

Here's my example:

$query->withSql(new SqlConstraint(
    'thread.discussion_state IN (%s)',
    ['visible', 'deleted'],
    $this->getThreadQueryTableReference()
));

The result is unexpectedly this:

thread.discussion_state = 'visible'

It only showed visible threads.

Turns out that vsprintf takes the first value from an array of arguments, not all of them. In other words, it expects a string, not an array. In order to fix this problem. You have to remember that $db->quote() supports arrays and returns comma-delimted quoted strings for use with the query construct IN(). But if there is only one, there will be no commas and existing queries will continue to work as usual.

To apply the fix, edit this file:

XF\Search\Query\SqlConstriant::getSql(\XF\Db\AbstractAdapter $db)
-- a/src/XF/Search/Query/SqlConstraint.php
+++ b/src/XF/Search/Query/SqlConstraint.php
@@ -53,7 +53,7 @@ class SqlConstraint
                {
                        return vsprintf(
                                $this->condition,
-                               array_map([$db, 'quote'], $this->values)
+                               $db->quote($this->values)
                        );
                }
                else

I mean, you're already doing the work to quote every single value but then proceed to throw away everything after the first one. It feels like a bug.


PS: I only make these bug reports because I love XenForo with a passion.
Ralle

 

[Jeremy P]

I think this is generally expected. The values in the second argument are supposed to correspond to sprintf directives, so you would either use thread.discussion_state IN (%s, %s) or call $db->quote() on the array prior to passing it in.

 

[Rasmus Vind]

Hey Jeremy.

Thanks for the reply.

From what I understand, this design might be made to allow for multiple parameters instead of an array of options, like so:

$query->withSql(new SqlConstraint(
    'thread.discussion_state = %s AND thread.discussion_type = %s',
    [$discussionState, $discussionType],
    $this->getThreadQueryTableReference()
));

Because I think that will work.

To take this one step further, realized that I could do this:

$query->withSql(new SqlConstraint(
    'thread.discussion_state IN (%s)',
    [['visible', 'deleted']],
    $this->getThreadQueryTableReference()
));

And get exactly the behavior I want.