1. Introducing KeyValue/FileStorage.
  2. Demonstration: Replacing State with FileStorage.

Objective

  1. The entire point of rewriting CMI 2-3 times was to decouple the storage from the application-level configuration concept.
  2. The underlying storage is nothing else than a key/value store.

Details

  1. The custom concept of a "ConfigStorageInterface" should not exist to begin with.
  2. The current FileStorage is a key/value store, because key=filename, value=string, collection=subpath.
  3. 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

  1. 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.

  2. 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.

Files: 
CommentFileSizeAuthor
#33 kv.file_.33.patch73.89 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch kv.file_.33.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

Status: Needs review » Needs work

The last submitted patch, keyvalue.file_.0.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
4.9 KB
FAILED: [[SimpleTest]]: [MySQL] 57,745 pass(es), 3,544 fail(s), and 857 exception(s). View
697 bytes

Fixed fatal errors caused by stdClass::__set_state().

Status: Needs review » Needs work

The last submitted patch, 2: keyvalue.file_.2.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
5.21 KB
FAILED: [[SimpleTest]]: [MySQL] 58,657 pass(es), 3,937 fail(s), and 806 exception(s). View
3.69 KB

Default 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()).

Status: Needs review » Needs work

The last submitted patch, 4: kv.file_.4.patch, failed testing.

sun’s picture

4: kv.file_.4.patch queued for re-testing.

The last submitted patch, 4: kv.file_.4.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
Issue tags: +KeyValueStore
FileSize
45.28 KB
FAILED: [[SimpleTest]]: [MySQL] 48,559 pass(es), 444 fail(s), and 1,373 exception(s). View

Here we go:

A fully working replacement of Config\FileStorage with KeyValueStore\FileStorage.

Including cleanly injected serialization formats: YAML, JSON, XML, PHP serialize().


Please disregard the KeyValueFileFactory - it is not used.

Status: Needs review » Needs work

The last submitted patch, 8: kv.file_.8.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
46.89 KB
FAILED: [[SimpleTest]]: [MySQL] 62,209 pass(es), 119 fail(s), and 56 exception(s). View
2.36 KB
  1. Fixed DrupalUnitTestBase.
  2. Fixed bogus addition of folders in InstallStorage.
  3. Fixed ViewTestData.

Status: Needs review » Needs work

The last submitted patch, 10: kv.file_.10.patch, failed testing.

The last submitted patch, 10: kv.file_.10.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
57.65 KB
FAILED: [[SimpleTest]]: [MySQL] 62,910 pass(es), 80 fail(s), and 23 exception(s). View
24.87 KB
  1. Fixed Config tests.
  2. Reverted removal of Boolean return values from CachedStorage.
  3. Fixed config_test module TestInstallStorage and TestSchemaStorage.
  4. Fixed false-positive matches in InstallStorage::getPathname() in case of multiple files with same prefix.
  5. Added KeyValueStore/FileStorageTest.

Status: Needs review » Needs work

The last submitted patch, 13: kv.file_.13.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
67.84 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch kv.file_.15.patch. Unable to apply patch. See the log in the details link for more information. View
13.12 KB
  1. Added missing InvalidDataTypeException.
  2. Added KeyValueStoreInterface::has().
  3. Updated tests.
  4. Use filtered FilesystemIterator instead of GlobIterator to avoid absolute path problem on Windows.

Status: Needs review » Needs work

The last submitted patch, 15: kv.file_.15.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
67.65 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Status: Needs review » Needs work

The last submitted patch, 17: kv.file_.17.patch, failed testing.

The last submitted patch, 17: kv.file_.17.patch, failed testing.

The last submitted patch, 17: kv.file_.17.patch, failed testing.

The last submitted patch, 17: kv.file_.17.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
68.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
1.56 KB

I'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... :-/

  1. Updated UpdateModuleHandler.
  2. Removed keyvalue.file service for now, since nonsensical.

Status: Needs review » Needs work

The last submitted patch, 22: kv.file_.21.patch, failed testing.

jthorson’s picture

22: kv.file_.21.patch queued for re-testing.

The last submitted patch, 22: kv.file_.21.patch, failed testing.

jthorson’s picture

22: kv.file_.21.patch queued for re-testing.

The last submitted patch, 22: kv.file_.21.patch, failed testing.

jthorson’s picture

WD php: Drupal\\field\\FieldException: Attempt to create an instance of [error]
field user_picture that does not exist on entity type user. in
Drupal\\field\\Entity\\FieldInstanceConfig->__construct() (line 258 of
/var/lib/drupaltestbot/sites/default/files/checkout/core/modules/field/lib/Drupal/field/Entity/FieldInstanceConfig.php).
Drupal\\field\\FieldException: Attempt to create an instance of field user_picture that does not exist on entity type user. in Drupal\\field\\Entity\\FieldInstanceConfig->__construct() (line 258 of /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/field/lib/Drupal/field/Entity/FieldInstanceConfig.php).
Drush command terminated abnormally due to an unrecoverable error. [error]

sun’s picture

FYI: There appears to be an undocumented/magic difference in the order of files returned by GlobIterator vs. 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:

  1. I'm not able to reproduce the error on my local [Windows] box — the installer + everything else works fine with this patch applied.
  2. The error appears to be caused by a lack of proper config dependency resolution — #2080823 will hopefully be ready + land soon.
  3. As soon as that problem is out of the way, the current patch here should pass >90% of all tests (like #13 before).

I explicitly do not want to go back to #13, because GlobIterator does not work with relative paths on Windows, and realpath() returns FALSE if the given filesystem path does not exist, and thus, $this->path = FALSE would train-wreck into the entire FileStorage implementation, destroying proper error messages, and more importantly, realpath() disallows usage of stream wrappers.

→ A generic k/v FileStorage MUST support stream wrappers, period.

sun’s picture

Status: Needs work » Needs review
FileSize
73.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,385 pass(es). View
24.9 KB

But in order to move forward, temporarily going back to a GlobIterator.

sun’s picture

markpavlitski’s picture

30: kv.file_.30.patch queued for re-testing.

sun’s picture

FileSize
73.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch kv.file_.33.patch. Unable to apply patch. See the log in the details link for more information. View

Merged 8.x.

sun’s picture

sun’s picture

Status: Needs review » Needs work

The last submitted patch, 33: kv.file_.33.patch, failed testing.

sun’s picture

Damien Tournoud’s picture

Just 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.

sun’s picture

Resurrected #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 FilesystemIterator and GlobIterator made 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 FileStorage itself into an own issue, too...)

markpavlitski’s picture

There 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).

markpavlitski’s picture

Also 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.

chx’s picture

As 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!

Crell’s picture

Re #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...

catch’s picture

With #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.

chx’s picture

> was throwing an exception on other types

array('foo' => new stdClass) and now you are screwed. #42 states

the data is recursive arrays and nothing else

Emphasis added.

andypost’s picture

Version: 8.0.x-dev » 8.1.x-dev
Assigned: sun » Unassigned
Issue tags: +Needs issue summary update

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.