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.- Introducing KeyValue/FileStorage.
- Demonstration: Replacing State with FileStorage.
Objective
- The entire point of rewriting CMI 2-3 times was to decouple the storage from the application-level configuration concept.
- The underlying storage is nothing else than a key/value store.
Details
- The custom concept of a "ConfigStorageInterface" should not exist to begin with.
- The current FileStorage is a key/value store, because key=filename, value=string, collection=subpath.
- There should only be one concept of a key/value store in Drupal and it should be used wherever applicable. Let's ensure that the configuration system does not re-invent that wheel.
Notes
-
The encoder/decoder for the serialization format should simply be injected, the storage doesn't care.
I wanted to suggest Drupal\Core\Serialization first, but then I wondered how the REST/Serialization module is handling serialization formats. I learned that we apparently have the Symfony Serializer component in core and REST/Serialization is a simple wrapper around that (for REST-specific purposes, not sure I get why those are separate modules...)
→ Use whichever serialization encoder you want to use. The key/value store does not and should not care.
-
In case Serializer is too slow/big/complex of a dependency, then let's simply do Drupal\Core\Serialization and provide standard implementations for serialize/YAML/JSON/YML that can be injected.
| Comment | File | Size | Author |
|---|---|---|---|
| #33 | kv.file_.33.patch | 73.89 KB | sun |
Comments
Comment #2
sunFixed fatal errors caused by stdClass::__set_state().
Comment #4
sunDefault DatabaseStorage introduced an implicit assumption that any KeyValueStore supports to store entire objects by using serialize() for all values (instead of e.g. json_encode()).
Comment #6
sun4: kv.file_.4.patch queued for re-testing.
Comment #8
sunHere we go:
A fully working replacement of
Config\FileStoragewithKeyValueStore\FileStorage.Including cleanly injected serialization formats: YAML, JSON, XML, PHP serialize().
Please disregard the
KeyValueFileFactory- it is not used.Comment #10
sunComment #13
sunComment #15
sunComment #17
sunComment #22
sunI'm not able to reproduce the installation failure reported by the testbot. @webchick was so kind to additionally test this patch in a PHP 5.3.18 environment, and installation also succeeded there. I don't really understand why the testbot suddenly shows this failure... :-/
Comment #24
jthorson CreditAttribution: jthorson commented22: kv.file_.21.patch queued for re-testing.
Comment #26
jthorson CreditAttribution: jthorson commented22: kv.file_.21.patch queued for re-testing.
Comment #28
jthorson CreditAttribution: jthorson commentedComment #29
sunFYI: There appears to be an undocumented/magic difference in the order of files returned by
GlobIteratorvs.FilesystemIterator, which causes this patch to fail on testbots — but NOT on my local Windows box.I intensively debugged this error in #1766036: ♪♫ Question the sun ♥, the moon, and the stars ♫♪ [Test that stuff.]
The error in #28 essentially means that the installer tries to install the user_picture field instance before the field.
Right now, the entire config installation process entirely depends on alphabetic file ordering, which, alas, is anything but guaranteed, as proven by this patch — proper configuration dependency resolution appears to be attacked and resolved by #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall
Therefore, my current status quo is this:
I explicitly do not want to go back to #13, because
GlobIteratordoes not work with relative paths on Windows, andrealpath()returnsFALSEif the given filesystem path does not exist, and thus,$this->path = FALSEwould train-wreck into the entireFileStorageimplementation, destroying proper error messages, and more importantly,realpath()disallows usage of stream wrappers.→ A generic k/v
FileStorageMUST support stream wrappers, period.Comment #30
sunBut in order to move forward, temporarily going back to a
GlobIterator.Comment #31
sunCreated #2208609: Move Json utility class into Drupal\Component\Serialization
Comment #32
markpavlitski CreditAttribution: markpavlitski commented30: kv.file_.30.patch queued for re-testing.
Comment #33
sunMerged 8.x.
Comment #34
sunCreated #2216459: Add a KeyValueStoreInterface::has() method
Comment #35
sunCreated #2216527: Inject a serialization format into database key/value storage
Comment #37
sunCreated #2216579: Add a KeyValueStoreInterface::rename() method
Comment #38
Damien Tournoud CreditAttribution: Damien Tournoud commentedJust pointing out that GlobIterator is documented as extending FileStorage which (if true), would make me doubt that the ordering would be different between the two.
Comment #39
sunResurrected #2161643: Add a KeyValueStore\IterableKeysInterface (allowing to retrieve keys with a given prefix)
@Damien: Yeah, I'm totally aware of that. Solely based on PHP docs, I'd agree.
However, my extensive debugging mentioned in #29 did literally probe for all possible vectors. Only the switch between
FilesystemIteratorandGlobIteratormade a difference.The difference appears to be OS-specific, as I'm not able to reproduce the failures reported by the testbot on my local Windows box in any way. All of the failing tests pass for me with
FilesystemIterator.The verbose testbot error output (kindly digged out + provided by @jthorson) essentially says the same: Files are returned in a different order.
In any case, upon @alexpott's request, I'm separating the individual change proposals into discrete issues that can be reviewed more easily. That process is done with this comment. (Although I'm not sure yet whether it wouldn't make sense to split the addition of the
FileStorageitself into an own issue, too...)Comment #40
markpavlitski CreditAttribution: markpavlitski commentedThere are differences in the system implementation of glob() on Windows vs Linux; I've experienced issues with different results (using complex patterns) and different/reversed ordering.
On Linux, PHP glob(), glob:// stream wrappers, and GlobIterator's relies on system glob() in glibc (see: man glob).
On Windows, it uses a custom implementation built into the PHP binary instead (see: php-src/win32/glob.c).
Comment #41
markpavlitski CreditAttribution: markpavlitski commentedAlso in response to #29, note that GlobIterator seems to use a glob:// stream wrapper internally, so I'm not sure how this would interact with Drupal stream wrappers. I think it expects a simple local path.
Comment #42
chx CreditAttribution: chx commentedAs it is usual it is almost impossible to follow which CMI issue does what and where should one chime in so I am repeating myself here. If this is not the place to speak up, tell me where and I will post this there as well gladly. Better than being burned like of old.
I will say that going key-value store is a bad idea unless somehow the config-ness (that the data is recursive arrays and nothing else) is communicated to the store -- this is necessary for the store to store config for config entities in a smart way that helps config entity queries to be fast. You can't do that if you know nothing about the data you store, a generic k-v store can only run serialize over the data. Don't dumb down more than you need to!
Comment #43
Crell CreditAttribution: Crell commentedRe #42, would that be the responsibility of the API to communicate, or whoever is setting up the KV store in the first place?
Now that the active store is in the DB only (and, presumably, swappable or soon will be), it seems logical to my uninformed eye that most people will want to use a proper KV DB for active config rather than SQL if they have the opportunity to do so. If so, we should make it as easy as possible to configure said store "correctly" (for some definition of correctly).
I don't know what that means for this issue, though...
Comment #44
catchWith #42, we've had exactly that issue with #2228261: Add a local, PhpStorage-based cache backend as well. The issue is that a generic store will work 'fine', but then is impossible to optimize for the more specific use case because there's no decent way to communicate the restriction. It's worse in the case of cache bins since we don't enforce that non-config/discovery shouldn't write to those bins, which could then be any type. Haven't reviewed the patch here yet but at least it'll be a seperate service even if the same interface.
I think we need a way for the implementation itself to communicate that it can only store arrays - the only idea we came up with in the config/discovery cache issue was throwing an exception on other types - which is a bit harsh if that's the first you hear about it. Extra interface would help slightly.
Then there also needs to be a way to indicate where it's actually OK to use that implementation.
In the end I think it'd be down to site admins to wire up the optimized version vs. the generic version, but there needs to be a mechanism to indicate what's possible.
Comment #45
chx CreditAttribution: chx commented> was throwing an exception on other types
array('foo' => new stdClass)and now you are screwed. #42 statesEmphasis added.
Comment #46
andypostComment #54
kim.pepper[removed]