Introduce a Connection::prepareStatement method allowing to pass options to the Statement, deprecate Connection::prepareQuery() and Connection::prepare()

I think this is a bug and I agree with the solution. What we also should do is to remove the method prepareQuery from the SQLite driver, because that method does already has the fix and the only difference with its parent method is the fix.

For testing: I do not see how we can create tests for this.

@david_garcia: A very good find and thank you!

This problem is a bug and I agree with the solution.

The patch looks good to me.

For testing: I do not see how we can create tests for this.

For me the patch is RTBC.

We should be able to add unit tests, no?

This bugfix seems like it might be backportable, but I wasn't sure. The impact on custom drivers would be that they now get tables prefixed, and then use their own overridden version of the prepare method if they don't already. What impact would this have (if any) on the MySQL and PostgreSQL drivers?

Here is the method we would now be calling:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Database%...

A bit in the docblock there confused me:

Emulated prepared statements does not communicate with the database server so this method does not check the statement.

That's copied from the PDO method:
http://php.net/manual/en/pdo.prepare.php

But in any case, unit tests would be good. :)

Added test to check if the method prepareQuery calls the method prepare.

Forgot the test only patch to show that it fails without the fix.

The testbot fails for the patch with the fix are not related to the patch.
The testbot fails for the patch without the fix.

The patch is ready for a good review.

For rtbc re-testing to work the last patch on the issue has to be the rtbc patch. Also the patch in #7 does not look ready from the test results.

All testbot problems where not related to the patch. Now all testbot results are green. So back to RTBC.

But still we are not passing the ['pdo'] options from the connectionOptions when obtaining the PDO Statement.

Do we need a followup for this? The issue title does not seem to chime with the actually patch either. Also the last patch on the issue is still not the rtbc patch.

@alexpott: Totally forgot to add the PDO options. The patch from comment #8 is the same as that from comment #7 only without the fix and that one should be failing. Which it does.

The testbot failures are related to the patch with the test and without the fix and that one should fail. So read for review.

Is this something that still needs to be worked on?

Is this something that still needs to be worked on?

Yes, this is something that should be fixed.

Reroll from #15

Patch 26 passes the Connection::defaultOptions() array to the PDO object. Is this okay? Connection::defaultOptions() is not an array of PDO driver settings. It's things like 'throw_exception' => TRUE that the Drupal Connection uses. Is PDO smart enough to know that 'throw_exception' is not a valid option and silently ignore it? (testing shows it must). I've been playing around with this and SQL Server does not seem to like something about this process.

If we were to follow the guidance of the docblock, and only pass PDO options to Connection::prepare(), then there needs to be some place to translate the contents of Connection::defaultOptions() and create an array of PDO driver settings. It looks like prepareQuery() is supposed to be the place, so I think the call should be something like:

$defualt_pdo_options = [];  // or should it be $this->connectionOptions['pdo'] or something similar.
return $this->prepare($query, $defualt_pdo_options);

Drivers that wish to pass some default options will have to override Connection::prepareQuery() and specify them. Similarly, there needs to be a way for Connection to prepare a single statement with a different set of options (and PDO driver settings) without changing the global state of the entire Connection. Something like:

public function customPrepareQuery($query, $options = []) {
   $options += $this->defualtOptions();
   $pdo_options = [];
   // Use options to determine PDO driver settings
   if ($options['foo'] === TRUE) {
     $pdo_options[\PDO::Foo] = \PDO::Bar;
   }
   return $this->prepare($query, $pdo_options);
}

Alternatively, the existing Connection::prepareQuery() could accept an array of options as specified above. This is what sqlsrv currently does, but the method signature of prepareQuery changes in Drupal 9, so I have to refactor the way this was implemented in the Drupal 8 driver.

My vote would be to change it to this:

