Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
The filled and bare database dumps have some router entries with broken serialized data:
Bare:
<current>
comment.approve
entity.comment.canonical
entity.comment.delete_form
entity.comment.edit_form
Filled:
<current>
comment.approve
content_translation.translation_add_comment
content_translation.translation_delete_comment
content_translation.translation_edit_comment
entity.comment.canonical
entity.comment.content_translation_overview
entity.comment.delete_form
entity.comment.edit_form
Proposed resolution
Correct the entries.
Remaining tasks
Test/review.
User interface changes
N/A
API changes
N/A
Data model changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#24 | update-route_serialization_broken-2565259-24.patch | 227.37 KB | plach |
#15 | interdiff.txt | 1.5 KB | dawehner |
Comments
Comment #2
Antti J. Salminen CreditAttribution: Antti J. Salminen as a volunteer commentedComment #3
Antti J. Salminen CreditAttribution: Antti J. Salminen as a volunteer commentedSorry for the duplicate comment, d.o or my connection was acting up...
Comment #4
catchThis would explain some of the many red herrings we ran into when debugging #2561121: Installer fails on postgresql due to uncaught exception and other issues.
Given the hours spent debugging those issues, many of them playing whack-a-mole with that serialisation error, bumping this to critical since it makes our upgrade path tests very untrustworthy.
Comment #5
dawehnerI'm curious whether this is the only broken route?
Comment #6
dawehner* Imported all the routes from the script
* Used this little script:
Comment #7
Wim LeersWow, amazing find!
Comment #8
stefan.r CreditAttribution: stefan.r commented@dewehner so the filled dump is fine and the bare dump has just those 5 additional broken routes?
Uploading a patch that includes those 5 manually replaced in addition to the ones in #1. Alternatively we could also just do a completely new beta12 dump...
Comment #9
Antti J. Salminen CreditAttribution: Antti J. Salminen as a volunteer commentedI did a couple of diffs between the bare dump scripts generated.
A diff of a newly made bare beta12 dump with and without the patch (attached as fresh_bare_to_patched_diff.txt) only differed for the routes.
A diff for the fixed dump versus what is currently in HEAD (bare_to_patched_diff.txt), has a lot more noise from UUIDs etc. in the diff but going thru it in ediff I didn't see any real differences besides the routes.
In the filled dump dawehner's script from #6 revealed the same 5 and these additional routes failing the unserialize:
Comment #11
alexpottThe test fails in
Drupal\config\Tests\ConfigInstallProfileUnmetDependenciesTest
look like testbot issues - going to press retest.Also I think this patch should include #2565225: Database dump script garbles the script contents by using an output formatter - not sure why we have two issues?
Comment #13
alexpottAlso since this was able to occur I guess we should add some test coverage.
Comment #15
dawehnerWrote some basic test coverage for that, and indeed these are the routes which cause issues.
Comment #17
stefan.r CreditAttribution: stefan.r commentedShould we reset the error handler after the router table check? I think we can also include #8 and #2565225: Database dump script garbles the script contents by using an output formatter in the patch after we see the failure messages are correct - not sure why in the CI results it is not listing the route names, and only one failure instead of the 6 in total we found.
Also do we need further test coverage than this, i.e. check other columns that may hold serialized data?
Comment #19
plachWorking on this a bit
Comment #20
dgv CreditAttribution: dgv commented@catch:
Exactly. The whack-a-mole endless game is the metaphor I wanted to use to reply in
https://www.drupal.org/node/2565529
Drupal needs to take a stand about whether in its code, "standard_conforming_strings" is ON or OFF with PostgreSQL, set that as a rule, and only then find and fix all the code that does not conform to that rule.
Right now what we're doing is backwards, changing the rule when code is found that does not conform to the rule, only to find that another test or code depended on the opposite interpretation.
Comment #21
dawehner@plach
Do you mind putting restore_error_handler() and the end here?
Comment #22
MixologicBecause you discovered a new bug in drupalCI - Thank you!
https://www.drupal.org/node/2566265
Comment #23
plachOk, this is green locally. It also includes the patch from #2565225: Database dump script garbles the script contents by using an output formatter as suggested by @alexpott.
I also manually tested the resulting dumps and they were working fine.
Aside from the PG issue we should good here, I think.
Comment #24
plachSorry, wrong patch. The previous interdiff should be ok, though.
Comment #26
stefan.r CreditAttribution: stefan.r commentedLooks good, I guess this is enough test coverage then?
Comment #27
plachYep, thanks
Comment #28
plachThe dump diffs as per @dawehner's request.
Comment #29
dawehnerThank you @plach!
These are indeed the routes I saw when running the tests in #15
Given that I think this should be ready to go.
Comment #32
alexpottCommitted 3444108 and pushed to 8.0.x. Thanks!