Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The SQLite and PostreSQL database drivers are doing some important things in their connection object constructor (e.g. altering the db prefix for SQLite, executing 'init_commands' for PostgreSQL) but Drupal\Core\Database\Connection::unserialize()
is only doing the job of its own constructor by duplicating the code.
Proposed resolution
Call the connection object constructor instead of forcing each driver to duplicate its code.
Remaining tasks
Review, commit.
User interface changes
Nope.
API changes
Nope.
Comment | File | Size | Author |
---|---|---|---|
#26 | interdiff.txt | 534 bytes | amateescu |
#26 | 2463321-26.patch | 14.61 KB | amateescu |
#24 | interdiff.txt | 1.66 KB | amateescu |
#24 | 2463321-24.patch | 14.09 KB | amateescu |
#20 | interdiff.txt | 801 bytes | amateescu |
Comments
Comment #1
amateescu CreditAttribution: amateescu commentedThis makes
Drupal\system\Tests\Database\ConnectionTest
pass on SQLite.Comment #2
Crell CreditAttribution: Crell at Palantir.net commentedFalse. If code from the constructor needs to be called under other circumstances, move it out of the constructor and call from both places.
Comment #3
dawehnerSo call it something like init() and do all the logic in there?
Comment #4
amateescu CreditAttribution: amateescu commentedOk, let's do that.
Comment #5
amateescu CreditAttribution: amateescu commentedI forgot to update a stale comment.
Comment #7
amateescu CreditAttribution: amateescu commentedComment #8
dawehnerI think we should not call it statically ... Sometimes PHP is just weird.
Comment #9
amateescu CreditAttribution: amateescu commentedGood catch :)
Comment #10
amateescu CreditAttribution: amateescu commentedLet's see if @Crell is happy with the latest patch.
Comment #11
Crell CreditAttribution: Crell at Palantir.net commentedI 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...
Comment #12
amateescu CreditAttribution: amateescu commentedThat's already the case since #1953800: Make the database connection serializable.
Of course, there's always the option to spend a few hours and fix it properly :) This basically reverts that patch and uses the dependency serialization stuff which was added a month later.
Comment #13
amateescu CreditAttribution: amateescu commentedComment #14
Crell CreditAttribution: Crell at Palantir.net commentedUgh. 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?
Comment #15
amateescu CreditAttribution: amateescu commentedThere's no new dependency on Drupal for the DB system in this patch, only
Drupal/Core/KeyValueStore/DatabaseStorage
,Drupal/Core/Session/SessionHandler
andDrupal/Core/Session/SessionManager
need it. But yes, the deep tree is (somewhat) fixed :)Comment #16
dawehnerI 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.
I'm a bit confused, does that mean every class getting the DB connection injected need that Trait?
Comment #17
Crell CreditAttribution: Crell at Palantir.net commenteddawehner: 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.)
Comment #18
amateescu CreditAttribution: amateescu commentedI like that exception message!
Comment #19
Crell CreditAttribution: Crell at Palantir.net commentedLOL. 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.
Comment #20
amateescu CreditAttribution: amateescu commentedHeh, that patch was just a cry for help with a better exception message :)
Comment #21
dawehnerIt looks great for me now!
Comment #22
webchickI'd feel better about Alex reviewing this one, given the possible security implications.
Comment #23
alexpottIn
Drupal\Core\Site\Settings
we throw aBadMethodCallException
which I think is wrong since the PHP docs are:I think what we're dealing with here is a
LogicException
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.Comment #24
amateescu CreditAttribution: amateescu commentedWhy not :) Something like this?
Comment #26
amateescu CreditAttribution: amateescu commentedFixed that.
Comment #27
Crell CreditAttribution: Crell at Palantir.net commentedSounds 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.
Comment #28
alexpottThis 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!
Comment #30
ArlaShouldn't all classes with references to \Drupal\Core\Database\Connection implement __sleep() to exclude that reference?
Comment #31
Crell CreditAttribution: Crell at Palantir.net commentedArla: 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.
Comment #32
larowlanCouple 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
Comment #34
vijaycs85The 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 notComment #35
andypostLooks we need follow-up to never print DB credentials to error output because now if limit mysql max_connections you will see:
d8/d8 is displayed
Comment #36
amateescu CreditAttribution: amateescu commentedHm.. that doesn't look related to the patch from this issue.
Comment #37
roynilanjan CreditAttribution: roynilanjan commentedSeems 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()
Comment #38
alexpott@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.
Comment #39
roynilanjan CreditAttribution: roynilanjan commented@alexpott, Issue raise here https://www.drupal.org/node/2620346 along the steps
Comment #41
johnvI 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..