/**
   * Prepares a query string and returns the prepared statement.
   *
   * This method caches prepared statements, reusing them when
   * possible. It also prefixes tables names enclosed in curly-braces.
   *
   * @param $query
   *   The query string as SQL, with curly-braces surrounding the
   *   table names.
   * @param $options
   *   Options that are used by the method to determine which PDO
   *   driver settings must be passed to Connection::prepare().
   * @return \Drupal\Core\Database\StatementInterface
   *   A PDO prepared statement ready for its execute() method.
   */
  public function prepareQuery($query, array $options = []) {
    $options += $this->defaultOptions();
    $driver_options = driverOptionsFromOptions($options);

    $query = $this->prefixTables($query);

    return $this->prepare($query, $driver_options);
  }

  /**
   * Custom Driver logic to determine what PDO driver options
   * must be used for a statement
   */
  protected function driverOptionsFromOptions(array $options = []) {
    return [];  // Or return $options['pdo'] ?? [];
  }

As an aside...where, exactly, does this supposed "caching of prepared statements" occur that is mentioned in the docbock? Does PDO cache queries? Is this consistent with all databases? Is there a setting which affects this behavior?

@Beakerboy: I do not fully understand what you want to happen. I understand that SQL server has some problems with "extra/other" connection options. Could you write a patch?

The return type listed in Connection::prepare() is listed as @return \PDOStatement|false while the type in Connection::prepareQuery() is @return \Drupal\Core\Database\StatementInterface despite the fact that they both call $this->connection->prepare(). The comment block for $this->connection says it is of type \PDO.

There probably needs to be some clarification as to the differences and purposes of prepare() vs prepareQuery(). Is Connection::prepare() only to be used when the statement class is not provided by Drupal? As far as I can see, Connection::prepare() is never used. I think understanding the intention of having two preparation methods is key to solving this issue.

I think understanding the intention of having two preparation methods is key to solving this issue.

I think this is a legacy impact of D7 architecture. In Drupal 7 the connection class extends directly from \PDO - so in order have a different prepare we added prepareQuery() - so now we have both prepare and prepareQuery(). Then in Drupal 8 we disentangle PDO and our Connection object and to preserve BC we implement the prepare method.

I think the best steps forward here are to:

• Deprecate the \Drupal\Core\Database\Connection::prepare() method and it's override
• Replace any calls to prepare() with prepareQuery()
• Refactor SQLites prepare implementation into its prepareQuery()
• Add an additional argument of $driver_options to prepareQuery() in case any custom code is relying on this

Also let's rename $driver_options to $pdo_driver_options to be 100% clear what they are. Note the only real use cases for passing stuff in here is custom database driver dependent code. It's not stuff we should see in core or normal contrib.

What drupal version should the deprecation be targeted for?

Drupal 10. We can only make this change / tidy up in Drupal 9.1.x

+1 to #31.

If I may, when touching this, I would also suggest to pass in the $args variable (see ::query). That could help contrib to manage e.g. replacing at low level named placeholders with positional placeholders and allow running with database clients that do not support named placeholders (for example, mysqli instead of pdo_mysql).

