Problem/Motivation
#2251795: Injected Settings may be serialized + unserialized later, not reflecting current settings.php values figured out that we should not serialize() information which can be determined by settings.php because otherwise changing settings.php
doesn't change it.
Proposed resolution
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#16 | interdiff.txt | 2.47 KB | dawehner |
#16 | 2252033-16.patch | 3.46 KB | dawehner |
#14 | interdiff.txt | 2.2 KB | dawehner |
#14 | 2252033-14.patch | 2.47 KB | dawehner |
#7 | 2252033-7.patch | 1.84 KB | olli |
Comments
Comment #1
olli CreditAttribution: olli commentedComment #4
jibranThis make sense to me but I don't have much expertise in this area. I think @chx can maybe review the patch.
Comment #6
sunIt's not clear to me why we're serializing anything at all.
What in a PDO/Connection object can be serialized/unserialized?
All we should need is a pointer to the connection to use (pseudo code):
Perhaps that means that we shouldn't pass around actual PDO connection objects, but wrapper objects instead?
Comment #7
olli CreditAttribution: olli commentedReroll.
Comment #8
olli CreditAttribution: olli commentedComment #9
dawehnerNote: The connection object no longer extends the PDO connection but wraps it, but yeah
For example things like the prefix also depends on the settings.php and driver_class
Comment #10
jhedstromDoes #9 mean this needs work? Patch in #7 still applies.
Comment #11
olli CreditAttribution: olli commentedRe #10: yes #9 sounds like more work.
I think this is critical like #2251795: Injected Settings may be serialized + unserialized later, not reflecting current settings.php values because we are storing db parameters (host, password, ..) in db and changing them in settings.php don't affect previously serialized connection objects that would be using another pdo with previous values.
Comment #12
dawehnerAdded a related issue
Comment #13
dawehner@olli
Its great that you found out this problem, good catch!
Comment #14
dawehnerExpanded the test coverage and covered the additional properties.
Comment #15
Fabianx CreditAttribution: Fabianx commentedThis is a little weak test, but given that both connection and connectionOptions are in there and it passes it _should_ be okay.
Might be better to check for it with '' to at least delimit properly.
Leaving at code needs review for now, but tending to RTBC.
Comment #16
dawehnerThat is a good idea.
On top of that I also wrote a test which actually changes the db settings under the hood and ensure they change after the serialize().
Comment #17
Fabianx CreditAttribution: Fabianx commentedGreat test! RTBC - if tests pass!
Comment #18
dawehnerThank you!
Comment #19
catchNice find. Committed/pushed to 8.0.x, thanks!
Comment #22
xjm