Add bindValue to a PDO::PARAM_* type in database query arguments

Not sure what we did in the meantime, but currently at least for PostgreSQL all LOB tests do pass (insert and update tests on docker testbot). From the logs:

---- Drupal\system\Tests\Database\InsertLobTest ----


Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Pass      Other      DatabaseTestBase.   23 Drupal\system\Tests\Database\Databa
    Enabled modules: database_test.
Pass      Other      DatabaseTestBase.   33 Drupal\system\Tests\Database\Databa
    Installed database_test tables: {test}, {test_people}, {test_people_copy},
    {test_one_blob}, {test_two_blobs}, {test_task}, {test_null},
    {test_serialized}.
Pass      Other      InsertLobTest.php   22 Drupal\system\Tests\Database\Insert
    Test data contains a NULL.
Pass      Other      InsertLobTest.php   27 Drupal\system\Tests\Database\Insert
    Can insert a blob: id 1,
    a:2:{s:2:"id";s:1:"1";s:5:"blob1";s:15:"This
    isa test.";}.
Pass      Other      DatabaseTestBase.   23 Drupal\system\Tests\Database\Databa
    Enabled modules: database_test.
Pass      Other      DatabaseTestBase.   33 Drupal\system\Tests\Database\Databa
    Installed database_test tables: {test}, {test_people}, {test_people_copy},
    {test_one_blob}, {test_two_blobs}, {test_task}, {test_null},
    {test_serialized}.
Pass      Other      InsertLobTest.php   41 Drupal\system\Tests\Database\Insert
    Can insert multiple blobs per row.

Seems this is still required for exactly one! failing test ;-) will see if so...

Sorry, I have no time to check this over the next days...

Patch reroll from comment #9

It all looks good to me.
I made some minor documentation changes.
For me it is RTBC.

Sorry, I think this needs at least testing for SQLite too and to be true, I never ever tested if this is still needed for pg/driver at all. At least I have seen no effect of this patch. Can we just try to run only the new tests without the patch itself first please, just to make sure?

I have ran the test with a PostgreSQL database and without the rest of the patch. I removed the extra parameters of one call in the test. The test fails.
Will try later with SQLite.

I ran the test with a SQLite database and no fails and no exceptions. No problems with SQLite for this patch!

Thanks daffie for testing! I'm fine with the patch as is, but would prefer a second review from @mradcliffe before setting this RTBC.

Simple reroll. Added two extra patch with only the the added test. Because we are doing a api extension for the Drupal\Core\Database\Connection::query by adding an extra parameter, I have also added a test only patch without the extra parameter being called.

The testbot is returning the following error: HP Fatal error: Call to undefined method Drupal\Core\Database\Driver\sqlite\Statement::bindValue() in /var/www/html/core/lib/Drupal/Core/Database/Connection.php on line 623

Changed Drupal\Core\Database\Driver\sqlite\Connection::query to be the same as the old Drupal\Core\Database\Connection::query, but the extra parameter.

Minor change to Drupal\Core\Database\Driver\sqlite\Connection::query. Adding the $bindValue to the calling of $this->expandArguments.

This issue is a feature request, so moving this to 8.3.

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -561,6 +561,10 @@ protected function filterComment($comment = '') {
   +   *   An array of PDO types for the arguements. If the prepared statement
   

   Typo in comment. Should be "arguments".

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -607,8 +611,23 @@ public function query($query, array $args = array(), $options = array()) {
   +        foreach ($bindValue as $k => $pdo_type) {
   +          $stmt->bindValue($k, $args[$k], $pdo_type);
   +        }
   +        $execArgs = array();
   +        foreach (array_diff_key($args, $bindValue) as $k => $v) {
   +          if (is_numeric($k)) {
   +            $execArgs[$k] = $v;
   +          }
   +          else {
   +            $stmt->bindValue($k, $v);
   +          }
   +        }
   +        if (empty($execArgs)) {
   +          $execArgs = NULL;
   +        }
   
   +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
   @@ -142,6 +142,66 @@ public static function open(array &$connection_options = array()) {
   +   * For SQLite it is no method
   +   * \Drupal\Core\Database\Driver\sqlite\Statement::bindValue()
   +   * so we cannot call the method when doing a query.
   +   */
   +  public function query($query, array $args = array(), $options = array(), $bindValue = array()) {
   

   If this were wrapped in a condition, then I don't think you would need to override the query method in sqlite driver.

   If there were a method like canUseBindParameters(), and that's an opt-in thing in 8.3.x, then only mysql and pgsql would implement, and sqlite and any other driver would not need to make any changes.

   Should this be its own method? That would mean passing the statement object with its state around, and that might not be desirable from a functional perspective.

3. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -607,8 +611,23 @@ public function query($query, array $args = array(), $options = array()) {
   +        foreach (array_diff_key($args, $bindValue) as $k => $v) {
   

   This should be fine since indexed keys wouldn't be in $args, right?

Can't find anything other than that.

I didn't have a detailed look at the patch yet, but looking at:

https://dispatcher.drupalci.org/job/default/226878/

I'm a bit concerned about performance as the runtime was 1h 19min compared to around 56min for the usual branch run. Should we start a new test just to make sure?

@bzrudi71: I did not looked at the run times. I have started to rerun the tests. If they are still as slow as the previous ones then we should move this issue to will not fix. Great find!

Okay, last run 1h 8min:

https://dispatcher.drupalci.org/job/default/242441/

Runtime for another issue that removes code from pg driver 1h 7min
#2057693: PostgreSQL orderBy method adds fields it doesn't need to, leading to fatal errors when the result is used as an insert subquery

https://dispatcher.drupalci.org/job/default/242468/

So this seems not to cause a bad performance impact. Anyway, looking at:

https://dispatcher.drupalci.org/job/php7_postgres9.1/buildTimeTrend

it is really annoying to see runtimes for the same branch tests differ that much :( The MySQL bot is always within 37-40 min. Should we ping the drupalCI guys?

Maybe we should wait until #2057693: PostgreSQL orderBy method adds fields it doesn't need to, leading to fatal errors when the result is used as an insert subquery and #1079762: DatabaseSchema_pgsql::queryTableInformation() is slow have landed. @bzrudi71: But if you want to contact the drupalCI guys that would also be great.

@daffie good idea let's wait for the other issues to go in first. Opened #2821754: PostgreSQL runtime PingPong in the DrupalCI queue.

Reroll is needed.

Postponed on #2057693: PostgreSQL orderBy method adds fields it doesn't need to, leading to fatal errors when the result is used as an insert subquery per #42.

Also needs an opinion from maintainer like @daffie on #38.2