In my experimental DruDbal driver, I have the following in the Connection class:

  /**
   * Prepares a query string and returns the prepared statement.
   *
   * This method caches prepared statements, reusing them when possible. It also
   * prefixes tables names enclosed in curly-braces.
   * Emulated prepared statements does not communicate with the database server
   * so this method does not check the statement.
   *
   * @param string $query
   *   The query string as SQL, with curly-braces surrounding the
   *   table names. Passed by reference to allow altering by DBAL extensions.
   * @param array $args
   *   An array of arguments for the prepared statement. If the prepared
   *   statement uses ? placeholders, this array must be an indexed array.
   *   If it contains named placeholders, it must be an associative array.
   *   Passed by reference to allow altering by DBAL extensions.
   * @param array $driver_options
   *   (optional) This array holds one or more key=>value pairs to set
   *   attribute values for the Statement object that this method returns.
   * @param bool $quote_identifiers
   *   (optional) Quote any identifiers enclosed in square brackets. Defaults to
   *   TRUE.
   *
   * @return \Drupal\Core\Database\StatementInterface|false
   *   If the database server successfully prepares the statement, returns a
   *   StatementInterface object.
   *   If the database server cannot successfully prepare the statement returns
   *   FALSE or emits an Exception (depending on error handling).
   */
  public function prepareQueryWithParams(string &$query, array &$args = [], array $driver_options = [], bool $quote_identifiers = TRUE) {
    $query = $this->prefixTables($query);
    if ($quote_identifiers) {
      $query = $this->quoteIdentifiers($query);
    }
    return new $this->statementClass($this, $query, $args, $driver_options);
  }

  /**
   * {@inheritdoc}
   */
  public function prepareQuery($query, $quote_identifiers = TRUE) {
    // Should not be used, because it fails to execute properly in case the
    // driver is not able to process named placeholders. Use
    // ::prepareQueryWithParams instead.
    // @todo raise an exception and fail hard??
    $args = [];
    return $this->prepareQueryWithParams($query, $args, [], $quote_identifiers);
  }

  /**
   * {@inheritdoc}
   */
  public function prepare($statement, array $driver_options = []) {
    // Should not be used, because it fails to execute properly in case the
    // driver is not able to process named placeholders. Use
    // ::prepareQueryWithParams instead.
    // @todo raise an exception and fail hard??
    $params = [];
    return new $this->statementClass($this, $statement, $params, $driver_options);
  }

then, in the Statement constructor,

  /**
   * Constructs a Statement object.
   *
   * @param \Drupal\drudbal\Driver\Database\dbal\Connection $dbh
   *   The database connection object for this statement.
   * @param string $statement
   *   A string containing an SQL query. Passed by reference.
   * @param array $params
   *   (optional) An array of values to substitute into the query at placeholder
   *   markers. Passed by reference.
   * @param array $driver_options
   *   (optional) An array of driver options for this query.
   */
  public function __construct(DruDbalConnection $dbh, &$statement, array &$params, array $driver_options = []) {
    $this->queryString = $statement;
    $this->dbh = $dbh;
    $this->setFetchMode(\PDO::FETCH_OBJ);

    // Replace named placeholders with positional ones if needed.
    if (!$this->dbh->getDbalExtension()->delegateNamedPlaceholdersSupport()) {
      list($statement, $params) = SQLParserUtils::expandListParameters($statement, $params, []);
    }

    try {
      $this->dbh->getDbalExtension()->alterStatement($statement, $params);
      $this->dbalStatement = $dbh->getDbalConnection()->prepare($statement);
    }
    catch (DBALException $e) {
      throw new DatabaseExceptionWrapper($e->getMessage(), $e->getCode(), $e);
    }
  }

This also tells me that maybe instead of a method on the Connection class to retrieve a (Drupal) StatementInterface object, we might as well just deprecate them all and just construct an instance of the driver's Statement object and move the initialization code there.

Very rough proposal re. #35 - no interdiff, different approach.

Tweaks.

OK, I'll work on a test issue and report back when I have sth shareable.

I've read the lastest patch and found some typos in the annotation. Therefore, I found it useful to provide the patch for those fixes here ( as per the code revewing guidelines).

