Problem/Motivation
- User translates the front page view to some language.
- User goes and changes the front page view, ie. makes it a table with fields instead of a node view.
- User DOES NOT update the translation manually.
- Now the translated version will use the translation still corresponding to the OLD structure of the view, so invalid residual pieces of row styles, etc. will show up.
May lead to all kinds of strange consequences. Also applies to other kinds of overrides.
Proposed resolution
When a configuration changes, overrides should be re-saved and only keys still overlapping with the original configuration should be kept. All other keys should be dropped. This may be applied to any kind of override.
Brought this to CMI meeting:
1. This is an API addition, not beta blocking.
2. Postponed on #2224887: Language configuration overrides should have their own storage since it implements override storage where save for overrides fires no events. Otherwise this is hard/impossible to implement because now language overrides are in general config and have events fired.
3. This could be implemented as a base class for overrides that would listen on save events and update its overrides.
4. Then language overrides would extend from this base class instead of just implementing interfaces.
Marked as beta target because this API addition would be nice to do before beta so modules can extend from that. Eg. domain module for its overrides.
Remaining tasks
Review. Commit.
User interface changes
None.
API changes
None, however overrides are now suggested to extend from ConfigFactoryOverrideBase.
Comment | File | Size | Author |
---|---|---|---|
#26 | interdiff.txt | 4.8 KB | Gábor Hojtsy |
#26 | 2268939-config-override-save-26.patch | 10.27 KB | Gábor Hojtsy |
#24 | interdiff.txt | 2.72 KB | Gábor Hojtsy |
#24 | 2268939-config-override-save-24.patch | 10.29 KB | Gábor Hojtsy |
#22 | interdiff.txt | 1.78 KB | Gábor Hojtsy |
Comments
Comment #1
Gábor HojtsyBTW this used to be #1966538: Translation is not updated for configuration, when the config changes but that was closed since it was about to cover other things which became outdated. Now its a "simple" structural question.
Comment #2
Gábor HojtsyBrought this to CMI meeting this week:
1. This is an API addition, not beta blocking.
2. Postponed on #2224887: Language configuration overrides should have their own storage since it implements override storage where save for overrides fires no events. Otherwise this is hard/impossible to implement because now language overrides are in general config and have events fired.
3. This could be implemented as a base class for overrides that would listen on save events and update its overrides.
4. Then language overrides would extend from this base class instead of just implementing interfaces.
Marked as beta target because this API addition would be nice to do before beta so modules can extend from that. Eg. domain module for its overrides.
Comment #3
Gábor HojtsyComment #4
Gábor Hojtsy#2224887: Language configuration overrides should have their own storage is definitely in, so we can unpostpone this.
Comment #5
Gábor Hojtsyvijaycs85 says he is working on this.
Comment #6
vijaycs85Comment #7
vijaycs85Here is an update on the approach as per issue summary.
Note. Can't really use baseClass for the event as it is a static method for registering events.
Comment #8
alexpottWe need to handle deletes and renames too. And we need tests.
Comment #9
vijaycs85Thanks for the review @alexpott. Here is an update. Still need to add test and make sure the code covers all functionalities mentioned in issue summary...
Comment #10
Gábor HojtsyI don't see how this will react to config data changes, maybe I'm not clear on something getOverride() does to ensure this? The rename and delete reactions look complete though. Besides missing tests, I only found this minor issue:
Not indented properly.
Comment #11
Gábor HojtsyFixed the indent and a comment and added a start for some tests. Although locally untested :D
Comment #13
Gábor HojtsyDecided to not modify the existing test because it deals with overrides for config not yet saved. Would have needed to modify it too much to be good for this other use case, so just added more test data for the new use case. Cleaner. Should theoretically have less fails, since we were testing things that were not appropriate for both scenarios when reusing the older test code.
Comment #15
Gábor HojtsyThe first fail was due to bad test :) Was about ensuring the original data was removed. Fixed that. The other fail proves the language override is in fact not updated when the original data changes. :) Just as I theorised in #10.
Comment #17
Gábor HojtsyI think this is about the solution to the problem. As discussed before we need to filter the override based on the original config. I think we'll need this as utility methods, so added to ConfigFactoryOverrideBase.
Comment #18
Gábor HojtsyImproved the test in two ways:
1. Made the override use a nested key element not on the top, so e can test that if that is removed in the data, the whole override container data structure is removed if empty.
2. Added a French override as well that uses less data and therefore disappear sooner, when it does not contain data anymore. Added tests for that.
Comment #19
Gábor HojtsyWith improved type hinting and phpdoc. I consider this ready.
Comment #20
Gábor HojtsyI don't think either vijaycs85 or myself are qualified to RTBC this one, so assigning to alexpott.
Comment #21
Jose Reyero CreditAttribution: Jose Reyero commentedLooks good to me, surprisingly simple and straight forward for what it does I'd say.
The only thing I don't get is using a base class + the actual implementation, that I don't know whether it can be reused for anything else and actually save any code somewhere..? It looks to me just too specific for this case.
Note the abstract class just gives us a few static methods that could be placed (and still be reused) anywhere else.
One minor issue / question:
Wouldn't it make sense to check whether there's actually any overridden data before moving on to filtering / deleting / etc..?
Comment #22
Gábor HojtsyYeah it makes sense to check for new-ness of the override (whether it actually exists or was just created). Also using the same logic in the other places to ensure consistency.
As for the applicability of the base class I think there is good value in both unifying the event listener methods (this is done a lot in Drupal 8 elsewhere) and also the generic override filter methods are applicable regardless of the override itself, whether it is domain based or og based. So those modules can use the same base class and feed their overrides to the filter code. I originally thought we would go as far as to implement this even outside the concrete override classes and on the collection storage level, assuming all storages but the default storage are overrides, but that may or may not be a good assumption. Then we would not need to do custom code for each override type to enumerate its (possible) list of overrides for a key. This is the main thing I guess I would love to get feedback from alexpott as well :)
Comment #23
Jose Reyero CreditAttribution: Jose Reyero commentedOk, this makes sense.
Comment #24
Gábor HojtsyMaking the utility methods protected instead of static based on in-audio meeting review with @alexpott.
Comment #25
alexpottrenaming from config_test.overide to config_test.override makes this test unnecessarily hard to grok. Lets rename from config_test.override to config_test.renamed_override.
Also calling the object override is confusing... we're overriding the override :). Plus we're overriding a non existent thing.
This is no longer used.
Comment #26
Gábor Hojtsy1a. All right, I think overide => override was fun, but I can see how that may be confusing. Now renamed to 'foo' => 'bar' to make it clear. It does not name the original override then :D
1b. As for how the override was first before the config, that was to avoid needing to manually clear the config factory cache :D Now I moved the original before the overrides, and now clearing the cache, so we don't get the non-overridden version from the cache.
2. Removed.
Comment #28
alexpottAwesome source. Nice to see this fixed and in a way that other configuration override implementations can benefit from.
Comment #29
alexpottI rtbc'd it so I can't commit it :)
Comment #30
catchCommitted/pushed to 8.x, thanks!
Comment #32
Gábor HojtsyAmazing, thanks.