Multiple add_where_expression clauses in hook_views_query_alter use same argument

[yorkshire-pudding]

Description of the bug

I'm working on a module and encountered an odd situation. I'm attempting to create an OR group of clause statements using $query->add_where_expression() within hook_views_query_alter()
The queries are more complex, but for simplicity, the bit that is failing is substituting arguments. Take the following line:

$query->add_where_expression('group_name', "node.type = :type", array(':type' => $type));

Now I loop through each type, but the resulting SQL only has the first $type for every condition even though the $query object after this, viewed through dpm(), looks correct and has the right arguments for each type.

The following statement works, but I thought variables should always be added into SQL using arguments. There is no user input for the source of this variable so no real risk in doing that but still seems the wrong way.

$query->add_where_expression('group_name', "node.type = '$type'");

See also https://www.drupal.org/project/views/issues/2854660

Another drupal issue has a comment with an add_where_expression built with the variables embedded in the statement.

It looks like views_plugin_query_default::build_condition() is where the processing happens and it doesn't look like it is handling arguments per condition.

I can see this not being a problem if the variables are from a safe source, but if they are going to take in user input then this limitation is not ideal. Also, using arguments to include variables in SQL is the recommended way to do it, so it is hard to see that this would be working as designed.
If however, this is working as designed, then it should probably be documented as such as this is an exception from the normal way of building SQL.

NOTE: The example given here is simplistic and is probably feasible without this, but I am doing more complex WHERE statements that need to be there for a module to do its stuff. The simple example is to illustrate the bug without too many steps or dependencies.

Steps To Reproduce

To reproduce the behavior:

1. Create a small custom module that implements hook_views_query_alter()

/**
 * Implements hook_views_query_alter().
 */
function my_module_views_query_alter(&$view, &$query) {
  // If the base table is not a node then no alter is needed.
  if ($query->base_table != 'node') {
    return;
  }

  // Check if the view is filtered by content type.
  if (isset($view->filter['type'])) {
    $filter_content_type = $view->filter['type']->value;
  }
  else {
    $filter_content_type = array_keys(node_type_get_types());
  }
  $type_edit_permissions = array();
  foreach ($filter_content_type as $type) {
    $type_edit_permissions[$type] = (bool) user_role_has_permission('administrator', 'edit any ' . $type . ' content');
  }
  dpm($type_edit_permissions);
  $count_permissions = array_sum($type_edit_permissions);

  $query->set_where_group('OR', 'custom');

  foreach ($type_edit_permissions as $type => $edit) {
    if ($edit) {
      $query->add_where_expression('custom', "node.type = :type", array(':type' => $type));
    }
  }
  dpm($query);
}

2. Install Devel
3. You can adjust what role you do this for it doesn't matter, just ensure the role has some permissions. This is only to provide a simple example for reproducing the issues
4. Create a view of nodes that has either no filter for type or a few types filtered (not just one type).
5. Set view settings to show the SQL generated.
6. Preview the view
7. Check the dpm() objects and the SQL

Actual behavior

While $query object is correct, SQL statement uses the first type for each condition

SELECT `node`.`title` AS `node_title`, `node`.`nid` AS `nid` 
FROM  {node} `node` 
WHERE (( (`node`.`status` = '1') )
AND( (node.type = 'card') OR (node.type = 'card') OR (node.type = 'card') )) 
LIMIT 10 OFFSET 0

Expected behavior

Arguments work on each add_where_expression() function instance independently in the generated SQL statement:

SELECT `node`.`title` AS `node_title`, `node`.`nid` AS `nid` 
FROM  {node} `node` 
WHERE (( (`node`.`status` = '1') )
AND( (node.type = 'card') OR (node.type = 'page') OR (node.type = 'post') )) 
LIMIT 10 OFFSET 0

Additional information

Add any other information that could help, such as:

• Backdrop CMS version: 1.30.1
• Web server and its version: n/a
• PHP version: 8.2
• Database sever (MySQL or MariaDB?) and its version: MYSQL 8.0
• Operating System and its version: linux
• Browser(s) and their versions: n/a

[argiepiano]

Yes, I've seen this behavior also in db_select() built queries. I don't think there is a way to address this, as this is a problem with PHP's PDO placeholder replacement, and placeholders must be unique (you can't use the placeholder with different values in the same query).

Instead, what I do with db_select queries is define a different placeholder for each type, as in
':type_card' => 'card', ':type_page' => 'page',

etc.

See https://stackoverflow.com/questions/42244086/why-pdo-doesnt-allow-multiple-placeholders-with-the-same-name

[indigoxela]

Wow, that looks like one of those views riddles. I tried to dig as deep as I could, but... 🤪
It seems like somewhere between views query building and handing that over to the Backdrop DB abstraction layer, something weird happens to the placeholders.

However, there seems to be an easy fix: make sure you're using different placeholder names in your loop, like just adding numbers like so:

function foobar_views_query_alter(&$view, &$query) {
  if ($query->base_table != 'node') {
    return;
  }

  $filter_content_type = array_keys(node_type_get_types());
  $type_edit_permissions = array();
  foreach ($filter_content_type as $type) {
    $type_edit_permissions[$type] = user_access('edit any ' . $type . ' content');
  }

  $query->set_where_group('AND', 'custom');
  $i = 0;// <-!
  foreach ($type_edit_permissions as $type => $edit) {
    if (!$edit) {
      $placeholder = ":type_$i";// Avoid using the *same* placeholder name.
      $query->add_where_expression('custom', "node.type != $placeholder", array($placeholder => $type));
      $i++;// <-!
    }
  }
}

Yes, I simplified this further, to see some actual result with an unprivileged user. The "magic thing" is $i appended to :type to avoid the placeholder mangling somewhere in the deep, deep sea of abstraction. 😜

And, yes, what @argiepiano suggests would be an alternative (using the name).

[indigoxela]

Hehe, I could have saved some time, if I looked at this issue's progress earlier. 😉 I was just too deep in the layers to check my notification mails. 🤫

[yorkshire-pudding]

I've added a PR to improve the documentation for this function including a link to the relevant PDO documentation page on PHP.net

[argiepiano]

Perhaps this should also be added to add_having_expression()?

[argiepiano]

This would also be true for a lot of other functions outside Views - for example for method join() of SelectQueryInterface and where() of QueryConditionInterface, etc. So, I'm wondering if documenting this is really needed, as this is a known functionality/limitation of PHP's PDO framework? I guess it wouldn't hurt...

[yorkshire-pudding]

It wasn't that well known as I didn't know it and was scratching my head for hours wondering why the function wasn't working.

[argiepiano]

LGTM!

[indigoxela]

Documenting that is a great idea! 👍