Updated: Comment #44
Problem/Motivation
Import process cannot handle modules being enabled/disabled. This issue was discussed at length in Prague. The UI with respect to synching configuration probably needs to special case enabling and disabling of modules. Anything that triggers data deletion needs to notify the user. This is related to #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall but we should remember that during config import that core only support importing a complete config tree.
Proposed resolution
Make the configuration import a 2-step process: (1) handle any module changes, then (2) do the current import process. Account for the various scenarious outlined in #31
Remaining tasks
- (done) Waiting on #1969800: Add UUIDs to default configuration
- (ongoing) open follow-up and "good idea" issues found while working on this issue
- check to make sure all follow-ups and related issues were opened (remove the needs tag when that is done)
User interface changes
None
Related Issues
#1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed
#1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config
#1890784: Refactor configuration import and sync functions
#1918926: Module dependencies are not respected when default configuration is imported
#1969800: Add UUIDs to default configuration
#2029771: Having installation profiles in system.module.yml causes config import to fail
#2029819: Implement a ThemeHandler to manage themes (@see ModuleHandler)
#2030073: Config cannot be imported in order for dependencies
Blockers of this issue
None.
(was blocking) #2030073: Config cannot be imported in order for dependencies
Follow-ups
Follow-ups needed because this was committed:
- #2226823: Make default views displays configuration have correct "position" values
- #2234159: Fix config importing through the UI and change ConfigImportAll test using the UI
- [#]
Spin off to separate issues
Good ideas of separate parts discussed or found during this issue, but did not blocking this, and might have been done even if this issue had not been fixed (not follow-ups)
- #2233929: drupal_set_time_limit should not be able to change the time limit if it's already unlimited (noticed as a result of this being a committed, but already an existing issue)
- [#]:
Comment | File | Size | Author |
---|---|---|---|
#128 | 1808248.128.patch | 84.58 KB | alexpott |
#128 | 125-128-interdiff.txt | 1.97 KB | alexpott |
#125 | 1808248.125.patch | 84.27 KB | alexpott |
#125 | 123-125-interdiff.txt | 810 bytes | alexpott |
#125 | progress.png | 23.73 KB | alexpott |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedi have some code i can resurrect for this, if i don't post a patch by saturday, feel free to reassign.
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedtagging.
Comment #3
andypostMaybe better change title to - Allow configuration import to enable/disable modules :)
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #5
sunYes, two passes sound sensible to me.
This shouldn't actually be very hard; I expect a ~10 lines patch here.
There's one complexity to this though:
1) Modules that need to be enabled need to be enabled before executing the import.
2) Modules that need to be disabled... are a different can of worms. Upfront or after? Will import hooks have to be executed or not? Dependencies?
3) Modules that need to be uninstalled... oh.my.holy.f/me runs.
And: Speaking of hooks in 1) and 2), I guess we actually need to run through module_enable() + module_disable().
Lastly, do we handle themes at the same time? Or separate issue? Or not at all? (I'd disagree with the last option.)
Comment #6
chx CreditAttribution: chx commentedThis rabbit hole is very deep. I suspect that we need to define an explicit dependency order between CMI objects (and have some sensible defaults, very likely building on the module dependencies) and import in that order and perhaps reboot Drupal (batch API? Just a big bad reset? You need to reset statics and rebuild the DIC, at least) between steps. It's a gigantic mess.
Think of modules, field types, entity types, entity bundles, fields, instances, actions and what dependencies they can have on each other.
Comment #7
sunBecause it wasn't mentioned yet, it needs to be asked, since we briefly discussed the alternative option a while ago:
What if we'd apply a special behavior to extension handling in the config import process?
I.e., instead of enabling/disabling extensions the normal way, we just update the respective system lists, and if necessary, install the schema. That would exclude hook_install(), hook_enable(), hook_disable(), and installation of default config. We'd reset/reboot once after doing that, and then go ahead with the existing import process.
This alternative sounds scary to me, but mayhaps, it is less scary than the problem space of having to deal with interdependencies between extensions.
OTOH, dependencies in config should actually be off-topic here -- that's what #1605460: Implement dependencies and defer process for importing configuration is about. Dependencies between extensions are properly handled built-in already; that's what the second argument to module_enable() & co is for. Perhaps it makes most sense to focus and progress in a pragmatic way.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedat this stage, i think all options should stay on the table. i'm almost finished a first cut naive-as-possible patch.
i definitely see it as a discussion starter - i'm ok with rejecting the approach i'm taking entirely if we can come up with something better.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedfirst cut attached with simple tests.
handles importing module enable, disable, unintsall and enable-disable cases.
i've taken a blindly optimistic and naive approach to this code, but it will hopefully give us something to shoot at in the form of tests for nastier scenarios that break this code.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commented#9: 1808248-9-config-import-modules.patch queued for re-testing.
Comment #12
sunSorry for the noise, just trying to clarify the issue title. Changes in the system lists are actually imported correctly already (just like any other config). The actual problem is that newly enabled extensions do not participate in the config staging/import process; i.e., their config import hooks are not invoked.
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedack to #12. those tests pass locally for me :-(
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedaaaaaand they test locally because i forgot to git add, git commit the test module and yml file to my local branch.
new patch with those files.
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #17
Anonymous (not verified) CreditAttribution: Anonymous commenteddowngrading so it doesn't block other features - we're right on 100 majors right now.
Comment #18
alexpottThis is an important issue and one of the reasons why I started on #1890784: Refactor configuration import and sync functions
Another thing we need to consider apart from just the config_import hooks are any install / disable / schema hooks. At the moment these are not fired at all.
The attached patch (which will fail) shows that after a config_import that updates system.modules - the module does not exist on the service, the schema is not installed and default config is not installed.
Comment #19
sunI'm pretty sure we cannot release without this, so bumping to critical.
The latest patch in #15 looks a bit messy ;) but I can perfectly see that the required logic will be relatively ugly either way.
Given the discussion results of #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed — I'd suggest to simply and strictly deny importing any config in case any module is disabled. Removing that exceptionally horrible edge-case from the equation will likely make the logic a lot easier.
Furthermore, do we want to bake this into the existing syncing functions, or do we perhaps want to split the functionality into completely separate functions? (and change the existing to ignore those special config object names) I'd assume that splitting it out would help us to make the code a little bit cleaner?
We also need to update this for the new module_handler service.
Comment #21
tim.plunkettLooking at the code in #15, and realizing we don't have a current solution for this yet, I think it would make sense to postpone this on #1890784: Refactor configuration import and sync functions. It will be easier to tackle once that is done.
I do agree that this is a critical that needs to be fixed.
Also, if #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed is decided, it will make this more simple.
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedaaaaand that landed a while back, so this is open again.
alexpott has some code locally for this.
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedretitling. i'll try to get a patch up for this today.
Comment #24
alexpottPatch attached adds
Comment #25
alexpottComment #26
alexpottRemoved prioritisation step as it is out-of-scope and untested...
Recalculating differences after enable / disable / uninstall...
Handling errors a bit better...
Comment #28
alexpottFixes installer and ensure that during a module or theme enable we don't end up in some mad recursion...
Comment #30
alexpottAnd to fix the view installer...
Comment #31
alexpottSo at the moment this recalculates the changelist after installing modules and themes as these actions create config in the active store and therefore stuff is going to have changed - @beejeebus thinks this is the way to go. Last night I said I wasn't sure and now thinking more about it I'm convinced and here is why...
Scenario 1 - with code in #30
Starting condition DEV and LIVE matching environments from configuration perspective...
On DEV
Then on LIVE import config
Scenario 2 - with code #30
Starting condition DEV and LIVE matching environments from configuration perspective...
On DEV
Then on LIVE import config
The point being we now have not followed exactly the same set of renames on LIVE as on DEV.
Scenario 2 - how I think it should work
Starting condition DEV and LIVE matching environments from configuration perspective...
On DEV
Then on LIVE import config
I think the given that two systems A and B start in exactly the same state. System A can take what ever path it likes to get to a point where it exports. On importing system B should take the shortest possible path to get to the expected config. As there is absolutely no way we can know the exact path the system A took to get to the point where it was when it exports its config.
... however this still does not get away from the fact we still have to implement renames. Because of this situation... it is just a different problem and out-of-scope for dealing with module and theme enables...
Scenario 3 - why renames are vital
Starting condition DEV and LIVE matching environments from configuration perspective...
On DEV
Then on LIVE import config
Then on DEV
Then on LIVE import config
Comment #33
tim.plunkett#30: 1808248.30.config-import-enable.test-only.patch queued for re-testing.
Comment #34
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #35
Anonymous (not verified) CreditAttribution: Anonymous commentedi'm meh about #30 and #31, but it's green, so lets go with it.
Comment #36
tayzlor CreditAttribution: tayzlor commentedHere's an updated patch. It follows more closely with "Scenario 2 - how I think it should work" in #31.
I've overridden the module handler to provide a config one, which is available during a config import. This allows us to bypass the config_install_default_config() call during a module enable, instead importing from the full config tree for the module that is being enabled.
To test this patch, what I was doing was getting a config export from a standard profile install, then doing a minimal installation, and trying to do a config import to take us from minimal > standard.
I've included an additional test that does just this, which should fail. This patch does the config import and manages to take us nearly all the way from minimal > standard, but there are some outstanding issues.
Comment #37
tayzlor CreditAttribution: tayzlor commentedPost save hooks are a bit of an issue. Couple of examples -
Custom_block module. It creates a field by doing custom_block_add_body_field(). if this field is coming in later on in the config import (from another config file) it will break.
Contact module - references $this->original->id() (where original may not actually exist if we are doing a config import)
Also because we are calling theme_enable() this installs the default config (as config_install_default_config() is embedded in that function). What we would possibly need here is a themeHandler class (similar to the moduleHandler), where we could do a similar override (to the one i've implemented in the ConfigModuleHandler) when we are enabling a theme via a config import.
Comment #39
tayzlor CreditAttribution: tayzlor commentedSince the node types conversion to CMI landed, here's an updated patch that includes those YAML files in the staging directory for the test minimal to standard config import.
Comment #40
andyceo CreditAttribution: andyceo commentedComment #42
tayzlor CreditAttribution: tayzlor commentedThis issue depends on #1969800: Add UUIDs to default configuration being committed.
Comment #43
tayzlor CreditAttribution: tayzlor commentedLinking some more related issues here -
#1918926: Module dependencies are not respected when default configuration is imported
#2029771: Having installation profiles in system.module.yml causes config import to fail
#2029819: Implement a ThemeHandler to manage themes (@see ModuleHandler)
#2030073: Config cannot be imported in order for dependencies
Comment #44
Anonymous (not verified) CreditAttribution: Anonymous commentedEDIT - remove unhelpful comment.
Comment #45
Anonymous (not verified) CreditAttribution: Anonymous commentedmarking this as blocked on #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed.
Comment #46
David_Rothstein CreditAttribution: David_Rothstein commentedIs there a reason this is postponed? It looks to me that disabled modules only come into play here via a few small lines of code in the handleModules() method.
Comment #47
Anonymous (not verified) CreditAttribution: Anonymous commentedsigh.
Comment #48
Anonymous (not verified) CreditAttribution: Anonymous commentedso here's a first cut that is almost certainly broken in important ways.
Comment #50
Anonymous (not verified) CreditAttribution: Anonymous commentedViewTestConfigInstaller extends ConfigImporter. no, no, no, this is getting out of hand.
Comment #51
Anonymous (not verified) CreditAttribution: Anonymous commentedcreated #2095115: delete ViewTestConfigInstaller and have ViewTestData create views, not 'import' them to start undoing the damage in Views.
Comment #52
Anonymous (not verified) CreditAttribution: Anonymous commentedsetting this as postponed on these two:
Comment #53
Anonymous (not verified) CreditAttribution: Anonymous commentedi've updated the patch to include #2095489: Separate out module install config code from import code - i'd like to see how this runs now.
Comment #55
Anonymous (not verified) CreditAttribution: Anonymous commentedshould be less fails with this one.
Comment #57
Anonymous (not verified) CreditAttribution: Anonymous commentedso. bad. at. this. game.
uploaded the wrong patch to #55. this is the one i meant.
Comment #59
Anonymous (not verified) CreditAttribution: Anonymous commentedless durp.
Comment #61
Anonymous (not verified) CreditAttribution: Anonymous commentedeven less durp.
Comment #62
Anonymous (not verified) CreditAttribution: Anonymous commentedbump.
Comment #63
Anonymous (not verified) CreditAttribution: Anonymous commentedreroll.
Comment #64
Anonymous (not verified) CreditAttribution: Anonymous commentedi'll try to write a test for install/uninstall modules now that #2106171: Write tests for simple configuration deployment scenario has landed.
related - #2108813: Add fancier config import/export test scenario.
Comment #65
Anonymous (not verified) CreditAttribution: Anonymous commentedthis adds a test that just installs config_test.
fails with:
this is why we can't have nice things.
Comment #66.0
(not verified) CreditAttribution: commentedmtift updated issue summary
Comment #66.1
alexpottUpdated issue summary.
Comment #67
xjmComment #68
alexpottComment #69
chx CreditAttribution: chx commentedAllow me to express my doubts over the direct approach here. Shouldn't this work via some event? Of course it would require that modules CMI object get processed first -- as I expressed above, the CMI dependency is a problem.
Comment #70
alexpottComment #73
Gábor Hojtsy@alexpott: are you working on this? Any feedback needed? :)
Comment #74
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #75
Nitesh Sethia CreditAttribution: Nitesh Sethia commentedComment #76
Nitesh Sethia CreditAttribution: Nitesh Sethia commentedPatch has been rerolled.
Comment #77
Nitesh Sethia CreditAttribution: Nitesh Sethia commentedComment #79
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #80
Anonymous (not verified) CreditAttribution: Anonymous commentedreroll based on #70. lets see where we're at.
Comment #81
yched CreditAttribution: yched commentedHate to be dropping this so late :-/, but shouldn't we defer the uninstalls to the end of the sync process ?
- install modules that need to be installed according to system.module.yml
- sync the config tree other than system.module.yml
- only then, uninstall modules that need to be uninstalled according to system.module.yml
?
For the same reason "enable first" is needed so that we can handle the creation of the config entities owned by the "new" modules, I'd think we want to "disable last" so as to keep "old" modules active until we're done dealing with their config entities ?
Comment #82
alexpott@yched good point! You're not late we're only just getting started with this issue :)
Comment #84
Anonymous (not verified) CreditAttribution: Anonymous commentedre. #81 - huh, took me a while to figure out what that meant, but yeah, yched++
i'll add that in to the next patch.
Comment #85
xjm@alexpott suggested doing this after #2030073: Config cannot be imported in order for dependencies. See new parent issue for details. :)
Comment #86
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #87
jessebeach CreditAttribution: jessebeach commentedComment #88
jessebeach CreditAttribution: jessebeach commentedUnpostponed!
Comment #89
alexpottRerolled patch - we need to consider how to do this now that the Config Importer is batched. Patch attached just does all the module / theme installing and uninstalling in
BatchConfigImporter::initialize()
which is wrong.Comment #91
alexpottComment #93
alexpottSo we need to ensure Extension storage has an up to date list of enabled modules and themes during regular default configuration import.
Comment #95
alexpottFixed up some of the test fails.
settings.translation_sync
issue will be resolved in #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configurationComment #96
alexpottComment #98
alexpottThe views were being ordered inconsistently during import because the default configuration had inconsistent position values.
Fingers crossed this is green :)
@todo: implement sensible batching of extension install and uninstall during config import.
However the ConfigImportAll test passes!!!! This test installs the standard profile. Then installs all available modules. Exports the configuration to staging. Deletes all fields and then uninstalls all possible modules and then syncs the site using the exported config to restore everything..
Comment #99
sunHm. No (new) test should rely on Standard profile.
cf. #1595028: Convert tests using Standard profile to use Testing profile instead
Any chance to convert that into DUTB + test the actual regression vectors instead?
I'm explicitly stating DUTB, because this test does not and should not care for anything else than installing, importing, and exporting configuration.
That said, it is not clear to me why such a test has to install all available configuration of all available modules in the first place... Tests should exclusively focus on possible test cases (possibly permutations) only. Installing a bunch of random modules that happen to have some random configuration does not cover any particular test case and only slows down the overall test suite performance. Those are the kind of tests that are guaranteed to get into your way, and everyone will avoid to run them at all costs ('cos a single test run takes ages to complete).
So, what are the actual test cases to cover?
Comment #100
alexpott@sun this explicitly tests that all non-required modules can be enabled through the config importer and that the default configuration created by a standard install can also by created reliably through the importer. If you do not want to rely on standard profile - what we could do is enable all the modules and then import the configuration from the standard profile. But in my mind this is still relying on the standard profile. Further more I'd like to add the converse test that all non-required modules and their configuration and config entities can be removed by the config importer.
Comment #102
alexpottCommentManager::addDefaultField needs to hide the comment field on any existing entity displays. Interdiff impossible so a real diff of the two patches attached.
Comment #105
alexpottFixing the new test
Comment #106
YesCT CreditAttribution: YesCT commentedrerolling this since, #2226823: Make default views displays configuration have correct "position" values got in.
Comment #107
Anonymous (not verified) CreditAttribution: Anonymous commentedworking on this.
Comment #108
Anonymous (not verified) CreditAttribution: Anonymous commentedcurrent patch. import UI tests are failing, and i don't know how to fix it.
Comment #110
alexpottThis patch adds:
Mind you this patch is very very ugly at this point :(
Comment #111
alexpottPatch attached implements a better way of dealing with default and admin theme changes after theme enables and (more importantly) before theme disables.
Comment #112
alexpottRefactored for #2228921: Consolidate system.module + system.theme + system.theme.disabled into core.extension config
Comment #113
BerdirWow, this is pretty crazy...
Looked through it, found a bunch of nitpicks and some minor issues, but I don't have any glorious ideas to make it all much easier and nicer...
Nice test coverage!
insertMissingDocumentationNitpickHere()
I don't think we do this.
Seems unrelated?
Aw.
Missing type.
Left-over?
docs...
Those two checks seem the conflict, the second one will never run then?
Cut off comment.
uninstalled ;)
Also, I *thought* we fixed this so that fields are uninstalled correctly, but it's possibly that there are some left-overs...
All modules aren't actually all modules anymore at this point, maybe use a different variable?
first sentence seems to be missing some words or so ;) Also, those two are now already enabled for the test? and are not in the list?
Ah, here we go ;) Still, the comment above seems outdated.
Not related, but pretty sure that this assertion is completely bogus. this happens inside the page request, it doesn't affect our global? It works because we do a dfac() during test setup I guess, should also be green when checking before you import ;)
two spaces :p
needs some inheritdocs on this and other methods.
.
I guess this is a workaround until that is in it's own files?
The fix should be the other way round? entity_type.fieldname, and taxonomy_term is the entity type, not forum..
All those checks and edge cases are becoming more and more complicated :(
Unrelated but this could use hook_entity_bundle_delete() in language.module now, which is triggered by entity_module_preuninstall().
There's also talk of moving those settings into the language field settings.
Comment #114
alexpottThanks @Berdir.
1 - 17. All fixed
18. Yep :(
19. Yes... nice catch
20. Indeed
21. Agreed but out of scope :)
Comment #115
Anonymous (not verified) CreditAttribution: Anonymous commentedalexpott++
berdir++
i found this really hard to follow. can we break it up in to some methods with names? or add a big docblock or something? i don't even know what to suggest, because i just don't follow what it's doing.
Comment #116
alexpottHow about breaking the batch up into truly separate steps for extensions and configuration - the attached patch does this and I think it is easier to understand.
Comment #117
Anonymous (not verified) CreditAttribution: Anonymous commentednice! #116 is much clearer. i think this is getting close, but i don't think i should RTBC it.
alexpott++
Comment #118
alexpottBeen testing this patch by doing the following:
Fixed an interesting issue with recreates going in in the wrong order due to being added after the creates and deletes.
Comment #120
sunThis looks great! Reviewed the patch/code (did not apply and/or test it, but #118 is more than sufficient proof for me :-))
s/system.theme/core.extension/ ?
Shouldn't this also reset
$processedSystemTheme
?In reviewing this patch, I found all the "Unprocessed" method + variable names very confusing — they sound as if they're about an unprocessed data/domain object.
Would be great to rename all of them into "Pending" (or similar).
Briefly discussed with @alexpott in IRC... let's do that in a follow-up issue.
Is this @todo still an open question?
Oh wow.
Since the workarounds appear to work for now, let's create separate follow-up issues to investigate whether we can resolve this cleaner/differently?
This comment (at least the parts about an absence of dependencies) sounds obsolete, no?
There's a small mismatch here:
If it's legit to pass an empty array for
$sort_order
, then the condition should beisset()
.Otherwise,
$sort_order
should probably default to an empty array.Hm. Doesn't the
ConfigInstaller
/ConfigImporter
handle this already?Also, wondering why we're not simply tagging config.installer with 'persist' instead?
Wouldn't that resolve all of the manual futzing with re-injecting the previous state into the config installer/importer...?
Wasn't this in-process rebuild from
ModuleHandler
the exact reason for why we introduced the 'persist' tag for services?Oh wow. I wonder why I didn't run into this problem in #1067408: Themes do not have an installation status ...?
At least I assume that comment to actually try to say:
"ExtensionInstallStorage only knows about the currently enabled list of themes, so it has to be reset in order to pick up the default config of the newly installed theme. However, do not reset the source storage when synchronizing configuration, since that would needlessly trigger a reload of the whole configuration to be imported."
No? Wanna take over that as a replacement comment? :-)
minor
Comment #121
alexpottThanks @sun
Fixed the test fails. These where due the sorting in StorageComparer::addChangeList() making the array keys not start from 0.
Comment #122
sunOK... I don't really agree with the approach taken just to avoid 'persist', but I guess we should simply move forward with this.
throw *new* ? :)
Comment #123
alexpottDoh!
So the other reason for not using persistent services is that it means that a module can not ever swap out the service. We'll just copy the old one over.
Comment #124
sunThanks! — Yeah, OK, let's move forward here.
The topic of persist + kernel rebuilds is a super complex topic of its own and definitely shouldn't hold up this patch.
Comment #125
alexpottImproved batch progress message.
See http://www.screencast.com/t/NXI1Xu5VQ3 for incorrect message.
Comment #126
BerdirI didn't do an in depth review of the latest patches, but +1 from my side, can't spot anything that is wrong and if sun and beejebus are OK with it then I am too :)
Comment #127
catchDiscussed some of the issues like the order of install/uninstall with Alex last week and have no issues.
Overall this looks great. I found some very minor nitpicks that we could handle in a follow-up, posting here rather than ignoring though. The container syncing issue is horrible but not at all the fault of this patch, we've got other issues open trying to figure that out.
Missing docs.
Also here.
During a container rebuild the module handler is called? Couldn't parse this. This should be fixed by #2194785: [meta] Stop relying on database schema info at runtime so could add a @todo.
If I have time for another read through, I'll commit this a bit later today, shouldn't stop another maintainer committing it if they get back here first though :)
Comment #128
alexpottFixed nits from #127
Comment #129
catchCommitted/pushed to 8.x, thanks!
Comment #131
dawehnerThis issue seems to be a potential source for "random" failures: https://qa.drupal.org/pifr/test/764353 is one example but I have seen that elsewhere.
I wonder whether we could file a followup which don't enables all the modules at once. On top of that it would be great to exclude contrib modules in sites/*/modules or modules/
Comment #132
YesCT CreditAttribution: YesCT commentedI'm going to open the follow-up for #98 "@todo: implement sensible batching of extension install and uninstall during config import." right now.
just putting the structure in place to track them in the issue summary.
--
Also, quickly noting re #131, #2233929: drupal_set_time_limit should not be able to change the time limit if it's already unlimited can deal with it (but the #98 follow-up might actually deal with the cause).
Comment #133
xjm#2234159: Fix config importing through the UI and change ConfigImportAll test using the UI is that followup
Comment #134
YesCT CreditAttribution: YesCT commentedthat was already a child. not sure if it needs to be related also. but these new fields are still new... not sure how best to use them.
updating the issue summary to note they are follow-ups though.
Comment #135
tim.plunkettAlso seeing the random fail, opened #2240709: ConfigImportUITest::testImport fails when the module list changes
Comment #136
effulgentsia CreditAttribution: effulgentsia commentedWas this implemented? If not, why not? If so, how? Looking at the code, I only see:
which from what I can tell, does installs and uninstalls in the same place, and before configs.
I'm curious, because #2198429: Make deleted fields work with config synch just landed, which front loads the purging to the beginning of the process, rather than between processConfigurations() and (the nonexistent) processUninstalls().
Comment #137
alexpottNo config uninstalls are not deferred. This is because they could affect config creation - that's why uninstalls are done first before installs. The other thing you is the final environment the config export came from - it is important to get to the same code base as soon as possible. I'm going to open an issue to add documentation to the ConfigImporter class about priority and ordering of everything.
Comment #138
effulgentsia CreditAttribution: effulgentsia commentedThat makes sense. Thanks.
Comment #139
sunCreated #2258247: Remove useless Drupal\config\Tests\ConfigImportAllTest