Problem/Motivation
In #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference we introduced a ModuleUninstallValidatorInterface that allows preventing modules from being uninstalled. Currently book field filter and forum implement hook_system_info_alter() to achieve the same.
Proposed resolution
Transform the logic of the hook_system_info_alter() implementations to uninstall validator services.
Remaining tasks
Transform the logic of the hook_system_info_alter() implementations to uninstall validator services.
User interface changes
Modules show up on the uninstall page having the checkbox disabled and showing a reason for which the module can not be uninstalled.
API changes
Instead of using hook_system_module_info_alter() you can now register services tagged with
+ - { name: module_install.uninstall_validator }
which validate whether you can uninstall a module.
This is an API change, but its not the case that many contrib modules break it.
Beta phase evaluation
Issue category | Bug because the uninstall validator is more specific than hook_system_info_alter which has buggy side effects. |
---|---|
Issue priority | Major because it solves other Major bugs such as #2387983: PluginNotFoundException when enabling module that provides text filter |
Prioritized changes | The main goal of this issue is to make it more stable. |
Comment | File | Size | Author |
---|---|---|---|
#65 | 2392293.65.patch | 77.36 KB | alexpott |
#65 | 63-65-interdiff.txt | 956 bytes | alexpott |
#63 | 2392293.63.patch | 77.35 KB | alexpott |
#63 | 62-63-interdiff.txt | 5.06 KB | alexpott |
#63 | Screen Shot 2015-05-27 at 17.02.03.png | 118.5 KB | alexpott |
Comments
Comment #1
xjmComment #2
catchWould fix #2387983: PluginNotFoundException when enabling module that provides text filter which is a major bug, so bumping priority.
Comment #3
bircherComment #4
fagoAs written in the dup #2392363: [META] Make use of module uninstall validators, I'd suggest to do a change notice also:
> - For communicating the new API provide a change notice and point to a nice example implementation.
Comment #5
tim.plunkettWorking on this.
Comment #6
tim.plunkettThat was actually pretty fun.
I was able to fully inject everything and unit test the new classes.
Comment #7
tim.plunkettThe only thing I was unsure of were all the MAINTENANCE_MODE checks. All of the ones I converted had a check for that first, due it "not being safe" to use entity.query, entity_load_multiple_by_properties, or filter_formats.
I removed those checks for now. What was the concern there? Seems like you could really break your site in HEAD right now by uninstalling modules while in MAINTENANCE_MODE.
Comment #9
tim.plunketthook_system_info_alter() doesn't interfere with direct calls to $module_handler->uninstall().
But ModuleUninstallValidatorInterface prevents that API call from working. Those tests are still failing.
Because of #2392695: Move module dependency messages from the install form to the uninstall form, we need to adjust some test expectations for where to find their required messages.
Also, these can contain HTML.
Comment #10
cilefen CreditAttribution: cilefen commentedComment #12
cilefen CreditAttribution: cilefen commentedThe change record https://www.drupal.org/node/2392677
Comment #13
tim.plunkettCommentUninstallTest::testCommentUninstallWithField() ensures that a field exists, and then uninstalls comments, asserting that uninstalling comment will trigger field deletion.
Yet our new FieldUninstallValidator explicitly prevents that (and previously, field_system_info_alter() claimed it made the module required!)
So which is it?
Comment #14
cilefen CreditAttribution: cilefen commentedShould we add a @see to the docblock of hook_system_info_alter() telling developers to use ModuleUninstallValidatorInterface in cases like these?
Comment #15
tim.plunkettConfirmed with @alexpott that the tests are wrong. Modules shouldn't be able to be uninstalled if they are required, or meet any of these conditions. To prove that these test failures aren't really the fault of this issue, here's a RequiredModuleUninstallValidator on it's own, and included in the test.
Ideally the required_modules patch will contain all of the failures of the above test fail (minus the single forum module error).
Comment #16
tim.plunkettLet's increase the helpfulness of that message.
Comment #20
catchMAINTENANCE_MODE means two or three different things:
1. That the site is 'offline' - everything should work fine in that case, and you might be uninstalling modules.
2. That you're on update.php - now that this is a route, it's the same as #1.
3. That you've hit an error condition and are in the maintenance theme - at which case we don't know how much of Drupal is working or not.
I think the check was only for the case if you hit #3, and something triggered a system info rebuild in that state. Trying to uninstall a module in the state should be impossible, so just removing the checks seems fine.
Comment #21
cilefen CreditAttribution: cilefen commentedReroll
Comment #23
tim.plunkettThanks for the reroll @cilefen! The code we're moving around has changed, adjusting for that.
Comment #25
tim.plunkettAnd adjusting the test as well.
Comment #27
cilefen CreditAttribution: cilefen commentedThis is one of the failing tests. It looks as though fields are not deleted, or not deleted before the uninstall validators execute.
Comment #28
cilefen CreditAttribution: cilefen commentedFixes ForumUninstallTest by removing the Forums vocabulary terms before module uninstall is attempted.
Comment #29
cilefen CreditAttribution: cilefen commentedReviewing myself: "form" should be "forum".
Comment #30
cilefen CreditAttribution: cilefen commentedUpends CommentUninstallTest::testCommentUninstallWithField() to expect an exception on module uninstall if a field exists.
Comment #33
tim.plunkettOn the next reroll/fix, please put `catch` on its own line
Comment #34
cilefen CreditAttribution: cilefen commentedIn #30, CommentUninstallTest::testCommentUninstallWithoutField() is causing an exception from the validator because a field exists, but the point of the test is to have removed the field before attempting uninstall.
Comment #35
gobinathmUpdated Patch (moving catch to new line)
Comment #36
gobinathmComment #37
cilefen CreditAttribution: cilefen commented@gobinathm Thank you for the interdiff. There is just one issue: there is now a trailing space after the
try{}
block that must be removed. On a personal note, I am so inept at trailing spaces that I have my IDE remove them when saving.After that, I wonder if you have any thoughts on the paradox I mentioned in #34?
Comment #39
bircherre-roll
Comment #40
bircheralso re-test
Comment #42
bircherfixing tests
Comment #44
bircherfixed the string.
Comment #45
tim.plunkettI would consider this RTBC, but I wrote a big chunk of this. The changes since my patch in #25 are good.
One tweak. In #2454287: Make a couple of services lazy, the existing ModuleUninstallValidatorInterface classes were marked lazy. Doing the same for the new ones (and fixing the newline problem at the same time)
Comment #46
Wim LeersJust nitpicks!
Missing the "Constructs new …" stuff that we have for other constructors.
s/are/is/
See earlier.
80 cols.
hasBookOutlines()
?
80 cols.
s/editor/Editor/
Should be
\Drupal\Core\Config\Entity\ConfigEntityStorageInterface
.Again here.
s/remains/remain/
Missing trailing period.
The reasons listed for book have trailing periods, these should too.
Prevents uninstallation of modules providing used filter plugins.
(This makes it fit on a single line.)
\Drupal\Core\Config\Entity\ConfigEntityStorageInterface
Trailing period in reason, just like elsewhere.
\Drupal\Core\Config\Entity\ConfigEntityStorageInterface
s/Forum first/Forum, first/
Comment #47
bircherComment #49
bircherwhen adding commas and renaming the function the unit test needs to be updated too of course...
Comment #51
birchercatching more commas...
Comment #52
catchComment #53
birchersimple re-roll since the old patch doesn't apply any more.
Comment #55
bircherReplacing
String::format()
withSafeMarkup::format()
makes the unit tests pass.Comment #56
dawehnerShould we provide a link to admin/content?
Should we explain which field types are in use?
Comment #57
tim.plunkettThese are existing strings, I don't think this issue is the place to improve them.
Comment #58
dawehnerI'm sorry I wasn't aware of that.
Here a general question: Is it just me or do the various unit tests just ensure that the code written like that, is written like that? For me, it feels wrong to have not integration level testing.
Comment #59
alexpott@dawehner there is integration level testing - for example ForumUninstallTest. I've reintroduced BookUninstallTest so we don't lose integration testing there either.
Also I discovered a problem with escaped html on the modules uninstall page. Considering all the validation reasons are run through
t()
i think it is safe to do:when we add them together for display.
Also in the same part of the code because we're adding them together with a
;
I think we need to remove all the final punctuation from the validation messages.I've also re-instated the test in StandardTest to ensure editor can be uninstalled and re-installed. Plus I've fixed ConfigImportAllTest so it can disable all modules apart from Standard, System and User.
Comment #61
catchOnly issue with this is it could look odd for RTL languages or any that normally wouldn't use a semicolon. I can only really think of an unordered list or similar as an alternative though. Not sure how that'd end up looking.
Didn't do an in depth review of the patch yet, but overall it looks good to me.
Comment #62
alexpottFixed test and made the exception message use the
;
reason separator to be consistent with the uninstall modules page.Doing this should also reduce memory usage on #2494987: [meta-6] Reduce cold cache memory requirements because rebuilding the system module data will be cheaper.
Comment #63
alexpottI think it looks great with an unordered list. Much much easier to read...
versus
Comment #65
alexpottFixing the test failure.
Comment #66
bircherIt looks very nice with the unordered list.
Comment #67
catchSo I think this is pretty much ready, but if we're using the unordered list, do we still need to make this change? I can see we might still implode them in the exception message but that's not end-user facing. On the other hand it doesn't do any harm really.
Apart from that question and one more look over, I think this is RTBC.
Comment #68
catchDiscussed with alexpott in irc: he thinks it's useful to keep this restriction for the exception messages or if we used them in other contexts.
That's fair enough, so RTBC from me.
Comment #69
dawehnerI really like how this issue moves an implicit "API" to an explicit API
Comment #71
catch#69 yes I really like that as well. It's nice to get from the 7.x stop-gap we added for field module (marking itself required) to this.
Committed/pushed to 8.0.x, thanks!
Comment #72
alexpottA small followup is required I think #2510310: Improve hook_system_info_alter documentation to mention ModuleUninstallValidatorInterface