Problem/Motivation
This is a sub-issue for #2201437: [META-1] Config overrides and language, now that we refactored configuration classes, events for read and override loading for language. We did not yet move config storage to its own place. That would hep with key length explosion as well as storing files at different places. It needs to handle import/sync/install/uninstall on its own.
Proposed resolution
- Implement separate storage for configuration.
Remaining tasks
- Import and sync have issues, that should still be solved.
User interface changes
- None.
API changes
- Introduce separate storage for configuration
- Add install/uninstall to the config override interface
Comment | File | Size | Author |
---|---|---|---|
#99 | 2224887.99.patch | 47.19 KB | alexpott |
#99 | 95-99-interdiff.txt | 807 bytes | alexpott |
#96 | 91-95-interdiff.txt | 5.23 KB | alexpott |
#96 | 2224887.95.patch | 47.2 KB | alexpott |
Comments
Comment #1
Gábor HojtsyComment #2
Gábor HojtsyTaking a stab at this one :)
Comment #3
xjmComment #4
jessebeach CreditAttribution: jessebeach commentedComment #5
Gábor HojtsyHere is a first roll of this. I did not successfully wired up the overrides yet.
Comment #7
Gábor HojtsyHelps to not introduce PHP errors.
Comment #9
Gábor HojtsyThe service definition should use the right storage not the old file storage. This exposes some new issues but shut cut down on fails a lot as a start :D Babysteps.
Comment #11
Gábor Hojtsy@swentel told me he has fixes to post. Now just need to get him post them :)
Comment #12
swentel CreditAttribution: swentel commentedWeirdly enough, it all broke down again after last week and I now have now even more errors then on the current patch, so there's no real use to posting it now, sorry for the delay on this :/
Comment #13
Berdirpersist can be removed now.
Hm... This introduces a much higher coupling between language storage, cached storage and file storage than right now, the fact that we have to extend the cached storage and then inside hardcode the active storage constants and use of FileStorage seems very unfortunate. Need to think if there's a better way.
Comment #14
BerdirThis should fix the most obvious mistakes, let's see where this puts us. Going to work on improving the storage now.
Comment #16
BerdirTrying to untangle the storage stuff, there are now two classes, one that subclasses CachedStorage, which now supports a prefix directly, the old method were not actually called, and one that subclasses FileStorage. Still have to hardcode the active path in the file storage, but I think this is much cleaner already.
Also added the missing return array(), that should fix those php notices.
Comment #18
Gábor HojtsyDebugged this further. The attached patch fixes the following two issues:
- The config translation UI is now getting a config override object and should typehint it as such (fixes some test fails)
- The setLanguage() method has an optional langcode, if no language was passed, NULL should be passed along (fixes lots if not all of the exceptions)
I also debugged why upon saving the translation, it is not showing up as translated through the config translation forms (its clearly exported into files right away), but did not yet get to the bottom of that. That results in several fails in config translation and possibly others. May be too aggressive caching.
Comment #20
Gábor HojtsySomehow the .yml file renames did not make it into my patch.... Doh. This should fix some further fails.
Comment #22
Gábor Hojtsy- Found one more language.config YAML that should be renamed.
- Found a use of typedConfig where it should be typedConfigManager.
- Invoking setLancgode() would not work :D setLangcode() is better.
Comment #24
Gábor HojtsyAll right, I spent considerable time with this and could not figure out an apparent fix. The problem is clear: The code used in the config translation form (ConfigTranslationFormBase::submitForm) does save proper to file storage but does not refresh the language override cache. This is apparent from the UI and is easily reproducible manually:
Ironically even though the bug looks very simple, I could not figure out how does the file storage / cached storage and the override object itself interact properly and where this goes wrong. It probably a simple thing and I got lost in the forest.
AFAIS fixing this would fix all but one of the fails. And there are no more exceptions already :)
I'll likely not have time to fix this anytime soon, so help is needed :) @alexpott, @berdir? :)
Comment #25
BerdirThat was a fun ride, going to explain the fixes with dreditor in a follow-up comment. Also added some basic test coverage for the cache prefix.
Comment #26
BerdirBecause we now use $cids to pass through to the cache backend, we also need to use that for checking if there is something that we couldn't load from the cache.
This fixes LocaleConfigManagerTest, we already pass $name (name of the module) to the method, so overwrote that and passed 'locale_test.translation' to the locale config override which then resulted in obviously not finding and importing any configuration.
We cloned the storage and changed the language but didn't actually pass that storage to the config override class, so we loaded from there, but passed in the storage with the old langcode. Then when saving, we wrote the cache with the wrong prefix.
See next snippet why it wrote the file correctly...
There are now two nested storage objects, and we only cloned the cache wrapper, so the file only existed globally and we changed the langcode on it globally.
Because I'm now passing the config names as array_keys($cids) to the inner storage, they are now re-keyed, which is fine I think but we need to update the test mock.
Comment #28
BerdirI was wondering what those inverse methods that I deleted were for :p
Comment #30
BerdirWill, implementing unit tests for the wrong behavior is a pretty stupid thing to do ... :)
Comment #32
Berdir30: language-override-storage-30.patch queued for re-testing.
Comment #34
BerdirLooks like the drupal_set_time_limit() fix wasn't fixing anything after all :)
Trying to see if that works, will re-open that issue.
Comment #35
Berdir30: language-override-storage-30.patch queued for re-testing.
Comment #36
tstoeckler30: language-override-storage-30.patch queued for re-testing.
Comment #37
alexpottSo we need to work out how to get configuration sync to play nice with this approach
Comment #38
xjmPoostponing on #2238069: Create a way to add steps to configuration sync per @alexpott.
Comment #39
Gábor HojtsyComment #40
xjmUnpostponed!
Comment #41
xjmComment #42
martin107 CreditAttribution: martin107 commented34: language-override-storage-34.patch queued for re-testing.
was green... but HEAD is moving fast
Comment #44
martin107 CreditAttribution: martin107 commentedRerolled.
Comment #45
Jalandhar CreditAttribution: Jalandhar commentedUpdating the patch with rerolled from #34.
Comment #46
Jalandhar CreditAttribution: Jalandhar commentedSorry , Its my mistake. Uploaded at same time with martin107
I think because of same name of patch, martin 107's patch might be updated with last one.
Comment #47
martin107 CreditAttribution: martin107 commented@ Jalandhar no problem, a happy mistake, critical issue will draw lots of attention... if there was any mistake is was myself not assigning it to me to prevent duplication
of your work lets see what testbot thinks of yours.
PS
Just did a diff of the two reroll patches.. virtually identical.. one patch renames a file another deletes files and add a new files .. both are equivalent
Comment #48
Jalandhar CreditAttribution: Jalandhar commented@martin107,
Hmmm. I think testbot also thought positively like you...:P. Tests got passed..:)
Comment #49
xjmThanks @Jalandhar and @martin107. Next we need to update this based on comment #37 following #2238069: Create a way to add steps to configuration sync.
Comment #50
Gábor Hojtsy45: language-override-storage-44.patch queued for re-testing.
Comment #52
Gábor HojtsyRerolled to apply again. Not sure how/if this is even going to work with the new database storage :)
Comment #53
BerdirI will try to update this this weekend to subclass DatabaseStorage instead and use a different table with a langcode column and open a follow-up/separate issue to make cached storage get support for cache prefix as we'll no longer need it here until we want to put a cache in front of database storage.
Comment #54
BerdirOk, let's try this.
- Removed cached storage and file storage changes
- Added a new database storage subclass, seems to be working fairly well but have to copy a lot of code because we have to add an additional condition to every query.
- Also added a deleteAllLanguages() which is deleteAll() for all languages, needs a better name but does make uninstall much easier. Not sure if we have test coverage for that?
Comment #56
BerdirThe null/false fails are because I changed read() to call readMultiple() and return NULL. Can be easily changed to FALSE but we should probably look into changing that to return NULL as we did elsewhere (like loading entities).
Everything else seems to pass, so now we need to solve export/import/sync.
Comment #57
xjmThanks @Berdir!
Next up, Mr. Pott.
Comment #58
alexpottPatch:
Now working on the configuration sync part of this.
Comment #59
Gábor HojtsyReviewed those changes:
Would be important to qualify this with an issue link :) Also fix to make it a proper sentence.
Good rename. This does not delete languages :D
switching/changing repetition :)
Incomplete doc.
Comment #60
alexpottThe patch attached adds the ability to synchronise language overrides. However I think this approach highlights several issues:
I attach the patch so people can see how ugly this approach is - I really do not want to go this way.
What I want to do is #2262861: Add concept of collections to config storages
Comment #62
alexpottpostponing on #2262861: Add concept of collections to config storages
Comment #63
alexpottRolled the patch on top of #2262861: Add concept of collections to config storages to ensure that collections is going to work out.
Comment #64
alexpottSo
So this code means that all language overrides are always installed regardless of whether the language exists on the site or not. I think this is the wrong approach. We should only be create language overrides for languages that exist. Especially since if you look at the uninstall code we're only uninstalling overrides for languages that exist.
Comment #66
alexpottFixed up test failures due to bug in collections code - new patch posted on issue. Improved install code according to #64. Now using collections in ExtensionInstallStorage which means that if language is enabled and then views is enabled and an another module provides views and translations in languages that are enabled they'll be installed at this point.
Comment #68
alexpottRefactored ConfigInstaller to work with collections so that Language can re-use to install overrides.
Comment #69
alexpottTestbot why did you not test?
Comment #70
alexpottKeeping up with #2262861: Add concept of collections to config storages which no includes code to handle installation, uninstallation and more.
Comment #71
alexpottless code++ :)
Comment #72
Gábor HojtsyPostpoed #2268939: Config overrides not updated when config changes (Critical beta target bug) on this one, since this introduces the infrastructure for it to be implemented sanely.
Comment #73
Gábor HojtsyComment #74
alexpottNow on top of 8.x as collections is in.
Comment #75
alexpottOops forgot to commit the deletes of the old language config overrides...
Comment #76
Gábor HojtsyYay, I love how this patch makes this possible :)
Typehint with string.
This method is a close replica of the save() method on Config, except there is no save event fired (good) and no validation delegation if there was no schema (confusing). How is that validation applicable there but not here?
Also once we do that validation, there is only 1 line of difference between the two methods.
Would be important to qualify this todo with an issue link.
I see this is unqualified the same way in Config.php and that delete is also very similar but not exactly. Of course the differences are THE reason we embarked on this whole process to begin with and PHP does not provide us with an easy way to do cross-branch inheritance like this :) At least we should cross-reference the two implementations as similar then so someone who fixes one may notice there is another similar copy.
Or alternatively move the differing logic to methods in the base class invoked from save()/delete() with empty implementations in StorableConfigBase and no implementation in LanguageConfigOverride and an actual implementation in Config. Would be nicer in terms of less code copied. :)
I don't think we abbreviate config like that? We write out "configuration" in docs if possible.
How would the rest of the code work if language module is not enabled?!
Comment #77
alexpott1. Ahh we should test collections config too in
DefaultConfigTest
-fixed - although this opens a can of worms - see below.2. Fixed
3,4. I've implemented a \Drupal\Core\Config\StorableConfigBase::preSave() method which limits the amount of code duplication to just the necessary differences whilst keeping the save and delete methods abstract and their implementations concerned with doing just that. The @todo is a copy and paste. Considering it was added in #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc) I we've had long enough to consider removing and we've not done it so removing. It is difficult to know how to add @see's to ensure that we make the correct changes to both Config and LanguageOverrideConfig.
5. Fixed
6. Not sure I get the question.
Can of worms
In order to get this configuration to validate and use schema correctly I've added the plugin key since the schema is dynamic and determined by the plugin - how are we going to do this in practice?
Comment #78
alexpottOkay so @Gábor Hojtsy and I agreed to descope use of schema during language config override saves since through using collections we already get the big wins. So the can of worms described in #77 has been descoped to #2270399: Use configuration schema during language override save
Comment #79
Gábor HojtsyOk, only 76/6 is left then. I asked if the test uninstalls language module, how would overrides even work afterwards? I don't get how that works. In the test. Maybe worth some added comments at least...
Comment #80
alexpottSo we're disabling language module to test enabling during the configuration synchronisation - so once the import is complete the language module should be enabled and the overrides working - which is exactly want is tested.
Comment #81
Gábor HojtsyRight, the way that happens is the two key lines above where the current copy is moved over to staging before language module is uninstalled, so that the only difference at that point is language module would be enabled with the sync. Then it adds language overrides to staging too, so then the diff is staging has language module enabled and language overrides set. Then the import proves the overrides have been loaded.
That seems to be completely testing what we aim to achieve here, and all other pieces look good now, so let's get this in :)
Comment #82
alexpottSo whilst thinking about how to solve #2270399: Use configuration schema during language override save I realised that we have a problem since during config install and config synchronisation we're not using the new LanguageConfigOverride class to create configuration - we're using the regular Config class which means events are fired. We need to change how collections interact with the ConfigInstaller and ConfigImporter and delegate Config object creation to an overrider if necessary. The patch attached adds a test to show what I'm on about - obviously it is going to fail.
Comment #84
alexpottAdding test for configuration synchronisation too... will fail.
Comment #85
alexpottNow for a patch that should make the two new tests pass. The event ConfigCollectionNamesEvent is renamed to ConfigCollectionInfoEvent since it now has the ability to get and set config factory overriders that are responsible for each possible collection. If a config factory overrider is responsible for a collection then the installer and importer delegate the responsibility for Config object creation to the overrider thereby allowing the use of LanguageConfigOverride instead of Config.
Comment #87
alexpottSo the patch in #84 did not have the expected additional failure because the event dispatcher was not refreshed after installing a module.
The will-fail.patch does not tie up the collection to the factory override so this should produce the expected fails.
The patch also moves the collection info getting to the ConfigManager so the ConfigImporter and ConfigInstaller can share code.
Comment #89
xjmMissing class docblocks here btw.
Comment #90
Gábor HojtsyI reviewed all the interdiffs individually up until 87. I did review 87 as well, but the renames/doc fixes make sense there, so no comment on those. Just keep this in mind as some notes may not be 100% the updated code. Overall this looks a bit sad as we said storage collections would not be tied to config access APIs but looks like sync/install has no other way and it needs to backtrack that and this patch introduces APIs distinct for accessing "override collections" and "non-override collections" (AKA the default collection), which distinction is not otherwise spelled out. And then the code to do this is special casing the two...
The test makes total sense and shows the problem :)
Can we use the existing language override test data to do this? Looks like overkill to me to add one more module for this.
minor: "are created" IMHO
This almost entirely overlaps with the other added test method, so why not test both there?
One has no type, other has no variable name.
Why not use addCollectionName() and do away with this hair splittling array combine business going on in addCollectionNames(). Ie don't have a method to mass-add collections, since we need to assemble that array to do it anyway.
Would be good to define what is this event.
Its combined with a reference to their implementation now, so need to document that. its keyed by collection name with references or NULLs.
Would the default collection name by no chance be in the list on the event? Ie. no chance of it being duplicate when we add it at the start? I guess that would be because core does not respond to this event or fills in the default...
Its unfortunate we need to special case it here, I hoped we could encapsulate this on the service, but they need very different method arguments :/
It looks strange to me we keep passing around an event with the information and not the information itself. Why would the rest of the API need to be concerned with what API was used to gather this information?
Comment #91
alexpott@xjm's review in #89 - fixed
@Gábor Hojtsy's review in #90
Also did some minor clean up of the patch.
Comment #92
alexpottI'm not happy with this method being called this on the ConfigManager. It is too easy to confuse this with the whole purpose of the ConfigFactory - which is to return Config objects :)
Comment #93
Gábor HojtsyI wanted to put my instantiation doubts into a visual so it is clear what I have in my head that hurts. I must be missing something and this is much simpler... This is how I understand it with the patch in this issue:
So if I'm a contrib maintainer I think this is what I need to understand to be able to tell when to use what. Now correct all the fallacies of this figure :)
Comment #94
alexpottI think it is more like this...
Comment #95
Gábor HojtsyYeah that is pretty much the same just a little simplified from what I have. My point is this is complex. Much more complex than what we used to have. Of course you either know the implementation to use or you don't. In the former case that will use the right config storage collection in the later case you need to ask the config storage collection. Those may be different enough that no middle ground is possible. In that case I'll just spend some time to mourn the simplicity :D
Comment #96
alexpottThe ConfigManager::createConfigObject really bothers me - removed it and added more documentation.
Comment #97
Gábor HojtsyRight, so at least we explored how this complicates things and the docs got better. I think the language handling itself is good, the collections changes needed are just based on how collections in combination with sync and install are and that we need to implement instantiation backwards there. I think this is ready for catch's eyes then.
Comment #98
catchHaven't got much time this week, so only a partial review.
Overall this looks complex for configuration overriders, but there's not going to be that many of them and it does centralize things a lot.
Couple of minor things:
Any reason why these have to be declared via an event rather than static YAML?
Also extra stray apostrophe in "Configuration overrider's".
If I want to get a list of overriders and collections how do I get that?
Comment #99
alexpott\Drupal::service('config.manager')->getConfigCollectionInfo()->getCollectionNames()
.\Drupal::service('config.manager')->getConfigCollectionInfo()->getOverrideService($collection_name)
Comment #100
catchHmm I guess that'd be the same for domain as well (one collection per-domain).
I realised I didn't get to review #2262861: Add concept of collections to config storages before it went in, so I'll go back over to there, then return here where things should make more sense.
Comment #101
catchRead through #2262861: Add concept of collections to config storages and this patch again. Agreed with Gabor's comments in #97 but the trade-off seems OK and I don't have any better ideas. We've still got some time to try to clarify things a bit more as well.
Committed/pushed to 8.x, thanks!
Comment #103
Gábor HojtsyYay, great to see this landed :) Woot!
Comment #104
Gábor HojtsyAdded a change notice for this at https://drupal.org/node/2274351, including the instantiation decision tree.