Add key value entity storage

Technically it would be a feature request.

Yeah, not sure about the create() flow going through the storage controller.

#2096899: Add $EntityType::create() to simplify creating new entities and #1867228: Make EntityTypeManager provide an entity factory and #1867228: Make EntityTypeManager provide an entity factory are related to that, but I'm also a bit worried about too many public static methods on an entity class (with those two issues, you need to be aware that Node::create() is supposed to be called from the outside but not Node::createInstance().. huh?)

+1,000 — This represents an actual proof that it is possible to use a key/value store for entities in D8. As such, I consider this a task, not a feature.

It looks like the storage controller has to copy/duplicate a lot of code, which we should be able to provide in an abstract base class or trait...? — Given how much common/standard logic code appears to be copied/duplicated here, that would certainly be a very good idea (separate issue).

Is there a particular reason for why revisions are not supported?

Do we actually need to normalize entities? Is a serialization not sufficient? (Related: #2216527: Inject a serialization format into database key/value storage)

1. +++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueEntityStorage.php
   @@ -0,0 +1,270 @@
   +      throw new EntityMalformedException('The entity does not have an ID.');
   

   Can this be expanded, as this behaviour is different to eg DB storage which most are used to. Something like 'The entity does not have an ID, in order to use KeyValueStorage for entities you must provide and ID.'?

2. +++ b/core/lib/Drupal/Core/Entity/KeyValueStore/Query/Query.php
   @@ -0,0 +1,78 @@
   +        return ($a[$field] <= $b[$field]) ? $direction : -$direction;
   

   this will only support a single sort, perhaps we should throw an Exception if two are passed?

Other than that, looks ok to me.
What's the next step - enabling this in more tests to save on DDL - anywhere we don't need revisions?

What's the next step - enabling this in more tests to save on DDL - anywhere
we don't need revisions?

As a result of the ID thing, this only supports config entities, and this won't make them faster :)

As discussed, we could make a Sequence service that by default is a simple wrapper for $database->nextId() to solve that.

I'm not sure exactly where we want to go with this. Core currently already has 4 entity storage implementations (if you cound the old database one and null ;)) but not an alternative config entity storage, as this issue showed on the other hand.

Awesome! :-)

This looks pretty much ready to me — aside from the ID thing being discussed above, I guess...?

1. +++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueEntityStorage.php
   @@ -0,0 +1,270 @@
   + * Provides a key value backend for entities.
   + */
   +class KeyValueEntityStorage extends EntityStorageBase {
   

   Can we add a description to the class to clarify that revisions are not supported currently? (possibly as a @todo even?)

   Ideally plus any other high-level information on potential pitfalls and noteworthy stuff that you think would be worth to share for ensuing ages :-)

2. +++ b/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/KeyValueConfigEntityStorageTest.php
   @@ -0,0 +1,35 @@
   + * Contains \Drupal\system\Tests\KeyValueStore\KeyValueConfigEntityStorageTest.
   ...
   +use Drupal\config\Tests\ConfigEntityTest;
   ...
   +class KeyValueConfigEntityStorageTest extends ConfigEntityTest {
   

   Oha. :) I found the location of this class and cross-component class inheritance a little bit confusing here — but huh, yeah, I assume you wanted to bundle all the related tests into a single spot, and I guess the situation is going to be weird regardless where we place 'em... ;)

   So I think this is fine, even tho a little bit hard to spot

3. +++ b/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/KeyValueContentEntityStorageTest.php
   @@ -0,0 +1,164 @@
   +class KeyValueContentEntityStorageTest extends WebTestBase {
   

   Any particular reason for why this is a WebTest and not DUTB?

   The test doesn't seem to contain any web/requests, just API level assertions? Therefore, it should work as DUTB?

That said, regarding tests, I wonder whether we should actually add one web test that extends an arbitrary existing web test that covers some basic add/edit/delete operations of a common entity that isn't revisionable (e.g., user or comment). — I.e., just extending that existing test, add $modules, and overriding the content entity storage in the test module; the existing test should pass. It would certainly not be a complete coverage, but at least some basic proof that it actually works with non-test-entities, too? (also happy to defer that to a follow-up issue tho)

Discussed some more with @tim.plunkett today:

1. This patch actually presents a working proof of concept for a key value entity storage implementation as-is. — It is awesome that we actually managed to make it possible.

2. Due to current feature constraints of our KeyValueStore backends, it is only able to work with string identifiers (machine names) right now. — However, that is sufficient for config entities.

3. For content entities - or to be more precise, entities using serial IDs - the storage backend must be capable of understanding a concept of nextId():

   1. The consuming code makes an assumption that the keys of stored items are numeric (despite being strings in a hashtable).
   2. It is fine for consuming code to make such an assumption on a particular k/v collection.
   3. A key/value store provides no guarantee on sequential order of keys (or any other order for that matter).
   4. Therefore, the task is: (1) List all keys, (2) sort numerically, (3) nextId() = max() + 1.

   Due to that, #2161643: Add a KeyValueStore\IterableKeysInterface (allowing to retrieve keys with a given prefix) is very closely related:

   The new interface introduced there only focuses on retrieving a filtered sub-set of key/value pairs for now, but technically the requirement here is asking the k/v backend to be capable of iterating over keys, too; just with a slightly different purpose:

   Ignoring values, just focusing on keys, enable nextId() within a collection.

   As such, that could be added to the new IterableKeysInterface in a separate follow-up issue, once it has landed.

4. I think it's awesome that we are actually this close already, and I'm confident that we're able to figure out that last remaining issue regarding nextId().

   I think we can move forward in independent steps - since the current incarnation works at least for config entities already.

   → Enabling it to additionally work for content entities (or rather, entities with serial IDs) would make most sense as a separate follow-up issue.

Thanks! :-)

35: kv-entity-storage-2208617-35.patch queued for re-testing.

Speaking of that and the related issues, this should come with a validation of the id length similar to ConfigEntityStorage, as this is limited to 128 characters right now in HEAD.

45: kv-entity-2208617-45.patch queued for re-testing.

+++ b/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/KeyValueConfigEntityStorageTest.php
@@ -0,0 +1,41 @@
+class KeyValueConfigEntityStorageTest extends ConfigEntityTest {

Why are there no test methods in this class?

Yay, finally green again! :-)

54: kv-entity-2208617-54.patch queued for re-testing.

lets do it.

I think this might merit a change record for the feature addition?

+++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueEntityStorage.php
@@ -0,0 +1,209 @@
+   * Length limit of the entity ID.
+   */
+  const MAX_ID_LENGTH = 128;

I started to ask why we were overriding this, but then realized we aren't -- ConfigEntityStorage specifies its own and content entities in DB storage have serial IDs. :) Right.

I don't think this is in a state to be announced as a feature of its own — as long as content entities aren't supported yet (cf. #34), this implementation only covers <50% (= entities using a machine name string/key as IDs). IMO, we should only announce this when content entities are supported (requires separate follow-up issues).

Thanks @sun. @Berdir said something similar in IRC. Makes sense.

Based on my discussion with @timplunkett earlier this week, I would go even further and say that this is 90% a test-only implementation (and will stay that way), a fancier version of the array based test field storage that we had in 7.x, if you will. (as the issue summary says)

You can't really use it for config entities, as they have to be in config, and it can not scale for content entities that need any kind of querying or even listing...

Read through this a few times and couldn't find much to complain about, looks like it'll come in useful and help us avoid introducing assumptions into the other entity storage.

Committed/pushed to 8.x, thanks!