Problem/Motivation
Once #2392319: Config objects (but not config entities) should by default be immutable is committed, we should be able to remove these functions and the whole concept of an override free config factory.
It should still be possible to get editable configuration objects override free but the state should not settable. This will help us prevent unexpected usage of override free configuration objects. For example, if you create a config event listener the config factory will be in an override-free state for the save and delete events but not for rename. The save and delete events should have the override-free config object but all other configuration obtained from the factory should have overrides.
Proposed resolution
Remove the methods.
Remaining tasks
Commit.
On commit add this issue to the CR https://www.drupal.org/node/2407153
User interface changes
None.
API changes
Removed ConfigFactory::setOverrideState and ConfigFactory::getOverrideState()
Beta phase evaluation
Issue category | Bug because a stateful config factory is problematic. It makes unintentional side effects very difficult to avoid. |
---|---|
Issue priority | Critical because config CRUD events are runtime actions but the config factory is in override free state. |
Disruption | Disruptive for anything that use setOverrideState or getOverrideState. This is mitigated by the fact that these should probably be using ConfigFactory::getEditable() anyway. |
Comment | File | Size | Author |
---|---|---|---|
#6 | 2406543.6.patch | 33.82 KB | alexpott |
#6 | 3-6-interdiff.txt | 1.44 KB | alexpott |
#3 | 2406543.3.patch | 33.79 KB | alexpott |
Comments
Comment #1
alexpottComment #2
alexpottComment #3
alexpottPatch that removes these methods from the factory. Things to note:
Comment #4
alexpottComment #6
alexpottFixing the tests (hopefully).
Comment #7
alexpottComment #8
alexpottI think the change record for #2392319: Config objects (but not config entities) should by default be immutable covers this issue too. (https://www.drupal.org/node/2407153)
Comment #10
Gábor HojtsyI think this is a clear improvement, removes one moving piece. I think it would be *really* fabulous to document when to use getEditable() on the factory vs. getOriginal() on config vs. getOverrideFree on storage. They are similar in some way (also see some notes below). However I don't think that could be in this patch in some way. Also whole #2392319: Config objects (but not config entities) should by default be immutable's change notice covers some of this, the removal of the public API to switch this state is well worth its own change notice.
Should this not depend on immutability as well? Correction: ah, you only invoke it if immutable.
Woot, so much better :)
Is this what we expect? Config's get returns the current runtime value (may be different from getOriginal() which is the "on disk" value). So there are subtle differences between override free and original depending on whether there are runtime changes.
Comment #11
alexpott#10.3 Well if we are relying on the object to hold changes that have not been saved for config entities we'll have major problems. So I think this is acceptable. No one should be interacting with config entities directly through the config factory unless they are doing something exceptionally low level - i.e. certain language operations. And these definitely should only be doing reads and not sets/saves - since if they save then all the entity hooks are avoided.
Comment #12
Gábor Hojtsy@alexpott: ok I don't have a problem with that; I think there are definitely tests where they use the config API direct so the config entity storage cache may not be updated; that evidently does not fail, I guess we don't use the entity API after that. Not sure if we want to add a test for or against what we expect here but if this is an expected behavior it sounds like it would make sense.
Comment #13
alexpottI wonder if we should remove the static cache in the ConfigFactory for config entities. This would remove an unnecessary layer of static caching and make the concerns go away.
Comment #14
BerdirConfig entities do not have static cache enabled by default at the moment: #1885830: Enable static caching for config entities
Comment #15
Gábor HojtsyI see there are getFromStaticCache and setStaticCache methods but the config entity type is not statically cacheable, so no use. Haha.
Comment #16
BerdirYes they are theoretically, we AFAIK use it for roles, but it is not enabled by default. And if you look at the other issue, the tests there fail with dozens of weird invalid cache issues, that I was too lazy to track down.
Comment #17
BerdirBut agreed that for those that we do cache statically, we should not cache them twice. Which is more complicated than it sounds I think, maybe the entity storage class could tell the factory to not cache what it is loading, but there is also the entity query implementation that afaik works on raw config values, so that would make entity queries possibly slower.
Comment #18
alexpottSo I've tried to add a test for this but it's just impossible because you can't set data on an immutable config object :) and if you are changing a config entity it does a setData() so anything that is set is blotto-ed anyway. So I think we are good to go here.
Comment #19
Gábor HojtsyAll right, I did not find anything else. Added draft change notice at https://www.drupal.org/node/2410365
Comment #20
webchickSince catch committed #2392319: Config objects (but not config entities) should by default be immutable and this seems highly related, tossing over to him.
Comment #22
catchReally like this. It's hard to get your head around config overrides and this removes a potential source of confusion.
Committed/pushed to 8.0.x, thanks!