Inconsistencies when updating overridden config entities

Problem

I think the main problem is actually, that config entities do 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 i fear it shows a bug that allows overriden values to accidentaly be leaked into the actual "editable" configuration.

proposed Solution

The solution for this two problems i think is:

• 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 i thing preSave/postSave hooks/entity methods work with a more predictable context.

I made this patch to prove my point.

I think this shows that the described inconsistencies are not only unpleasant but also point out a problematic bug.

Now I think fixing this bug in the way proposed in #2 will actually also solve the inconstistency problems.
That's because per default the preSave/postSave hooks/entity methods would only be called on the 'not-overriden' values & therefore as pointed out before behave more predictable.

I tried to implement the solution incremantally in 5 different ways as I am unsure what is acceptable within the existing BC policy:

• Version A: Directly modify ConfigEnityBase & ConfigEntityStorage (Existing code may get an exception thrown at unecpected times)
• Version B: Directly modify ConfigEnityBase & ConfigEntityStorage; Only thrown an exception if the corresponding EnityType has set a flag in it's annotation for supporting config overrides
• Version C: Directly modify ConfigEnityBase & ConfigEntityStorage + Add OverrideSupportingConfigEntityInterface; Only thrown an exception if the corresponding EnityType implements that interface
• Version D: Directly modify ConfigEntityStorage + Add OverrideSupportingConfigEntityInterface & OverrideSupportingConfigEntityBase; Only thrown an exception if the corresponding EnityType implements that interface
• Version E: Add OverrideSupportingConfigEntityInterface, OverrideSupportingConfigEntityBase & OverrideSupportingConfigEntityStorage; Only thrown an exception if the corresponding EnityType implements that interface. Direct save() calls on the storage are also only secure if the correct storage base class is used

I tried to fix the problems in Version A and Version B (as repesentative for C,D & E) by improving the override detection.

The general idea is to set the respective immutability information when retrieving the configuration using \Drupal\Core\Config\Config.
The *2.. patches change the methods on \Drupal\Core\Config\Config to only add the mutability information when beeing retrieved for config entities.

Wrong interdiff ...

Tried to fix the fails by relocating the immutability info assignment back to the storage & using a new method containsOverrides() on \Drupal\Core\Config\Config to accuratly assign it.

I had some time to think about it more and came to the following conclusion:
This problem only affects the active usage of config overrides which in turn seems to be an edge-case in and of it self. There fore it is propably best to go with an opt-in approach.

With the attached patch core would provide a special interface OverrideSupportingConfigEntityInterface, a corresponding base class OverrideSupportingConfigEntityBase & strorage class OverrideSupportingConfigEntityStorage for config entities extending their existing counterparts.
Both OverrideSupportingConfigEntityBase & OverrideSupportingConfigEntityStorage contain checks that prevent overriden config values from beeing saved via config entities.

What do you think?

Updated issue summary!

The previous patch is ment as a foundation, but what to do with it ...

If we would go with it, it would be a non-breaking api addition (i believe). Still the issue would remain that users of config overrides could have those overrides leak unexpectedly.
I see 5+ possible solutions for that:

1. Change nothing
2. Mark the old config entity classes as deprecated & point to these new classes
3. Warn the users when when using old entity classes & an leakage might have occured (watchdog log / drupal message). Possible combine with (2)
4. Try to prevent overrides from beeing saved (notify when it happens). Possibly combine with (3) and or (2)
5. Any of the other ideas proposed in #2744057-6: Inconsistencies when updating overridden config entities

Is this issue related?

Ran into this.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.