Serializing the database connection is dangerous and error-prone, make it unserializable again

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1315,17 +1315,13 @@ public function unserialize($serialized) {
+    // Call the constructor so all the internal properties are properly set.
+    static::__construct($connection, $connection_options);

False. If code from the constructor needs to be called under other circumstances, move it out of the constructor and call from both places.

So call it something like init() and do all the logic in there?

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -143,16 +143,24 @@
+  protected function init() {

@@ -1320,12 +1328,8 @@ public function unserialize($serialized) {
+    static::init();

I think we should not call it statically ... Sometimes PHP is just weird.

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
@@ -507,4 +505,15 @@ public function popTransaction($name) {
+  public function serialize() {
+    // Don't serialize the 'attachedDatabases' property because it's lazily
+    // populated in static::init().
+    unset($this->attachedDatabases);
+
+    return parent::serialize();
+  }

I just tested with the attached file and this is very dangerous. It modifies, and renders unusable, the current connection object. That means serializing an object that references back to the connection (which is already a bad idea and a sign of a bug elsewhere) will destroy that connection. That is, serialize() becomes a non-idempotent destructive operation.

I'm not sure if I'm worried that the testbot is not dying as a result of that or what...

Ugh. The trait introduces another dependency on Drupal, which we want to remove from the DB layer. The real solution is "WTF are you doing serializing the connection, fix your damned code." But... Drupal. :-(

IIRC, this was due to a deep deep dependency from serialized forms -> entity -> entity storage -> DB. Or similar. Is that still an issue, or did we fix that deep tree already, rendering this unnecessary?

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -20,7 +17,7 @@
   -abstract class Connection implements \Serializable {
   +abstract class Connection {
   

   I think much like Settings we should mark it as not serializeble by throwing an exception. I could imagine more places with direction injection of the database connection.

2. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -31,6 +32,8 @@
     */
    class SessionManager extends NativeSessionStorage implements SessionManagerInterface {
    
   +  use DependencySerializationTrait;
   

   I'm a bit confused, does that mean every class getting the DB connection injected need that Trait?

dawehner: Only those classes that get a DB connection and should themselves be serialized... which is a very small number. More often than not the answer is that we need to better separate "be a thing" objects (which should be serializable) from "do a thing" objects (which should never be). Currently we do a poor job of that, which is the root problem.

I'd be fine with the Connection object throwing a "don't serialize this, nitwit!" exception, since there's no case in which serializing it isn't a bug somewhere, by someone. (You'd get one anyway with the PDO object it wraps, but a nicer error == better DX.)

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1297,35 +1294,10 @@ public function quote($string, $parameter_type = \PDO::PARAM_STR) {
+    throw new \Exception("Don't serialize this, nitwit!");

LOL. I'd suggest something a little more friendly, like:

"The database connection is not serializable. This probably means you are serializing an object that has an indirect reference to the database connection. Adjust your code so that is not necessary. Alternatively, look at DependencySerializationTrait as a temporary solution."

I'm otherwise much happier here, because it pushes the fugly ("You're serializing something you shouldn't be, here's the workaround") further up the stack to where it belongs, ie, closer to where the actual bug is.

It looks great for me now!

I'd feel better about Alex reviewing this one, given the possible security implications.

In Drupal\Core\Site\Settings we throw a BadMethodCallException which I think is wrong since the PHP docs are:

Exception thrown if a callback refers to an undefined method or if some arguments are missing.

I think what we're dealing with here is a LogicException

Exception that represents error in the program logic. This kind of exceptions should directly lead to a fix in your code.

It is illogical to serialise the database object and the exception leads directly to a fix.

I know it's a little bit of scope creep but lets update Drupal\Core\Site\Settings to throw a LogicException and use a similar message.

Sounds good. In IRC we also discussed the potential for these exceptions turning into assertions later depending on if those get green-lighted, since these are very clearly "dude, your code is broken, fix it" situations that should not be possible at runtime.

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 3a6f5da and pushed to 8.0.x. Thanks!

Shouldn't all classes with references to \Drupal\Core\Database\Connection implement __sleep() to exclude that reference?

Arla: They should either 1) not be serialized, because they're a service ("do" object) rather than an entity or value object ("be" object); or 2) Use the DependencySerializationTrait, which is our hacky workaround for failure to do option 1. The __sleep() here is to improve error messaging, essentially.

Couple of follow ups from this
#2481731: Cannot inject DBLog instances into forms - The database connection is not serializable
#2481729: Cannot inject DatabaseQueue instances into forms - The database connection is not serializable

The exception introuduced in __sleep() causing some error on form rendering. Created an issue for this at #2502195: \Drupal\Core\StringTranslation\Translator\CustomStrings should be serializable, but contain \Drupal\Core\Site\Settings which is not

Looks we need follow-up to never print DB credentials to error output because now if limit mysql max_connections you will see:

$ drush @d8 cr
exception 'PDOException' with message 'SQLSTATE[HY000] [1040] Too many connections' in /home/andypost/www/core8/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php:92                                                [error]
Stack trace:
#0 /home/andypost/www/core8/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php(92): PDO->__construct('mysql:host=loca...', 'd8', 'd8', Array)

d8/d8 is displayed

Seems this still exists e.g. when we try to use comment in a block, then preview of the comment provides that error
& Log says about the LogicException

The database connection is not serializable. This probably means you are serializing an object that has an indirect reference to the database connection. Adjust your code so that is not necessary. Alternatively, look at DependencySerializationTrait as a temporary solution. in Drupal\Core\Database\Connection->__sleep()

@roynilanjan can you please open a new issue with complete steps to reproduce - I tried to reproduce by guessing that you were adding a comment field to a content block but that didn't cause this.

@alexpott, Issue raise here https://www.drupal.org/node/2620346 along the steps

I realize this is a very old, closed, issue, but the patch #29 indeed leads to lots of follow-up problems with random classes implementing DependencySerializationTrait.

I created a meta issue with a list of issues reporting this problem here: #3400483: [meta] LogicException: The database connection is not serializable (for string translations in Ajax callback),
and proposed a solution in this old issue with many followers: #2987548: LogicException: The database connection is not serializable..