Updated: Comment #0
Suggested comment message
Issue #2082117 by alexpott, xjm, tim.plunkett, heyrocker, beejeebus, sun: Fixed Install default config only when the owner and provider modules are both enabled.
Since this was split from the meta issue.
Problem/Motivation
Based on various IRC discussions with alexpott, xjm, mtift, and others, this is a sub-issue of #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API that focuses exclusively on what happens to configuration entities when modules are installed. Many of the problems are discussed in that other issues. The options are basically to install default config or not.
Please note that #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall covers the topic of uninstallation.
Proposed resolution
One suggestion is to only install default config when both the providing module and the config entity defining modules are (or will be) installed. The proposal is to create a new ExtensionInstallStorage class that extends InstallStorage and that can be used in config_install_default_config() to determine if an extension provides default configuration for any other enabled extensions. This is the approach used in #1776830-75: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API.
Remaining tasks
Decide what to do when a module is installed that provides default configuration that uses another module's API
User interface changes
None. However, this patch might enable changes to the UI during module installation.
API changes
TBD
Related Issues
#1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed
#1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API
#2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall
Comment | File | Size | Author |
---|---|---|---|
#32 | 30-32-interdiff.txt | 2.42 KB | alexpott |
#32 | 2082117.32.patch | 17.92 KB | alexpott |
#30 | 26-30-interdiff.txt | 2.57 KB | alexpott |
#30 | 2082117.30.patch | 17.88 KB | alexpott |
#26 | 23-26-interdiff.txt | 790 bytes | alexpott |
Comments
Comment #1
alexpottThis is currently blocked by #2082499: Uninstalling action module removes actions created by the user module since whilst action config entities are owned by the action module things just break eg. Drupal\user\Tests\UserAdminTest
Comment #1.0
alexpottUpdated issue summary.
Comment #2
alexpottAdding a patch that includes the fix from #2082499-6: Uninstalling action module removes actions created by the user module to get testbot to see where I'm at.
Comment #3
alexpottSome extra change to ./core/vendor crept into the last patch cause I was playing around with composer :)
Here is a cleaned up patch
Comment #4
alexpott#2082499: Uninstalling action module removes actions created by the user module has landed so removing it from this patch.
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedmissing whitespace...
too much whitespace...
otherwise, this looks good to go.
Comment #6
Anonymous (not verified) CreditAttribution: Anonymous commentedoh, i forgot to ask - what's up with the commented out bits?
Comment #7
alexpottPoints addressed - that test is now removed since this is now impossible as we search default configuration directories for the name plus a period.
Removed
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentednice work.
Comment #9
yched CreditAttribution: yched commentedThis doesn't look completely polished yet ? :-)
Also, there are phpdocs in this class that look like they should be {@inheritdoc}
Comment #10
yched CreditAttribution: yched commentedMore nitpicks :-)
name looks weird ?
Misses phpdoc
I might be wrong, but s/this/the/ ?
+ Could use an empty line and a "Check that the config entity doesn't exist before the config_test module is enabled" beforehands.
This is why the last part of the method makes sense, but this could use a word of explanation here.
Extra white line
Comment #11
alexpottThanks @yched - why is reviewing your own code is so difficult?
I've also improved the
ConfigOtherModuleTest
to test what happens when you uninstall the config entity provider. Whilst that's not actually changed by this patch it's good to have coverage of the current behaviour.Comment #13
yched CreditAttribution: yched commentedDon't tell me about it :-)
Gone into annoying / überpicky mode, sorry...
I still wouldn't make sense of that test name if I saw it in the test listing page (but maybe it's just me, feel free to ignore)
"cannot" ?
This sounds like one single sentence ?
Comment #14
alexpottThe reason the test fail is that I left a spurious assert in :(
Hopefully have fixed everything from yched's review.
Comment #15
yched CreditAttribution: yched commentedLooks good
Comment #16
catchInstalling default config for other modules only when the other module is installed makes complete sense.
However the patch currently feels like it'd be easy to get inconsistent behaviour.
So taking default views for example, seems like with the same module being enabled, two things can happen depending on whether Views is enabled before or after:
1. Views isn't installed. Module foo is enabled. No default views are imported. Views is enabled. Still no default views.
2. Views is installed. Module foo is enabled. Default views are imported.
I don't see anything in the patch that deals with that, so I'm either missing something or we likely need a new issue to discuss that - I think it can be a follow-up but still might need to be critical.
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commentedyep, #16 is a good point, and can be a follow up i reckon.
Comment #18
catchAlex pointed out in irc that this is actually covered by the patch, it's just easy to miss. Asked for a better comment.
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedhuh, yep, i had no idea InstallStorage did that. ignore me.
i really, really don't like the layout of these file storage objects.
Comment #20
Anonymous (not verified) CreditAttribution: Anonymous commentedoh yeah, i guess that means we're back at RTBC.
Comment #21
catchStill needs the improved comment.
Comment #22
yched CreditAttribution: yched commentedYeah, that part confused me as well when reviewing, I had to take a good look at the code to realize that this was the "collect the config in all other, already enabled modules, that hadn't been installed so far and can now be installed" part, and that the code indeed implemented the behavior advertized in the issue. The second half of the method is more akin to what you would expect, but that 1st part is easy to miss.
Maybe this just needs some explicit higher level comment explaining what config_install_default_config() does (and maybe starting with the second part makes things a bit easier to grasp):
- among the config items provided by the newly enabled module, install the ones that can be installed (those that are owned by modules that are available)
- among the config items provided by all other modules, install the ones that can now be installed (the ones owned by the newly enabled module)
1st part feels fairly intuitive, 2nd part is "a natural counterpart of the 1st part if you think about it"
Comment #23
alexpottHere's the new and improved comment, tests and some coding standards things I spotted :)
Comment #24
yched CreditAttribution: yched commentedWorks for me.
Comment #26
alexpottOh that was fun - the system.theme config was being cache in the factory because we where calling
list_themes(TRUE)
during atheme_enable()
.Comment #27
catchIt'd be good to add the additional comment yched mentioned in |#22 to config_install_default_config(), with that I think it'll be clear what's happening, the new comment should be enough with that extra context, but I'm still not sure I could figure it out scanning through the code not knowing what it does already.
Comment #28
yched CreditAttribution: yched commentedwell, +1 on #27 then ;-)...
since that comment would be about "what the function does" rather than "how it does it", it might be more helpful in the phpdoc ?
(the patch actually makes the current function description stale anyway)
Comment #30
alexpottYep moved the documentation into the function doc block and worked on the text with @xjm
Also hopefully fixed the test failures. Basically we need to ensure that the theme_list() is correct when calling config_install_default_config().
Comment #31
catchThat's great.
Comment #32
alexpottA couple of minor coding standard issues noticed by @xjm
Comment #33
webchickI think I'd sooner this be committed by catch than me.
Comment #34
catchCommitted/pushed to 8.x, thanks!
Comment #35.0
(not verified) CreditAttribution: commentedAdd a suggested commit message