Posted by beejeebus on March 25, 2012 at 11:08pm
18 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | configuration system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Configuration system |
Issue Summary
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.
Comments
#1
Clarifying 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.
#2
Yes I only let the original patch go in because I assumed we'd switch over to this :)
#3
#4
Each 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.
#5
Well, 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.
#6
The 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.
#7
This issue is about the default database storage driver only, should be zero effect on other drivers either way.
#8
A 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.
#9
First 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:
#10
The last submitted patch, cmi-serialize-1500238-9.patch, failed testing.
#11
Fixed copying config to database and files directory on module enable.
#12
Improved documentation of the config classes and interfaces
#13
Hm. 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.
#14
Unless I'm terribly mistaken, this should do it.
Code lives in the cmi/config-format-1500238-sun branch (based on config-format-base).
#15
The last submitted patch, config.format.14.patch, failed testing.
#16
#14: config.format.14.patch queued for re-testing.
#17
The last submitted patch, config.format.14.patch, failed testing.
#18
Fixed the import of module default configuration.
#19
The last submitted patch, config.format.15.patch, failed testing.
#20
Fixed ConfigFileSecurityTestCase.
#21
New patch improves some comments and a couple of code tidy ups.
#24
#25
Patch 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
#26
Nice, very nice. The API is so much better! + $data = $this->encode($data); instead of config_encode that's how the world should be, truly.
#27
This seems to cause an infinite loop in the tests. I interrupted the test.
Status: Running tests, 495,389 assertion(s).
#28
Yes, probably was #22 (http://qa.drupal.org/pifr/test/269358) that caused the infinite loop. Setting back.
#29
#25: config.format.23.patch queued for re-testing.
#30
This looks great. Committed/pushed to 8.x.
#31
Automatically closed -- issue fixed for 2 weeks with no activity.