In various places we ran into issues with PDO Connections stored at some object which is part of the form state,
which results in a error telling you that PDO is not serializable.

Try to implement serialize()/unserialize() to reestablish the connection

Files: 
CommentFileSizeAuthor
#34 drupal-1953800-34.patch29.26 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,610 pass(es).
[ View ]
#34 interdiff.txt4.27 KBdawehner
#31 drupal-1953800-31.patch27.99 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,527 pass(es).
[ View ]
#31 interdiff.txt2.78 KBdawehner
#28 drupal-1953800-28.patch26.88 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 55,688 pass(es).
[ View ]
#28 interdiff.txt706 bytesamateescu
#24 drupal-1953800-24.patch26.8 KBamateescu
FAILED: [[SimpleTest]]: [MySQL] 55,460 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#24 interdiff.txt4.81 KBamateescu
#21 drupal-1953800-21.patch26.47 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 55,630 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#21 interdiff.txt4.54 KBdawehner
#20 drupal-1953800-20.patch23.48 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 55,547 pass(es).
[ View ]
#20 interdiff.txt3.31 KBamateescu
#17 drupal-1953800-17.patch21.94 KBamateescu
FAILED: [[SimpleTest]]: [MySQL] 55,405 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
#17 interdiff.txt6.57 KBamateescu
#15 drupal-1953800-15.patch18.76 KBamateescu
FAILED: [[SimpleTest]]: [MySQL] 55,388 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#15 interdiff.txt705 bytesamateescu
#13 drupal-1953800-13.patch18.61 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 3,220 pass(es), 754 fail(s), and 9 exception(s).
[ View ]
#11 interdiff-6-8.patch2.63 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-6-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#11 drupal-1953800-11.patch18.44 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 2,777 pass(es), 833 fail(s), and 46 exception(s).
[ View ]
#11 interdiff.txt2.27 KBdawehner
#9 drupal-1953800-8.patch18.96 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#6 drupal-1953800-6.patch17.9 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#6 interdiff.txt6.65 KBdawehner
#4 drupal-1953800-4.patch20 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 2,671 pass(es), 756 fail(s), and 10 exception(s).
[ View ]
#4 interdiff.txt1.39 KBdawehner
#2 drupal-1953800-2.patch18.54 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#1 drupal-1953800-1.patch4.79 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 53,903 pass(es).
[ View ]

Comments

dawehner’s picture

StatusFileSize
new4.79 KB
PASSED: [[SimpleTest]]: [MySQL] 53,903 pass(es).
[ View ]

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
StatusFileSize
new18.54 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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
StatusFileSize
new1.39 KB
new20 KB
FAILED: [[SimpleTest]]: [MySQL] 2,671 pass(es), 756 fail(s), and 10 exception(s).
[ View ]

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
StatusFileSize
new6.65 KB
new17.9 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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
StatusFileSize
new18.96 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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
StatusFileSize
new2.27 KB
new18.44 KB
FAILED: [[SimpleTest]]: [MySQL] 2,777 pass(es), 833 fail(s), and 46 exception(s).
[ View ]
new2.63 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-6-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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
StatusFileSize
new18.61 KB
FAILED: [[SimpleTest]]: [MySQL] 3,220 pass(es), 754 fail(s), and 9 exception(s).
[ View ]

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
StatusFileSize
new705 bytes
new18.76 KB
FAILED: [[SimpleTest]]: [MySQL] 55,388 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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
StatusFileSize
new6.57 KB
new21.94 KB
FAILED: [[SimpleTest]]: [MySQL] 55,405 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

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
StatusFileSize
new3.31 KB
new23.48 KB
PASSED: [[SimpleTest]]: [MySQL] 55,547 pass(es).
[ View ]

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
StatusFileSize
new4.54 KB
new26.47 KB
FAILED: [[SimpleTest]]: [MySQL] 55,630 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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
StatusFileSize
new4.81 KB
new26.8 KB
FAILED: [[SimpleTest]]: [MySQL] 55,460 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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
StatusFileSize
new706 bytes
new26.88 KB
PASSED: [[SimpleTest]]: [MySQL] 55,688 pass(es).
[ View ]

Sure thing.

larowlan’s picture

larowlan’s picture

dawehner’s picture

StatusFileSize
new2.78 KB
new27.99 KB
PASSED: [[SimpleTest]]: [MySQL] 55,527 pass(es).
[ View ]
  • 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
StatusFileSize
new4.27 KB
new29.26 KB
PASSED: [[SimpleTest]]: [MySQL] 55,610 pass(es).
[ View ]

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.