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.
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
Comment | File | Size | Author |
---|---|---|---|
#34 | drupal-1953800-34.patch | 29.26 KB | dawehner |
#34 | interdiff.txt | 4.27 KB | dawehner |
#31 | drupal-1953800-31.patch | 27.99 KB | dawehner |
#31 | interdiff.txt | 2.78 KB | dawehner |
#28 | drupal-1953800-28.patch | 26.88 KB | amateescu |
Comments
Comment #1
dawehnerThis 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.
Comment #2
dawehnerchx had the wonderful idea to inject the PDO connection into the connection object.
Running an existing site works, install drupal still fails.
Comment #4
dawehnerNice, it starts to actually install now (on mysql), though this really need help for pgsql, and sqlite.
Comment #6
dawehnerChx had the idea to remove all kind of changes, which aren't required in this patch.
Comment #8
yched CreditAttribution: yched commentedRelated proposal: #1977206: Default serialization of ConfigEntities - might not be a good idea for Views though, not sure.
Comment #9
dawehnerIn order to install drupal you need at least these changes!
Comment #11
dawehnerPosted the interdiff of previous change.
:(
Comment #13
dawehnerCleaned up the patch a bit, though I still could not figure out why the tests aren't running.
Comment #15
amateescu CreditAttribution: amateescu commentedHere you go :)
Comment #17
amateescu CreditAttribution: amateescu commentedThis one should fix that error and also clean it up a bit.
Comment #18
dawehnerWe 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?
Comment #20
amateescu CreditAttribution: amateescu commentedAnd here's another update that actually does what this issue was intended for, handle serialization and deserialization of Connection objects.
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.
Comment #21
dawehnerLet's document that a bit better. I tried to write also a test with an actual form, but for some reason failed.
Comment #23
Crell CreditAttribution: Crell commentedSeems like this should be an abstract method, to force implementers to implement it.
Nitpick: It's probably better to use a keyed array, as it's more self-documenting than list() when unserializing. Just a better practice.
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.
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?
Comment #24
amateescu CreditAttribution: amateescu commentedOk, this one should be better. Fixed the serialization and unserialization code and also improved the tests.
Because they are now called from a static method (Drupal\Core\Database\Driver\sqlite\Connection::open()).
Suggestions welcome :)
Comment #26
tim.plunkettSince this means we have to kneecap all of the controller conversions (which isa critical), this also deserves to be critical.
Comment #27
Crell CreditAttribution: Crell commentedWe probably want to also exclude things like the schema object, and anything else that was lazy-instantiated. We can just lazy-instantiate those again.
Comment #28
amateescu CreditAttribution: amateescu commentedSure thing.
Comment #29
larowlanBlocks #1974492: Convert taxonomy_overview_terms to a Form Controller
Comment #30
larowlanAnd therefore #1974210: Convert admin/structure/forum to a Controller
Comment #31
dawehnerComment #32
Crell CreditAttribution: Crell commentedI 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.
Comment #33
alexpottThis patch break overriding PDO options...
Using mode context this should be...
This needs fixing for the same reason...
This needs fixing for the same reason...
Here's why...
And the output is...
Whereas I think the expected output is
And since this is breakable (as proved by the patch) and I think quite easy to test... we should add a test for it.
Comment #34
dawehnerSo what about a test like that?
Comment #35
tim.plunkettNice test! Back to RTBC
Comment #36
alexpottCommitted 5665307 and pushed to 8.x. Thanks!
Comment #37
dcrocks CreditAttribution: dcrocks commentedThis fix introduced errors during install when SQLite was chosen as the database. See #1998366: [meta] SQLite is broken. A fix will be generated there.
Comment #39
amateescu CreditAttribution: amateescu as a volunteer commentedNoting 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.