Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

FileSize
4.79 KB

This doesn't work yet, as you can't replace the full object but just replace certain properties, so I'm not sure whether this is the right approach.

dawehner’s picture

Status: Active » Needs review
FileSize
18.54 KB

chx had the wonderful idea to inject the PDO connection into the connection object.

Running an existing site works, install drupal still fails.

Status: Needs review » Needs work

The last submitted patch, drupal-1953800-2.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.39 KB
20 KB

Nice, it starts to actually install now (on mysql), though this really need help for pgsql, and sqlite.

Status: Needs review » Needs work

The last submitted patch, drupal-1953800-4.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.65 KB
17.9 KB

Chx had the idea to remove all kind of changes, which aren't required in this patch.

Status: Needs review » Needs work

The last submitted patch, drupal-1953800-6.patch, failed testing.

yched’s picture

Related proposal: #1977206: Default serialization of ConfigEntities - might not be a good idea for Views though, not sure.

dawehner’s picture

Status: Needs work » Needs review
FileSize
18.96 KB

In order to install drupal you need at least these changes!

Status: Needs review » Needs work

The last submitted patch, drupal-1953800-8.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.27 KB
18.44 KB
2.63 KB

Posted the interdiff of previous change.

:(

Status: Needs review » Needs work

The last submitted patch, drupal-1953800-11.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
18.61 KB

Cleaned up the patch a bit, though I still could not figure out why the tests aren't running.

Status: Needs review » Needs work

The last submitted patch, drupal-1953800-13.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
705 bytes
18.76 KB

Here you go :)

Status: Needs review » Needs work

The last submitted patch, drupal-1953800-15.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
6.57 KB
21.94 KB

This one should fix that error and also clean it up a bit.

dawehner’s picture

