Problem/Motivation
Config entities seem to not mirror the behaviour enforced by underlying configuration API.
The underlying configuration API behaviour is described as:
- Normally you would get the configuration as "immutable", so it can not saved & in fact will even throw an exception when you still try it (But it will contain corresponding config overrides if any are specified)
- If you want to change the configuration you need to get it as "editable", so you can save it(But it will not contain any overrides)
Configuration API
But for config entities this is not described as clearly. The only information you get is that for "admin"- routes (/admin/*) if config entities are used as route parameters, their values are ensured to be not-overriden (like e.g. routes that use an entity form); but only on those routes.
Config enties actually do not distinguish between a "immutable" & "editable" variants.
This allows for the following two code flows:
a) Loading an not-overriden version of the config entity, change it, save it:
- load/change: the entity contains not-overriden, possibly changed, values
- preSave: the not-overriden values can be compared agains the "original" version (the "original" does contain overriden values)
- save: the not-overriden values may be cast to match the configuration schema & are stored as configuration
- postSave: the not-overriden values (which may have been cast to match the schema) can be compared agains the "original" version (the "original" does contain overriden values)
b) Loading an overriden version of the config entity, change it, save it
- load/change: the entity contains overriden (possibly changed) values (the changed values might be the ones that were actually overriden or not)
- preSave: the overriden values can be compared agains the "original" version (the "original" does contain overriden values)
- save: the overriden values may be cast to match the configuration schema & are stored as configuration (At this point overriden values might be stored although they were not actively modified)
- postSave: the overriden values (which may have been cast to match the schema) can be compared agains the "original" version (the "original" does contain overriden values)
This shows on one hand that as @druken monkey described above the code in the preSave/postSave hooks/entity methods may be called with overriden or not overriden values. On the other hand it shows a bug that allows overriden values to accidentaly be leaked into the actual "editable" configuration.
Proposed resolution
- Introduce a flag to config entities like: isEditable / isImmutable / mayContainOverrides
- Per default load config entities as: !isEditable / isImmutable / mayContainOverrides
- Disallow saving entities in the ConfigEntity storage (by throwing an exception) which are: !isEditable / isImmutable / mayContainOverrides
- Maybe add a method on config entities / simply use the ConfigEntityStorage::loadOverrideFree to get instances that are flagged as: isEditable / !isImmutable / !mayContainOverrides
- Since all config saved would now be ensured as not-overriden, preSave hooks/entity methods should get an not-overriden original
- postSave hooks/entity methods would also get a not-overriden original
- If someone needs an overriden original in preSave, they could simply load an overriden version (as the changes are not yet persisted)
- If someone needs an overriden original in a postSave hook / method, they could could theoretically set it as a seperate Field like maybe 'overridenOriginal' in a corresponding preSave hook/ method so they cant it get's passed along to postSave hook/ method
- A generic 'overrideOriginal' solution would also be possible but would need to be implemented at the storage level (the "original" should be unset at the end, but because entity->postSave ist called before the entity hooks this could not be done in the config entity class)
This way the beghaviour would be more in line with the underlying configuration API & And preSave/postSave hooks/entity methods work with a more predictable context.
Remaining tasks
- Decide what to do with the existing config entity related classes (merge changes into them, deprecate them, change them to not allow config overrides ...)
API changes
- Introduces new Classes & an Interface / expands the existing ones (OverrideSupportingConfigEntityInterface,
OverrideSupportingConfigEntityBase & OverrideSupportingConfigEntityStorage)/li>
Data model changes
No changes, the overriden data only resides in the class and is not persisted.
Original report by drunken monkey
In the Search API (
#2682369: Fix problems with overridden config entities →
) we've discovered problems when using config overrides with config entities, which I think could affect other modules as well, and might therefore be better solved in Core than in each of them separately.
The root cause of the problem is basically that an entity update will mix non-overridden and overridden entities in non-obvious ways.
Specifically, config entities are by default not overridden when loaded for admin routes, where most of the changes probably occur. But when saving such an entity, the original that is loaded is loaded with overrides, so when any pre-/post-save code tries to assess the changes done to the entity, it will inevitably end up with wrong results when comparing overridden properties.
Ideally, I think, both the "current" and the "original" entity used would show all the overridden values, so that really only those properties that effectively changed their value are actually detected as changes.
However, especially for the pre-save code, this is probably not that easy to do, and doing it just for post-save might just add more confusion.
So, this is more meant as the start of a discussion, or a request for other solutions.
Do you even agree that this is a problem? And, if so, how can we solve it?