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.

Files: 
CommentFileSizeAuthor
#25 lets-be-honest-1853856-25.patch1.1 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,249 pass(es).
[ View ]

Comments

@xjm said in #1801304-204: Add Entity reference field:

FooClass instanceof ContentEntityInterface was supposed to be the way to determine the distinction, except for users being weird and extending Entity directly right now.

instanceof requires an object, so we would have to instantiate every one of those classes to check...

You can use is_subclass_of to check the class inheritance without an instance.

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

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

"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)

ad #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.)

-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

@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 :-)

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?

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

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.

I 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 :-)

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

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 ;)

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

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.

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.

Sure, 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?

The drupal_get_schema() could disappear now that SchemaCache is in.

Adding a configuration => TRUE flag and force CMI storaged based upon that would work also.

So let people alter the storage key but it'll fail silently?

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?

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.

@fago:

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 ;)

I'm afraid I don't get that paragraph :-p

There is zero use case for this, config entities only exist because of CMI

E.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?

I'm afraid I don't get that paragraph :-p

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

re @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 ?

@fago

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.

That ignores this point:

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.

i.e. when importing configuration, CMI only looks in one place (the file storage currently, or whatever it might be swapped out with).

ad #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?

If it doesn't use CMI, how is it configuration?

Quoting from #14:

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.

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

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

Use-case: Handle it with a next generation CMI in contrib. But on the contrary, no reasons for locking it down have been provided?

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

Why 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?

Because next generation CMI would probably work differently than CMI, thus have other interfaces etc. - else it would not be really a new CMI.

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.

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?

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

Title:Separate content and config entity infoDocument that ConfigEntityBase and ConfigStorageController are improperly coupled
Assigned:Unassigned» catch
Status:Active» Needs review
StatusFileSize
new1.1 KB
PASSED: [[SimpleTest]]: [MySQL] 49,249 pass(es).
[ View ]

From the OP in #1853856: Document that ConfigEntityBase and ConfigStorageController are tightly coupled:

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)

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

Title:Document that ConfigEntityBase and ConfigStorageController are improperly coupledDocument that ConfigEntityBase and ConfigStorageController are tightly coupled
Component:entity system» configuration entity system
Status:Needs review» Reviewed & tested by the community

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

Committing 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)

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

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

Again: 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.

@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?

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

Status:Reviewed & tested by the community» Needs work

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

@tim.plunkett:

ConfigEntity is not fieldable, but that's a bug, not by design.

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.

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?

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.

Assigned:catch» Unassigned

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

Issue tags:+Configuration system

Issue tags:+Configuration system

We've been using both tags for issues like this.

Yeah I'd prefer to keep using both so I don't have to track two tags separately

Issue summary:View changes

Updated issue summary.

Issue summary:View changes
Status:Needs work» Closed (won't fix)

This ship has long since sailed.