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.[Berdir] KeyValueEntityStorage just a test implementation really, I hope nobody is going to use it for real data :)
[yched] OK, but nothing really hints that it's a test-only-do-not-use-for-real implementation :-). It's not in a "test" directory or namespace, Drupal\Core\Entity\KeyValueStore makes it kind of "officially endorsed", and no comment or phpdoc say anything about that either
So maybe we should open a separate issue to move that class to a test folder somewhere ?[Berdir] /me points to @timplunkett ;) I know and I'd be happy to do that.
So if @timplunkett approves, let's do that ?
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | 2393751.15.patch | 9.99 KB | alexpott |
| #15 | 12-15-interdiff.txt | 2.76 KB | alexpott |
| #12 | 2393751.12.patch | 7.23 KB | alexpott |
| #6 | 2393751-keyvalue-6.patch | 3.33 KB | tim.plunkett |
Comments
Comment #1
yched CreditAttribution: yched commentedComment #2
Dave ReidI was planning on using it for actual data and had use cases. I would be sad to have this relegated to test code only.
Comment #3
yched CreditAttribution: yched commented@Dave Reid : Fair enough. Any thoughts on #2391829: ContentUninstallValidator relies on the not required ContentEntityStorage::hasData() method then ? :-)
If we want to have EntityStorageInterface::hasData(), an implementation for KeyValueEntityStorage would currently need to use
$this->keyValueStore->getAll(), which would be a perf killer...Comment #4
BerdirYes, this doesn't scale at all, not just hasData() but entity query is basically the same implementation as config entities AFAIK; which means, load them all and then search in PHP.
Let's ask @timplunkett.
Comment #5
BerdirComment #6
tim.plunkett"It's in my way, let's [re]move it" is exactly why I wanted this added.
As @Berdir points out, ConfigEntityStorage is no worse than KeyValueEntityStorage in terms of entity query.
Why not just improve KeyValueStoreInterface? hasData() should be cheap for this.
Also, way to lure me back to d.o from vacation by assigning an issue to me :) Happy New Years!
Comment #7
BerdirSorry and to you to! :)
This solves the hasData() problem, but it does not solve the entity query implementation. The difference to config entities in my opinion is that they have scalability issues in many different ways, like the list UI's not having any pagers, we also commonly load them all for various operations.
That is not obvious for a key value storage, you might expect this to scale. But I think we had this discussion before ;)
Comment #8
tim.plunkettI still don't see how this is worse than config storage right now.
Other than repurposing this issue for the above example patch, I think this issue should be won't fixed.
Comment #9
Gábor HojtsyKeyValueEntityStorage is also used for config entities. As found in #2136559: Config entity admin lists show configuration with overrides (eg. localized), if you attempt to actually use it for config entities, it does not support overrides, such as settings.php, language, etc. which is a huge part of the config system. ConfigEntityStorage works with the config factory to load things through which is where the overrides are implemented. KeyValueEntityStorage is also used in tests direct with the config system and does not implement the config entity storage interface, so if some methods are actually invoked on it, it will fail hard. The tests do not cover those cases which is why it does not fail now. We are about to introduce a config specific subclass at #2136559: Config entity admin lists show configuration with overrides (eg. localized) which would not actually resolve those problems due to the scope of that issue. If this storage is to be considered a real thing not a test, it would need to step up in a big way. It manages to pass some limited tests now which is why many of us look at it as a test implementation not useful for a real scenario.
Comment #10
Gábor HojtsyComment #11
alexpottAnd it does not support config entity dependency management see #2416409-47: Delete dependent config entities that don't implement onDependencyRemoval() when a config entity is deleted
I propose that we remove the ability to use storage classes that don;t extend ConfigEntityStorage. If you want to store configuration in something other than the db then you need to swap out all configuration storage.
Comment #12
alexpottHere is a patch that removes the config entity part of this.
Comment #13
Gábor HojtsyMakes total sense to me.
Comment #15
alexpottFix the test by removing it - it's pointless need we are not swapping out the config entity storage. Added tests for the exception.
Comment #16
tim.plunkettThis needs a new issue title and issue summary, but this effectively does what #1853856-25: Document that ConfigEntityBase and ConfigStorageController are tightly coupled intended over 2 years ago.
Comment #17
tim.plunkettComment #18
alexpottThis is now fixing a bug. Thanks @tim.plunkett for updating the the issue title and summary.
Comment #19
BerdirHm, but this doesn't address the original reason this issue was opened, removing the config part only addresses part of the problem.
Comment #20
alexpott@Berdir oops you're right - I seem to have inadvertently diverted this issue - sorry. Is the hasData() the only problem with using key value storage for content entities? Shall we merge the patches - or open a new issue?
Comment #21
BerdirI think we still weren't able to convince tim about the test module part, so merging isn't a good idea. Maybe we should just re-open #1853856: Document that ConfigEntityBase and ConfigStorageController are tightly coupled (which was closed as won't fix, which is wrong now anyway) and post the patch there?
Comment #22
alexpottOkay moved my patch to #1853856: Document that ConfigEntityBase and ConfigStorageController are tightly coupled and re-opened. Going to revert the issue summary on this issue.
Comment #23
alexpottComment #24
alexpottComment #25
BerdirThanks!
Comment #26
cilefen CreditAttribution: cilefen commentedIs this still needed? It seem so.
Comment #31
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedSo #1853856: Document that ConfigEntityBase and ConfigStorageController are tightly coupled removed the ability of using
KeyValueEntityStoragefor config entities, and if we want to keep this storage as a non-test implementation, we should simply document that it's not designed to be scalable like the SQL content entity storage.