Issue tags: +Needs manual testing
+++ b/core/lib/Drupal/Core/Database/Connection.phpundefined
@@ -1192,12 +1197,42 @@ public function commit() {
+   *   (optional) Provides a data type hint for drivers that have alternate
+   *   quoting styles.

We should mention the default value here.

@amateescu
I guess you have tried to run the code with sqlite, but we certainly need someone who tries the patch out with pgsql. That's a pretty depth change, so we maybe should even run the full testsuite for pgsql. Does the testbot allows us to do that? Does someone know whether the testsuite runs without errors?

Status: Needs review » Needs work

The last submitted patch, drupal-1953800-17.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
3.31 KB
23.48 KB

And here's another update that actually does what this issue was intended for, handle serialization and deserialization of Connection objects.

I guess you have tried to run the code with sqlite

Not really, it's just my IDE that warned me about using $this in a static method, so I fixed it. This needs real testing on both SQLite and PostgreSQL.

dawehner’s picture

Title: Figure out serialization of PDO Connections » Make the database connection serializable
FileSize
4.54 KB
26.47 KB

Let's document that a bit better. I tried to write also a test with an actual form, but for some reason failed.

Status: Needs review » Needs work

The last submitted patch, drupal-1953800-21.patch, failed testing.

Crell’s picture

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -135,23 +142,34 @@
+  public static function open(array &$connection_options = array()) { }

Seems like this should be an abstract method, to force implementers to implement it.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1178,4 +1195,73 @@ public function commit() {
+  public function serialize() {
+    // Just store the target and the key of the connection because this is all
+    // what drupal needs to get a proper connection running again.
+    return serialize(array($this->target, $this->key));

Nitpick: It's probably better to use a keyed array, as it's more self-documenting than list() when unserializing. Just a better practice.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1178,4 +1195,73 @@ public function commit() {
+  public function unserialize($serialized) {
+    list($target, $key) = unserialize($serialized);
+    foreach (get_object_vars(Database::getConnection($target, $key)) as $name => $value) {
+      $this->{$name} = $value;
+    }

That looks very wrong to me. In this case, shouldn't you be calling self::open() with the saved data?

In which case, you need more than just target and key; you'd need whatever connection params were given originally, which would include things like prefix.

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
@@ -158,14 +173,14 @@ public function __destruct() {
-  public function sqlFunctionIf($condition, $expr1, $expr2 = NULL) {
+  public static function sqlFunctionIf($condition, $expr1, $expr2 = NULL) {

Why do these need to become static?

Serializing a connection object makes me squirm, and strikes me as a code smell elsewhere that we're storing the DB connection in too many places. That said, I LOVE the idea of separating the PDO object from Connection; making it a subclass in the first place was, in hindsight, a mistake. So, it averages out to +1. :-)

The split between getConnection() and open() feels wrong, though. Connection::open() seems like it should... open a Connection, not be a factory for PDO. Maybe it's just a rename that's needed?

amateescu’s picture

Status: Needs work » Needs review
FileSize
4.81 KB
26.8 KB

Ok, this one should be better. Fixed the serialization and unserialization code and also improved the tests.

Why do these need to become static?

Because they are now called from a static method (Drupal\Core\Database\Driver\sqlite\Connection::open()).

The split between getConnection() and open() feels wrong, though. Connection::open() seems like it should... open a Connection, not be a factory for PDO. Maybe it's just a rename that's needed?

Suggestions welcome :)

Status: Needs review » Needs work

The last submitted patch, drupal-1953800-24.patch, failed testing.

tim.plunkett’s picture

Category: task » bug
Priority: Normal » Critical

Since this means we have to kneecap all of the controller conversions (which isa critical), this also deserves to be critical.

Crell’s picture

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1178,4 +1195,76 @@ public function commit() {
+  public function serialize() {
+    $connection = clone $this;
+    // Don't serialize the PDO connection.
+    unset($connection->connection);
+    return serialize(get_object_vars($connection));

We probably want to also exclude things like the schema object, and anything else that was lazy-instantiated. We can just lazy-instantiate those again.

amateescu’s picture

Status: Needs work » Needs review
FileSize
706 bytes
26.88 KB

Sure thing.

larowlan’s picture

larowlan’s picture

dawehner’s picture

FileSize
2.78 KB
27.99 KB
  • Let's also test the actual values on the object.
  • Move the tests to unit tests.
Crell’s picture

Status: Needs review » Reviewed & tested by the community

I love that this splits Connection from PDO itself. That was one of our big mistakes originally so I'm glad to see it fixed here.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

This patch break overriding PDO options...

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.phpundefined
@@ -58,7 +68,9 @@ public function __construct(array $connection_options = array()) {
     // Allow PDO options to be overridden.
     $connection_options += array(
-      'pdo' => array(),
+      'pdo' => array(
+        PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
+      ),
     );
     $connection_options['pdo'] += array(
       // So we don't have to mess around with cursors and unbuffered queries by default.

Using mode context this should be...

    // Allow PDO options to be overridden.
    $connection_options += array(
      'pdo' => array(),
    );
    $connection_options['pdo'] += array(      
      PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
      // So we don't have to mess around with cursors and unbuffered queries by default.
      PDO::MYSQL_ATTR_USE_BUFFERED_QUERY => TRUE,
      // Because MySQL's prepared statements skip the query cache, because it's dumb.
      PDO::ATTR_EMULATE_PREPARES => TRUE,
    );
+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.phpundefined
@@ -61,14 +81,14 @@ public function __construct(array $connection_options = array()) {
     // Allow PDO options to be overridden.
     $connection_options += array(
-      'pdo' => array(),
+      'pdo' => array(
+        PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
+      ),
     );
     $connection_options['pdo'] += array(
       // Prepared statements are most effective for performance when queries

This needs fixing for the same reason...

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.phpundefined
@@ -107,24 +102,44 @@ public function __construct(array $connection_options = array()) {
+    $connection_options += array(
+      'pdo' => array(
+        PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
+      ),
+    );
+    $connection_options['pdo'] += array(
+      // Convert numeric values to strings when fetching.
+      PDO::ATTR_STRINGIFY_FETCHES => TRUE,

This needs fixing for the same reason...

Here's why...

<?php
  $connection_options = array('pdo'=>  array(PDO::MYSQL_ATTR_USE_BUFFERED_QUERY => FALSE));
  
  // Allow PDO options to be overridden.
  $connection_options += array(
    'pdo' => array(
      PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
    ),
  );
  $connection_options['pdo'] += array(
    // So we don't have to mess around with cursors and unbuffered queries by default.
    PDO::MYSQL_ATTR_USE_BUFFERED_QUERY => TRUE,
    // Because MySQL's prepared statements skip the query cache, because it's dumb.
    PDO::ATTR_EMULATE_PREPARES => TRUE,
  );

  var_dump($connection_options);

And the output is...

array(1) {
  'pdo' =>
  array(2) {
    [1000] =>
    bool(false)
    [20] =>
    bool(true)
  }
}

Whereas I think the expected output is

array(1) {
  'pdo' =>
  array(3) {
    [3] =>
    int(2)
    [1000] =>
    bool(false)
    [20] =>
    bool(true)
  }
}

And since this is breakable (as proved by the patch) and I think quite easy to test... we should add a test for it.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.27 KB
29.26 KB

So what about a test like that?

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Nice test! Back to RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5665307 and pushed to 8.x. Thanks!

dcrocks’s picture

This fix introduced errors during install when SQLite was chosen as the database. See #1998366: [meta] SQLite is broken. A fix will be generated there.

Automatically closed -- issue fixed for 2 weeks with no activity.

amateescu’s picture

Issue summary: View changes
Issue tags: -Needs manual testing

Noting for posterity that the "db connection object is now serializable" part of this patch was reverted in #2463321: Serializing the database connection is dangerous and error-prone, make it unserializable again, but the change to Drupal\Core\Database\Connection to not extend \PDO anymore was kept.