Make Connection::expandArguments() public to allow array argument expansion outside of Connection::query()

Maybe we could just move the call to expandArguments to within prepareStatement instead of within query. It's protected and it's not called anywhere else within Connection.

Also, make it public looks a viable alternative.

It was made difficult to make a generic query return AFFECTED_ROWS to keep up with https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/lib/Drupal/...

Thanks! That's a great suggestion. If I might add, maybe make it optional? Add an "expand_arguments" option to prepareStatement $options? This way no surprises can happen.

It would look like this.

So back in #2345451: Introduce a Connection::prepareStatement method allowing to pass options to the Statement, deprecate Connection::prepareQuery() and Connection::prepare() → I was already proposing to pass $args to prepareStatement, see my comment at #67. Now it seems we may have a good case to revise the decision to leave it out.

If we pass $args to prepareStatement with a NULL default in the signature, we won’t even need to bother with the $options.

When I tried to code this up and write the comment for this it makes it clear it is a side effect of calling prepareStatement which is usually not desired. Maybe let's get back to making expandArguments public?

Yes, probably #6 is the best option.

Conceptually, one 'prepares a statement' independently from its 'arguments', than 'executes' one or more times the 'prepared argument' with one or more sets of 'arguments' to obtain a 'result' in each execution. So the idea in #2 of mixing the arguments with the statement preparation is a break in the separation of concerns and therefore not a good idea in the first place.

That said, this probably opens up a question of how currently 'reusable' is a statement prepared by query(), that calls expandArguments() on the arguments and modifies them. Trying to re-execute that with a new set of similarly formed arguments should(?) require to run expandArguments() on the additional sets indpendently. But that is probably not a case in core, a test would be needed. That would be a separate issue probably though.

Certainly not the most complex core patch I've ever written.

The last submitted patch, 8: 3349753_8.patch, failed testing. View results →

Unrelated random testing failure.

RTBC for me; what do you think @daffie?

I get that we cannot support all queries that are possible with SQL in our DBTNG. What I am worried about is that those complicated SQL queries will only work on a specific type of database. For instance: it works on MariaDB, mostly on MySQL and not on PostgreSQL and SQLite.

Another way to fix this is to split the complicated SQL query up into multiple "simpler" queries. I have seen the example given by @chx. In that case we could split into a select query with a join to get the data and use the result to do the update queries. His example was also for a cron job and then it does not matter if they take a bit more time to run.

I do not want to give a hard no, but I can imagine that in some cases having the ability to run complicated SQL queries is important.

If we are going to do this we are going to need a CR, we also need testing for the "new" public method and I think we should also update the file database.api.php with an example.

Maybe issue title and summary here should be reworked like "Make Connection::expandArguments() public to allow array argument expansion outside of Connection::query()". This would abstract away from @Charlie ChX Negyesi use case which IMHO should not be part of the discussion here.

And yes, we need tests to make this method part of the public API - actually I tried to find test coverage for it and could not find it; it may be indirect but then it's very hidden. Such tests here would also address #7.

I made a striking discovery when trying to write a test: this is static. Could be anywhere else even, has nothing to do with Connection at all. Once that realization is made, testing is easy.

The last submitted patch, 14: 3349753_14.patch, failed testing. View results →

Sorry I can't resist properly fixing the failing line.

Except phpcs doesn't recognize that...

Looks good to me; we have now test coverage for the method we did not have before.

We are adding a new method to the public API, therefor we need to add a change record. Can we put a good example in the CR with how to use this method in custom code.

Can we put a good example in the CR with how to use this method in custom code.

I will be honest with you: no. There are no good, clean examples for this. Anything neat is handled by DBTNG all fine. Any usage of this goes against the documented best practices of not using query for UPDATE/INSERT/DELETE.

But it is a necessary evil. I see no way out.

Except for undeprecating RETURN_AFFECTED and let it fly under the radar.

The issue summary mentions two approaches. Can it be highlighted the approach taken.

Appears to be the function was made pubic can that be mentioned in the CR too please.

Thank you. Besides addressing the need in the IS, this also adds explicit tests for expandArguments(), which is good.

Not supporting a particular use case doesn't necessarily make this a bug, I think now the solution/approach has changed this is better classified as a task

One of the fails above on PHP 8.2 relates to the changes here (see the sqlite run)

https://www.drupal.org/pift-ci-job/2634061 →

Fine to self RTBC after that change.

I spent some time reviewing the security implications of this and I can't see any issue.

I'd like to see an additional test case covering the drupalgeddon fix, i.e. a case where the arguments are an array, the placeholder uses [], but the keys to the argument array are not numerically indexed, something like this

  /**
   * Tests the sanitizing of placeholders.
   */
  public function testExpandKeyedArguments(): void {
    $query = '(:x[])';
    $args = [':x[]' => [' where 1=1 OR ' => 1, ' nefarious ' => 2]];
    $modified = Connection::expandArguments($query, $args);
    $this->assertSame(TRUE, $modified);
    $this->assertSame('(:x__0, :x__1)', $query);
    $this->assertSame([':x__0' => 1, ':x__1' => 2], $args);
}

That little array_values in there is one of the most significant in Drupal's history