Allow ConfigEntity classes to control how they are saved

Yes, we definitely want to do something about that, so thanks for creating this issue. I think Views has a similar need.

I think a new method on ConfigEntityInterface makes sense, but I'm not too fond of the name "export", since that is veeery ambiguous in the configuration system space. ;)

Long-term, I'd think we'd also need the counter-operation, actually; i.e., "import" (bleheheh).

The current/original code and logic was rather quickly invented for the initial Configurables/ConfigEntity concept patch. At that time, I looked into how Views and CTools are handling it and only saw some negated logic in there (i.e., extract all properties, except some reserved property names), which didn't really make sense to me, so I essentially added a positive/non-negated extraction of properties, and limited it to public properties.

In light of EntityNG and current attempts to add metadata to config entities, I wonder whether it wouldn't make sense to describe config entity properties a bit more formally. That said, I'm not up to speed with the new EntityNG world order yet.

Oh, and btw, a more formal definition of properties would most probably also help with #1653026: [META] Use properly typed values in module configuration — i.e., the lack of data types. If we'd know what properties exist and of which data type they are, then we'd be one step closer to removing the string-casting for all config object values.

Views lets its storage controller handle how it is stored, which I think is correct. (It's completely different now from how it was as described in #5).
That's why getProperties even exists, see #1824484: Allow config storage controllers to define their own properties.

I disagree with this entire premise.

Sorry, I obviously missed that getProperties() change.

I don't understand why it was added on the storage controller though. The storage should not have a business in deciding what makes up the actual entity class/properties. Or in other words, the entity class properties are always entity-specific, not storage-specific — if you swap out the storage, the entity properties won't be different.

I'd tend to agree with this. It's the role of the storage controller to decide how to save, but not *what* to save, which shouldn't change if the controller was swapped.
Same pattern as for non-config entities IMO. An entity type knows its own components.

Yeah, that's what I'm saying, too (not sure whether my comments were fuzzy).

Entity::getProperties(), however, is apparently taken by the new EntityNG TypedData stuff already. So we cannot override that in ConfigEntityBase but need to find an alternative method name instead.

Since this might be migrated and merged into the EntityNG design in the long run, I don't think we need to spend a long time with finding a method name — anything except "import" and "export" will probably work for me :)

Should entities that use a subclass of ConfigStorageController be required to be an implementation of ConfigEntityInterface?

That question I think determines the resolution of this issue, and also clarifies the entire separation between ConfigEntity and Entity.

Should a contrib module be able to implement hook_schema() for a ConfigEntity and switch its storage controller to a subclass of DatabaseStorageController?

If so, we've made a mistake with the existence of ConfigEntityInterface. Fields are unfortunately bound to a base table, so currently a ConfigEntity could never be fieldable. Most of the overridden methods in ConfigEntityBase are done because "configuration entities are not fieldable".

Most of this belongs in another issue, but my point is that DatabaseStorageController depends on an externally defined schema, but not one controlled by the Entity class. ConfigStorageController should also not depend on the Entity class for its "schema" (list of properties to export).

Looks like this is indeed a bigger conceptual problem, but we need a resolution for displays sooner in #1841584: Add and configure master displays, so looking forward to where @sdboyer submitted that other issue he mentions would be opened :)

I'd love for this to be won't fixed. See #1841584-66: Add and configure master displays

The minimum task and adjustment I see for this issue here is to move the getProperties() code from the storage controller to the entity class, since the storage controller should not have any knowledge about how the entity is composed/structured. The storage controller is about CRUD and I/O only.

Any kind of k/v store will be able to read and write config data just fine. To use the Entity\DatabaseStorageController, yes, one would have to define, supply, and set up a table schema first, which got inherently lost by moving from SQL to a file-based storage (and that's ultimately one more reason for #1648930: Introduce configuration schema and use for translation), but that's a different issue.

Speaking of, the closely related, low-level Config\DatabaseStorage backend still exists. Along those lines, a Config\Entity\DatabaseStorageController that simply uses a keyvalue table schema to store entities is perfectly possible and does not require a formal schema definition.

Apparently, Entity::getProperties() exists already, and it seemingly appears to have the exact same purpose, so perhaps all we need to do is to override that method in ConfigEntityBase::getProperties()?

That's actually ComplexDataInterface::getProperties() :(
To clarify my -1, I don't think there should be more than one place to control this.

This resolves my qualms about this issue.

Or, you know, a patch that works.

Thanks! Still not a fan of the term "export", but happy to rename the method later.

Looks RTBC to me. @sdboyer?

This looks good so I committed this to 8.x. Instead of 'getExportProperties' we could have used 'getConfiguration' or something. We can debate that separately; we can stick with the existing terminology for now. This is a major bug report so I want to get it out of the way for people to make progress.

Thanks!

Follow-up: #1864096: Rename EntityInterface::getExportProperties() to something that is not ambiguous with config import/export