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.
right now we store data for a given config xml file in the same formate as we store it on disc.
this is slow and unnecessary - we should just store it in the active store as a serialised php array, which is much quicker and simpler code.
Comment | File | Size | Author |
---|---|---|---|
#25 | config.format.23.patch | 10.51 KB | alexpott |
#25 | interdiff.txt | 2.22 KB | alexpott |
#24 | interdiff.txt | 2.63 KB | chx |
#21 | config.format.21.patch | 10.93 KB | alexpott |
#20 | config.format.20.patch | 10.66 KB | sun |
Comments
Comment #1
gddClarifying title. FYI I think this is a good idea. It will be quicker and we will be less reliant on our serializiation/deserialization code from a performance perspective.
Comment #2
catchYes I only let the original patch go in because I assumed we'd switch over to this :)
Comment #3
philippejadin CreditAttribution: philippejadin commentedComment #4
JamesK CreditAttribution: JamesK commentedEach database driver needs to be able to define their own method of handling configuration storage. For an RDBMS storing a serialized array makes sense, but for a key-value store storing a serialized array would be nonsense.
Comment #5
gddWell, no matter what storage, its going to be a serialized array. It may be PHP or XML or JSON or whatever, but one way or another you're serializing an array down to a specific format. That's not going to change at this point.
Comment #6
JamesK CreditAttribution: JamesK commentedThe issue summary says "serialised php array" so I'm just pointing out that serializing the data into a string doesn't make sense for every storage system.
Comment #7
catchThis issue is about the default database storage driver only, should be zero effect on other drivers either way.
Comment #8
alexpottA possible solution for this exists over in #1470824: XML encoder can only handle a small subset of PHP arrays, so switch to YAML and it also might be able to allow for different storage options based on database type through using dependency injection to replace the DrupalVerifiedStorageSQL class.
Comment #9
alexpottFirst effort at moving config encode / decode out of the config interface layer which will allow the DatabaseStorage class to use php serialize. The attached patch takes the following approach:
Comment #11
alexpottFixed copying config to database and files directory on module enable.
Comment #12
alexpottImproved documentation of the config classes and interfaces
Comment #13
sunHm. Yes, we need to untangle the synchronization interface methods from storage controllers. But it might be a good idea to do that only after #1447686: Allow importing and synchronizing configuration when files are updated. That patch stumbled over the StorageInterface methods already and additionally introduces a second notion of how FileStorage and DatabaseStorage controllers may be used. The StorageInterface changes should most probably also be discussed in #1560060: [meta] Configuration system roadmap and architecture clean-up first.
That said, I'm not sure why these changes are necessary for this issue/patch in the first place? We should be able to move the encoding/decoding operations into the storage controllers without the other architectural changes.
Comment #14
sunUnless I'm terribly mistaken, this should do it.
Code lives in the cmi/config-format-1500238-sun branch (based on config-format-base).
Comment #16
gdd#14: config.format.14.patch queued for re-testing.
Comment #18
sunFixed the import of module default configuration.
Comment #20
sunFixed ConfigFileSecurityTestCase.
Comment #21
alexpottNew patch improves some comments and a couple of code tidy ups.
Comment #24
chx CreditAttribution: chx commentedComment #25
alexpottPatch re-rolled after comments from sun on irc: "revert the additional phpDoc fixes" - "because those custom methods will go away entirely, and only the StorageInterface will remain"
Interdiff is for patch in #20
Comment #26
chx CreditAttribution: chx commentedNice, very nice. The API is so much better! + $data = $this->encode($data); instead of config_encode that's how the world should be, truly.
Comment #27
rfayThis seems to cause an infinite loop in the tests. I interrupted the test.
Status: Running tests, 495,389 assertion(s).
Comment #28
rfayYes, probably was #22 (http://qa.drupal.org/pifr/test/269358) that caused the infinite loop. Setting back.
Comment #29
rfay#25: config.format.23.patch queued for re-testing.
Comment #30
catchThis looks great. Committed/pushed to 8.x.