Fix Issue with system query in php8

[seamuslee001]

We (CiviCRM) have been testing out php8 on our CI infrastructure and using a development version of drush8 and have found that we get the following error when trying to do a drush user-create command

PDOException: SQLSTATE[HY093]: Invalid parameter number: number of bound variables does not match number of tokens: SELECT name FROM {system} WHERE type='module' AND status=1; Array
(
    [:type] => module
    [:status] => 1
)
 in drush_db_select() (line 131 of phar:///home/jenkins/bknix-edge/bin/drush8/includes/dbtng.inc).

This resolves it by avoiding the use of parameters as they get replaced https://github.com/drush-ops/drush/blob/8.x/includes/dbtng.inc#L111 but then still passed to db_query https://github.com/drush-ops/drush/blob/8.x/includes/dbtng.inc#L131 and it seemed easier to patch the backdrop integration than drush

ping @herbdool @totten

[herbdool]

I'm not clear on why this bug is showing up just with php8, or why it seems like it's just a BD issue.

So the parameters are passed even after the tokens have been replaced, after which there are no more tokens in the string? I'm trying to understand this better. That seems like a potentially broad issue, so I'm curious as to if it's also shown up for Drupal sites.

[seamuslee001]

@herbdool I too am a bit confused I don't know if I am missing something here but maybe something with the changes brought across from D7 is triggering something. In regards to why not for other drupal sites well it is only d6 and backdrop that get into this part of the dbtng https://github.com/drush-ops/drush/blob/8.x/includes/dbtng.inc#L105 as the If on L81 there will capture D7,D8,D9 and they don't trigger the _drush_replace_query_placeholders function

[totten]

Invalid parameter number: number of bound variables does not match number of tokens: SELECT name FROM {system} WHERE type='module' AND status=1; Array
(
    [:type] => module
    [:status] => 1
)

Just verbalizing the error message for clarity - the SQL expression doesn't have any variables/tokens. The error seems to be saying: "You asked me to interpolate some variables, but they've already been interpolated!" That is a smelly circumstance - I could imagine that php8's PDO might be stricter than php7's PDO?

it is only d6 and backdrop that get into this part of the dbtng

Is that just happenstance of the numbering (ie Backdrop 1.x is numerically less than Drupal 7.x)? Or is there a real difference in the API support?

It feels like maybe the branch/API-detection should have a way to say: "Yes, this first code-branch works fine on Backdrop"?

This resolves it by avoiding the use of parameters as they get replaced https://github.com/drush-ops/drush/blob/8.x/includes/dbtng.inc#L111 but then still passed to db_query https://github.com/drush-ops/drush/blob/8.x/includes/dbtng.inc#L131

So if L111 has already interpolated $args, then L131 doesn't need $args? You could omit $args and make parameter-count match up?

-    return db_query($query, $args);
+    return db_query($query);

and it seemed easier to patch the backdrop integration than drush

Right, but we don't know where it ends, do we? It looks like this is structural problem in drush's query function that would affect other use-cases?

[totten]

So if L111 has already interpolated $args, then L131 doesn't need $args? You could omit $args and make parameter-count match up?

Ah, hrm... if you have any variables in other parts of the query, then L111 won't address those. Maybe it should be more like this?

L111
- $where = _drush_replace_query_placeholders($where, $args);

L131
- db_query($query, $args);
+ db_query(_drush_replace_query_placeholders($query, $args));

Or... if db_query() is adequate, then maybe you just remove L111?

[quicksketch]

With only two calls to drush_db_select() in this extension and my inability to find the root of this problem, I'm fine with merging this as-is to keep the drush extension hobbling along until Drush 8 EOL. Thanks @seamuslee001, @totten, and @herbdool. My apologies for the long delay on this.