@mondrake looking at #38

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
@@ -404,6 +405,7 @@ public function mapConditionOperator($operator) {
   public function prepareQuery($query, $quote_identifiers = TRUE) {
+    @trigger_error('@todo. See https://www.drupal.org/node/TODO', E_USER_DEPRECATED);

I think we should only be deprecating prepare() and I think we need to add the $pdo_driver_options to this method.

@shobhit_juyal Your formatting issue is unrelated to this one. As straightforward as your patch is, it’s likely to be RTBC and merged much more quickly on its own than to wait for this issue to come to consensus and for someone to merge your formatting proposal with it.

Only deprecating prepare() as per #41. Still, adding also the $args to prepareQuery per #35, which would fit with DruDbal.

PgSql fixes.

Add passing the options to the PDO prepare() method ... which is actually the reason of this issue.

Patch looks good!

1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -184,7 +184,7 @@ public function query($query, array $args = [], $options = []) {
   -  public function prepareQuery($query, $quote_identifiers = TRUE) {
   +  public function prepareQuery($query, $quote_identifiers = TRUE, array $args = [], array $pdo_driver_options = []) {
   

   We are touching this method. Can we add an @inheritdoc docblock to the method?

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -489,17 +489,23 @@ public function getFullQualifiedTableName($table) {
   +  public function prepareQuery($query, $quote_identifiers = TRUE, array $args = [], array $pdo_driver_options = []) {
   

   Can we add a test that uses the pdo_driver_options.

The IS needs an update.
A CR is needed.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -489,17 +489,23 @@ public function getFullQualifiedTableName($table) {
@@ -730,7 +736,7 @@ public function query($query, array $args = [], $options = []) {

@@ -730,7 +736,7 @@ public function query($query, array $args = [], $options = []) {
         if (strpos($query, ';') !== FALSE && empty($options['allow_delimiter_in_query'])) {
           throw new \InvalidArgumentException('; is not supported in SQL strings. Use only one statement at a time.');
         }
-        $stmt = $this->prepareQuery($query, !$options['allow_square_brackets']);
+        $stmt = $this->prepareQuery($query, !$options['allow_square_brackets'], $args, $options);
         $stmt->execute($args, $options);

Actually, here $options does not bear PDO options, but the options passed to ::query. Passing those as such to the prepare statement would lead to unpredictable results. I think we should pass a $options['pdo'] subarray, documenting clearly in ::query docblock that when you want to pass PDO options to the driver, you have to structure thos likewise. I do not think core will be using any of this, right?

Also, I would suggest to deprecate prepareQuery so we can rename the method to prepareStatement, since that's what this method is doing AFAICS.

EDIT - by the way this would also be more gentle to contrib, that would be otherwise forced to update the signature of prepareQuery.

1. For comment #48: The only example that is given in https://www.php.net/manual/en/pdo.prepare.php is: "You would most commonly use this to set the PDO::ATTR_CURSOR value to PDO::CURSOR_SCROLL to request a scrollable cursor. Some drivers have driver-specific options that may be set at prepare-time." Therefor I do not think that core will be using this. Not sure if we can create a test for this. The $options are not the same as $pdo_driver_options. Something like passing $options['pdo'] instead of $options looks like a good idea.
2. +1 For renaming prepareQuery to prepareStatement.

3. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -489,17 +489,23 @@ public function getFullQualifiedTableName($table) {
   +   * @param array $args
   +   *   An array of arguments for the prepared statement. If the prepared
   +   *   statement uses ? placeholders, this array must be an indexed array.
   +   *   If it contains named placeholders, it must be an associative array.
   

   Why are we adding this parameter to the method prepareQuery?

#50.3 see #35.

Mainly for contrib.

btw, StatementPrefetch, used by Sqlite, requires the args.

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -489,17 +489,23 @@ public function getFullQualifiedTableName($table) {
   +   * @param array $args
   +   *   An array of arguments for the prepared statement. If the prepared
   +   *   statement uses ? placeholders, this array must be an indexed array.
   +   *   If it contains named placeholders, it must be an associative array.
   

   Given that this is not used in the default implementation we need a couple of things.
   * Good docs about why it's here.
   * A test driver that helps us test was is passed in.

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -730,7 +736,7 @@ public function query($query, array $args = [], $options = []) {
   +        $stmt = $this->prepareQuery($query, !$options['allow_square_brackets'], $args, $options);
   

   $options here are not pdo options. I don't think we should be passing them in here.

So here:

• deprecating prepareQuery
• introducing prepareStatement, building more rounded docs hopefully
• adding a 'pdo' key to defaultOptions, init to empty array, to manage properly passing of $driver_options to PDO::prepare (see https://www.php.net/manual/en/pdo.prepare.php)
• AFAIK Drupal ONLY uses named placeholders now, so the docs for ::query are stale - adjusted

The doc block says

This method caches prepared statements, reusing them when possible.

Is that true? Where? How?

Well I think that means that if, instead of calling query, you call queryPrepare and then iterate on the statement with execute and different sets of args, you just ‘reuse’ the single initial statement object. But I am not sure there is explicit test coverage for that, and AFAICS it’s not a pattern used in core.

Fixes for PgSql, deprecation test for prepareQuery, and adding a test for prepareStatement exceptions like prepare used to have. Need a test for prepareStatement (in general + for the specifics of the $args and $options parameters). There is no test coverage in HEAD currently for prepareQuery :( otherwise we would have got deprecation errors.

Fixing failures + adding test coverage for prepareStatement. Still missing a test for usage of $args and $options within an alternative test driver.

Now we have prepare and prepareQuery and prepareStatement. Yes two are deprecated but still... I'm not entirely convinced that passing more than $quote_identifiers and $pdo_driver_options is a good idea. I'm not sure that we really want DB drivers to execute any real logic in this method.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -315,6 +315,11 @@ public function destroy() {
+   * - pdo: By default, queries will execute with the PDO options set on the
+   *   connection. In particular cases, it could be necessary to override the
+   *   PDO driver options on the statement level. In such case, pass the
+   *   required setting as an array here, and they will be passed to the
+   *   prepared statement. See https://www.php.net/manual/en/pdo.prepare.php.

This is a good idea. Adding this as an additional option here makes a ton of sense.

Addressing SQLite failure. Thinking on a test to demonstrate #35.

One thing to consider is, if a driver overrides the Statement class and wishes to pass statement-level, non PDO direction to the class, there needs to be a place for this. I think this is a good argument for not mandating that only PDO options are passed to prepareQuery (or prepareStatement).

Hi @mondrake: As for your DruDbal database driver and the $args parameter in the method Drupal\Core\Database\Connection::prepareStatement(). I think that we should remove the $args parameter in Drupal\Core\Database\Connection::prepareStatement() as it is not used in core and the method is also not used outside of any database driver. Therefor your DruDbal driver can create it own version of the method with the extra parameter $args. If this does not work for the DruDbal database driver, then please say so and explain why it does not. Thank you for working on this patch, it looks good!

#63 yes I think I can work with that.

On this.

Removed $args.

The reason I was reluctant to remove $args is that if you use ::prepareStatement outside of ::query, or more in general outside of database driver code, and your driver Statement object needs that at the time of instantiating it, then there will be no way to get that back. In my specific case in DruDbal, since there's a case (as described in #35, already) that I will need to alter the querystring BEFORE it is prepared into the statement, I will change my implementation of Statement just to hold calling the lower-level statement prepare method until I have the $args available, i.e. until the first time ::execute is called.

1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -184,7 +185,23 @@ public function query($query, array $args = [], $options = []) {
   +  /**
   +   * {@inheritdoc}
   +   */
      public function prepareQuery($query, $quote_identifiers = TRUE) {
   +    @trigger_error('Connection::prepareQuery() is deprecated in drupal:9.1.0 and is removed in drupal:10.0.0. Use ::prepareStatement() instead. See https://www.drupal.org/node/TODO', E_USER_DEPRECATED);
        // mapConditionOperator converts some operations (LIKE, REGEXP, etc.) to
        // PostgreSQL equivalents (ILIKE, ~*, etc.). However PostgreSQL doesn't
        // automatically cast the fields to the right type for these operators,
   
   +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
   @@ -5,6 +5,7 @@
    /**
     * SQLite implementation of \Drupal\Core\Database\Connection.
   @@ -336,6 +337,7 @@ public static function sqlFunctionLikeBinary($pattern, $subject) {
   
   @@ -336,6 +337,7 @@ public static function sqlFunctionLikeBinary($pattern, $subject) {
       * {@inheritdoc}
       */
      public function prepare($statement, array $driver_options = []) {
   
   @@ -400,10 +402,22 @@ public function mapConditionOperator($operator) {
      /**
       * {@inheritdoc}
       */
      public function prepareQuery($query, $quote_identifiers = TRUE) {
   

   It's quite tempting to make the \Drupal\Core\Database\Connection implementations call prepareStatement(). That way every core driver has the same deprecations and any prepareStatement overrides are where to look for anything special.

   Plus the deprecated methods need an @deprecated annotation.

2. +++ b/core/tests/Drupal/KernelTests/Core/Database/DatabaseExceptionWrapperTest.php
   @@ -40,6 +43,51 @@ public function testPreparedStatement() {
   +    try {
   ...
   +      $this->fail("A \\PDOException or a DatabaseExceptionWrapper should be caught, none was thrown.");
   +    }
   +    catch (\Exception $e) {
   +      $this->assertTrue($e instanceof \PDOException || $e instanceof DatabaseExceptionWrapper, "A \\PDOException or a DatabaseExceptionWrapper should be thrown, " . get_class($e) . " was thrown instead:\n" . (string) $e);
   +    }
   ...
   +    try {
   ...
   +      $this->fail("A \\PDOException or a DatabaseExceptionWrapper should be caught, none was thrown.");
   +    }
   +    catch (\Exception $e) {
   +      $this->assertTrue($e instanceof \PDOException || $e instanceof DatabaseExceptionWrapper, "A \\PDOException or a DatabaseExceptionWrapper should be thrown, " . get_class($e) . " was thrown instead:\n" . (string) $e);
   +    }
   

   If instead of a try catch can we use $this->expectException - and change the class dependent on the db driver.

Thanks for review.

#68.1: Done, with the caveat that

1. we can do that on prepareQuery, but not on prepare unless we accept a behavior change since that method is not resolving the table names nor replacing the square brackets with identifiers
2. done the @deprecated, but the CR is stiil left in as TODO in @see

#68.2: actually that entire test seems weird and maybe a bit rusty - I assume that, with StatementPrefetch, SQLite syntax is no longer checked at prepare time? Let's see this.

Created a change record.

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1649,9 +1679,14 @@ public function commit() {
   +    @trigger_error('Connection::prepare() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use ::prepareStatement instead. See https://www.drupal.org/node/TODO', E_USER_DEPRECATED);
   
   +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
   @@ -336,6 +337,7 @@ public static function sqlFunctionLikeBinary($pattern, $subject) {
   +    @trigger_error('Connection::prepare() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use ::prepareStatement instead. See https://www.drupal.org/node/TODO', E_USER_DEPRECATED);
   

   This method is not being replaced by ::prepareStatement. It is being replaced by \PDO::prepare().

2. The patch needs updating for the added CR.

3. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1649,9 +1679,14 @@ public function commit() {
       * @see \PDO::prepare()
   

   Can we change this to: "@see https://www.php.net/manual/en/pdo.prepare.php"

Thanks @daffie!

#72:
1. well yes and no - I added more explanation to the deprecation message.
2. Done
3. Done

Updated the IS.

All code changes look good to me.
The IS has been updated.
The 2 deprecated method have deprecation testing.
There is also testing for the new method.
There are a lot of comments added to the patch.
For me it is RTBC.

I filed a possible follow-up, #3137883: Deprecate passing a StatementInterface object to Connection::query. Looking forward to discussion there!

The current relevant tag would have been "Drupal 9 porting weekend" but there was no activity on it yet for that, so removing it for now.

#74 got broken in test by #3133798: Semicolon removed from query even when it is allowed. Here we are introducing return typehints for prepareStatement which is a benefit, I suppose.

Test fix looks correct.
Back to RTBC.

Committed 3917030 and pushed to 9.1.x. Thanks!

As this contains deprecations we can only commit this to 9.1.x which might mean that some contrib db drivers will only support 9.1.x and on but there's not much we can do about that.