Problem/Motivation
Unless a config entity always uses or subclasses ConfigEntityStorage, there is no way to be sure that it supports overrides, dependency management, scalable entity queries, etc.
Proposed resolution
Enforce tight coupling between ConfigEntities and ConfigEntityStorage
Remove KeyValueConfigEntityStorage and associated tests
Remaining tasks
User interface changes
API changes
Trying to use a storage class that does not extend ConfigEntityStorage will now throw a ConfigEntityStorageClassException exception.
Original summary
Content and config entity info is currently collected at the same time, and also has the same _alter() hook.
Configuration entities have a locked storage backend (must always be configuration storage, you can't swap it to something else and expect it to work). There's also going to be more divergence down the line.
So we should split the discovery and the alter to make it clear these are different things, and enforce the storage restriction on the configuration entities.
This would also solve issues like #1801304-195: Add Entity reference field where there's no differentiation between the two.
Comment | File | Size | Author |
---|---|---|---|
#41 | 1853856.41.patch | 9.99 KB | alexpott |
#25 | lets-be-honest-1853856-25.patch | 1.1 KB | tim.plunkett |
Comments
Comment #1
Berdir@xjm said in #1801304-204: Add Entity reference field:
instanceof requires an object, so we would have to instantiate every one of those classes to check...
Comment #2
damiankloip CreditAttribution: damiankloip commentedYou can use is_subclass_of to check the class inheritance without an instance.
Comment #3
xjmAs discussed in IRC, is_subclass_of() would work IF we required 5.3.7. @Berdir also pointed out that we still have to load all the classes to display the list, unless we cache it somehow.
Comment #4
fagoIf an instance check isn't enough, why not just a 'configuration' => TRUE|FALSE flag? Although, I suspect that not being enough either. What kind of entity type is really relevant depends on the use-case, i.e. in a lot of cases I think people want to deal with "renderable" entity types. I'd suggest we do some helpers in the EntityManager to ease filtering entity info/definitions.
Comment #5
yched CreditAttribution: yched commented"Oh, yes please" for separation.
Having all of those lumped together in a single definition set that's collected in one single pass raises all kind of dependency loop issues in #1735118: Convert Field API to CMI (turns $field and $instance into Config Entities)
Comment #6
fagoad #5: I suppose that's because view modes and bundles, which we wanna separate anyway? Or is there anything else problematic? (I cannot think of anything.)
Comment #7
sun-1
Separating them means that we'll ultimately end up with entirely different APIs and subsystems, which is not what we want.
I additionally question the reason that caused this issue — who says that we don't want to use Views to list configuration entities? Who says that config entities cannot be fieldable?
The current problems and confusion solely stem from (de)coupling issues like
#1822458: Move dynamic parts (view modes, bundles) out of entity_info()
#1782460: Allow to reference another entity type to define an entity type's bundles
Furthermore, #1801304: Add Entity reference field seems to make the same known wrong assumption as
#1823494: Field API assumes serial/integer entity IDs, but the entity system does not
Comment #8
yched CreditAttribution: yched commented@fago #6 :
Well,
- there's bundles and view modes, and true, we know we want to remove them from entity_info
- there's drupal_get_schema(), that, because field_sql_storage_schema() by nature depends on reading Field config entities, introduces a dependency on the whole entity_info discovery and triggers hook_entity_load()
- there's stuff like rdf_entity_load() that was coded in a world where entities were content entities, and triggers sql queries on config load - doh !
Hard to say what other hook_entity_[op]() exist in contrib and what they do that would potentially be a stupid thing to do on a config entity :-)
Comment #9
catchNone of these things are blocked by having separate discovery, nor are they listed in the opening post.
The main thing at the moment is there is an implicit dependency of all configuration entities on CMI storage. Right now it we happen to specify that in the entity info for each config entity and hoping no-one overrides it, if they do then it'll blow up.
Even if CMI storage is pluggable, then configuration entities will still have to use the global CMI storage - you can't (at least not with the current design) combine multiple different CMI storage backends and have things like configuration staging work.
So since that is an absolute, explicit, not going to be changed for a very long time dependency (that config entities need to be stored in config), we should make that explicit. Being able to easily get a separate list of them would save horrors like http://drupalcode.org/project/drupal.git/blob?f=core/includes/config.inc... too.
Some people in irc said they might want to use CMI storage for content entities too - that's fine since there's no such restriction in the other direction (except for it not working). wouldn't be affected by this change at all.
Further to that, even if config entities are fieldable, those fields will need to be stored in the CMI storage, otherwise they can't be deployed with the config entity. But yes right now if we made config entities fieldable and the storage defaults to field_sql_storage then it won't work at all, because config entities are a subset, not a superset of the Entity API as it currently stands.
Comment #10
fagoI fail to see why this demands a different discovery. Adding a configuration => TRUE flag and force CMI storaged based upon that would work also.
Although I still question the need for forcing the storage to CMI. What are the reasons for that? Maybe someone wants to do config entities without CMI for whatever reason?
So I dislike the idea for the same reasons sun brought up in #7, it separates system which in reallity are just the same.
ad #8: Good points :-)
I see - this "denormalization" already caused quite some trouble for other modules trying to get entity-info from the schema (uuid..), so I think it's something that should go away also. Once we've metadata for all our fields it shouldn't be necessary any more either, because then, the entity system can control its storage instead of the storage system telling the entity system what it really is about ;)
yeah, rdf module was coded in a d7 world and needs to be updated fo d8. Moreover, I'd like to do away with entity_* hooks for CRUD and leave it for notifications only (if not completely remove load). See #1729812-2: Separate storage operations from reactions. We should make fields the only way to extend entity storage - modular storage generally doesn't work as well.
Comment #11
fagoSure, that's why we wanna do #1497374: Switch from Field-based storage to Entity-based storage?
Not sure why config entities should be superset or subset of the entity api, they are just entities?
Comment #12
catchThe drupal_get_schema() could disappear now that SchemaCache is in.
So let people alter the storage key but it'll fail silently?
There is zero use case for this, config entities only exist because of CMI - i.e. so you can stage them between sites using the configuration management system, which relies on having them in the same storage as the rest of your configuration.
Comment #13
yched CreditAttribution: yched commented@fago:
I'm afraid I don't get that paragraph :-p
Comment #14
fagoE.g, node types make sense to be entities, as well as to be config and so to be stored via CMI. Given we'd have a "configuration" flag denothing the type to be treated as config, I still might want to have the same *config entity* with a different - custom - storage backend, with whatever storage engine. Maybe it uses the next generation CMI or something else.
So I think having that storage pluggablitiy is nice, and might be become useful. But, the other way round, why should we lock it down?
Although catch already solved that part for us, I try again: The entity system should not depend on the metadata from the storage system to work. The db storage controller might, but we should do away with this dependency in the general entity info.
(Furthermore we should be able to leverfage entity field definitions instead of db schema information once all entity types are converted to entity-ng).
Comment #15
yched CreditAttribution: yched commentedre @fago #14 : "The entity system should not depend on the metadata from the storage system to work".
It's the other way around - right now, at some point we need a db schema, and we need to read field config to build it. This triggers collecting entity_info and fires hook_entity_load(), which is a nasty dependency.
"DB schema depends on config" is the essence of "configurable fields stored in sql (be it at the field level or entity level)". This won't change whatever the place where we put the metadata.
But, so, ok, as @catch points in #12, this might be less of an issue if the catch-all drupal_get_schema() goes away in favor of the more granular SchemaCache. Probably depends on how that would work in module_enable(). Is there an issue for that ?
Comment #16
catch@fago
That ignores this point:
i.e. when importing configuration, CMI only looks in one place (the file storage currently, or whatever it might be swapped out with).
Comment #17
fagoad #16:
No it doesn't ignore it - if it doesn't use CMI any more there is no reason CMI should deal with it ;) Still, I'm missing the reason to lock it down to CMI?
Comment #18
catchIf it doesn't use CMI, how is it configuration?
Comment #19
fagoQuoting from #14:
Comment #20
catch"treated as config" to me means deployable (and all the other things you get from CMI like diffing changes or whatever down the line).
We developed CMI to have a unified and deployable configuration system, if something's not in there, it's either 'wrong' or not configuration or an extremely special case. "I might want to" is not a special case, I'm still waiting to for a use case for actually doing this but none has been provided.
Nothing stops you re-declaring these as a config entity with joint CMI + Entity storage if you want to (if they were actually separate things), the only thing we want to prevent here is taking something specifically designed to be in CMI out of it.
Comment #21
fagoUse-case: Handle it with a next generation CMI in contrib. But on the contrary, no reasons for locking it down have been provided?
Comment #22
catchWhy not swap all the CMI storage out to use next generation CMI? Why would you want to do that for only one config entity or all config entities, but leave the rest in the old storage?
The reason for locking it down is that things will suddenly stop working if you swap it out to say MongoDBEntityStorage or SQLEntityStorage or anything else that's not the same as the global CMI storage backend for the site.
Comment #23
fagoBecause next generation CMI would probably work differently than CMI, thus have other interfaces etc. - else it would not be really a new CMI.
Just like all other entity storage backends? If you swap out node storage, lots of stuff will break - unless you'll take care of it. You can break a lot via altering things - so why should we handle this case differently?
Comment #24
catchThe custom node (and other) storage controllers exist almost entirely because the core entities still have some hard-coded (and often strange, like user pictures) behaviour that hasn't been brought up to date yet. To move to entity-centric storage we'll need to separate stuff out of those anyway so they can be cleanly replaced. It's not the same thing at all as CMI storage which provides a generic feature that all config entities exist explicitly to take advantage of.
Comment #25
tim.plunkettFrom the OP in #1853856: Document that ConfigEntityBase and ConfigStorageController are tightly coupled:
This was never the intention of the configuration entity system.
catch and I had a good talk about this.
As I understand it, we both realize that the currently system is implicitly coupled.
His suggestion above was to make this explicit and live with it, while I hoped to "just fix it".
As a stop-gap compromise, let's acknowledge the brokenness, and fix it in #1862300: Move ConfigStorageController::getQuery() to EntityStorageControllerInterface
Assigning to catch to make sure this matches what he understood of our conversation. :)
Comment #26
xjmThis seems to me like a correct stopgap fix to me (as we discussed with @catch last night, um, between sets). For test coverage related to this, tim.plunkett has filed #1862302: Add a test implementation of EntityStorageControllerInterface. We can further explore whether #1862300: Move ConfigStorageController::getQuery() to EntityStorageControllerInterface is correct in that issue. RTBCing for catch's review.
Comment #27
yched CreditAttribution: yched commentedCommitting this sounds fine, but it does not adress the points discussed earlier in the thread, which basically boil down to "we need a way to tell config entity types from content entity types" ?
Thus I'm a bit reluctant on calling this issue fixed after #25 gets in. (and not convinced by the title change either)
Comment #28
xjm@yched, I think that's actually somewhat a separate concern from #1862300: Move ConfigStorageController::getQuery() to EntityStorageControllerInterface (at least, it does not necessarily need to be fixed in the same way). This patch addresses the issue in the OP, although not the previous title.
Comment #29
tim.plunkettI think the distinction between ConfigEntity and ContentEntity is an invalid one that we'll be ideally phasing out.
The only current use of that is in EntityReference, which is actually too limited. File is a ContentEntity, and yet I might not want there to be "file references". That should be its own annotation property.
ConfigEntity is not fieldable, but that's a bug, not by design.
The distinction between "content" and "configuration" is not important if you can freely swap where its stored. We choose to store some in the DB, and we choose to store others in CMI.
Comment #30
yched CreditAttribution: yched commentedAgain: hook_entity_load() needs a way to tell whether the entity type is config or not. rdf_entity_load() currently fires an sql query on each loaded config entity.
Let's please not shut the discussion still, we seem to be far from consensus here.
Comment #31
tim.plunkett@yched are taxonomy terms configuration or content? Are users content?
I think "content" means different things to different people and systems. But perhaps a
* content = true,
annotation flag is in order?Comment #32
xjmMy point is just that the discussion can be had separately and the patch here is worthwhile on its own to address the original problem. This issue has a tangle of several discussion threads: the distinction between config and content, and whether/how to list or load them; then whether the coupling of of ConfigEntityInterface to ConfigStorageController is a half-implemented design decision or a bug; and then the simple fact that it's broken right now. I suggest separating those discussions--not "shutting them down"--because they don't necessarily have the same solution.
Comment #33
sunI don't really see what this patch buys us (or anyone).
The reasoning that has been stated for this patch is essentially: "Prevent someone from altering entity info in a way that may not work."
I consider this wrong for two reasons:
1) We do not babysit broken code. Users of
drupal_alter()
are on their own, by design. An incomplete/bogus config entity type definition that does not use what it wants or intended to use is equally broken, logically broken. We generally have close to zero protections for/against broken declarative definitions in core.2) The patch validates the binding in the wrong direction. ConfigStorageController is tightly coupled with ConfigEntityBase currently, but ConfigEntityBase is not bound to ConfigStorageController. I don't see why one shouldn't be able to use ConfigEntityBase with other controllers. As such, the validation would at least have to be moved into ConfigStorageController instead of the generic entity plugin manager.
Comment #34
yched CreditAttribution: yched commented@tim.plunkett:
That's a bold statement :-).
- That would mean a way to store field values within CMI in a way that works with CMI tools like deployability, metadata...
- Before we go there, I'd suggest we come out with some real life, mainstream use cases.
Sure, the myhtical "grey area". I'm not sure we can really say we conquered that by stating "we don't know where entity X is stored and deployed, anything can be altered to be stored in DB or in config or somewhere else". As the discussion with @catch above shows, that's still highly hypothetical IMO.
At any rate, *at a given point in a given runtime context*, we need to be able to distinguish between entity types according to some criteria, like, yes, "is it config / content ?", "can it be displayed in HTML ?", "is it custom-fieldable ?"
Or else the generic hook_entity_OP() is a fiction, there's not really such a thing as "an operation that you'll want to do on any entity type whatever". rdf_enitty_load() needs to be smarter than it currently is, only it doesn't have any way to become smarter right now, i.e to identify the entity types it really wants to act upon.
Comment #35
catchThis still needs more discussion, and yes I opened this for separate discovery not just because swappable storage is broken but because lots of other things don't make sense at the moment either (in other words, what yched said). I'd prefer to make it absolutely explicit (and enforced) that configuration entities don't have these features since that is the current status quo and pretending it isn't is a massive wtf
Then if issues get filed to make them work again in a sane way we might end up removing it all again, but that's not going to be a small task and there's more critical entity and configuration system issues around.
Comment #36
gddComment #37
sunComment #38
tim.plunkettWe've been using both tags for issues like this.
Comment #39
gddYeah I'd prefer to keep using both so I don't have to track two tags separately
Comment #39.0
gddUpdated issue summary.
Comment #40
tim.plunkettThis ship has long since sailed.
Comment #41
alexpottAnd sailed right back round again. This patch is the same as was rtbc in #2393751-15: Document that KeyValueEntityStorage is not scalable beyond a few hundred entities after I derailed that issue.
Comment #42
alexpottThis is a bug. The assumptions made about configuration overrides and dependencies make this change a necessity.
Comment #43
catchCommitted/pushed to 8.0.x, thanks!
Comment #44
jibranNot pushed yet.
Comment #46
daffie CreditAttribution: daffie commentedAnd now it has.
Comment #47
chx CreditAttribution: chx commentedReport from the "alternate storage engine" patrol: this makes sense and I never felt the need to change the ConfigStorageController because it does not really operate on storage directly instead it uses the config storage and that one is pluggable. All is well.