Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
When a module cannot be installed but an attempt is made to uninstall it via an API call, an exception is thrown that says: "The following reasons prevents the modules from being uninstalled..." This contains a grammatical error.
Proposed resolution
Remove the grammatical error.
Comment | File | Size | Author |
---|---|---|---|
#61 | improve_the-2392533-61.patch | 1.81 KB | snehi |
Comments
Comment #1
catchComment #2
catchComment #3
catchComment #4
catchComment #5
bircherwill fix it on the plane
Comment #6
bircher1) I didn't find a generic way. There is the list_builder handler, but then that gets mapped to a route or not in other places. But maybe someone else has more insight.
2) yes we can
3) thanks
4) fixed.
Comment #8
fagoUsually, the exception won't be visible to users as validation is run before - it only appears if someone tries to uninstall without making sure validation passes first. Drush can/should do so as well and it will get individual reasons. Exceptions are not good for fetch validation fail messages as you never can fetch multiple but just end up with the first fail.
Comment #9
bircherof course if the reson changes we also have to update the text expecting the reason.
@fago, yes that is true, but if you try to uninstall several modules at once the exception returns all reasons anyway, this patch adds the module name in the right places to the long string.
Comment #10
YesCT CreditAttribution: YesCT commentedadded some tasks to summary, also needs a title to capture the actual problem or change.
Comment #11
bircherrewrite the issue summary
Comment #12
bircherattach the re-rolled patch
Comment #14
bircherAn issue to watch that would likely influence this again: #2392293: Refactor hook_system_info_alter implementations to use ModuleUninstallValidatorInterface
Comment #15
joshi.rohit100Needs re-roll.
Comment #16
rpayanmComment #18
SerShevchykFix for 2392533-11.patch
Comment #20
andypostunneeded indent
no reason for that - *extra_base-field* is displayed twice and confusing
no @reasons is passed for formatting function
Comment #21
OleksiyComment #22
andypostFinally looks good
Comment #24
andypostComment #25
venkatadapa CreditAttribution: venkatadapa as a volunteer commentedThis patch is again having some problem, I will upload the updated patch again. Don't use this for now
Comment #27
dawehner+1
Comment #28
andypostincomplete re-roll, still needs new one
Comment #29
deepakaryan1988Comment #30
deepakaryan1988Re-rolling it as per #21
Comment #31
deepakaryan1988Comment #32
bircherThe reason prevents (with the s).
I haven't applied the patch and tested it, but it looks good from a first glance at the patch.
But since I worked on it a while ago I wouldn't rtbc it anyway.
Thanks for continuing to work on it.
Comment #33
andypost@bircher so what is proper grammar?
"reason prevents" - single reason
"reasons prevent" - multiple reasons
Comment #34
bircherI am not a native English speaker but yes, that would be the proper grammar according to me.
You use the third person singular (he/she) for a single reason (she prevents something) and the third person plural (they) for multiple reasons (they prevent something).
Comment #35
cilefen CreditAttribution: cilefen commentedYes, #33 is correct. Can someone see if this one is a duplicate? #2489472: Field-based module dependency uninstall message is unhelpful and not grammatically correct
Comment #36
bircher@cilefen yes I would say its a duplicate, at least in my understanding. The issue you referenced is also about telling which field is in use and I think that is the same goal as here, which is to make this message more useful.
I closed the other issue, feel free to merge your patch from there here or re-open it.
Comment #37
cilefen CreditAttribution: cilefen commentedAlso Re #33. As a native speaker, it is ok to say "The following reasons prevent .... :" even if there is only one reason in the list. The idea is that we understand we are looking at a list.
Comment #38
cilefen CreditAttribution: cilefen commentedI rerolled and merged in my changes from the other issue, which is an improvement to the field message. I do not understand in the prior patches why we list the reasons after a colon in addition to the unordered list. I also do not understand the role of the exception, but I need to read more of this code to understand why it exists.
This will fail some of the new tests.
Comment #40
cilefen CreditAttribution: cilefen commentedI have decided #2489472: Field-based module dependency uninstall message is unhelpful and not grammatically correct is not a duplicate, because it pertains specifically to the field message. Let's reroll #30 and continue work on that.
Comment #41
cilefen CreditAttribution: cilefen commentedComment #42
cilefen CreditAttribution: cilefen commentedChanging the uninstall UI message is out-of-scope for this issue and actually does not make sense. With #30, it looks like this:
That is strange. It should look like this (which is this patch):
Comment #43
cilefen CreditAttribution: cilefen commentedLet me be clearer. Changing the uninstall UI message to include the reasons in the same line does not make sense. They are already in an unordered list below it.
Comment #44
dawehnerI'm curious whether we should use the human readable versions of the module?
Comment #45
fil00dl CreditAttribution: fil00dl at Skilld commentedThe grammar issue is visible on this screenshots
Comment #46
fil00dl CreditAttribution: fil00dl at Skilld commentedThe grammar issue are visible on this screenshots
Comment #47
priya.chat CreditAttribution: priya.chat at Publicis Sapient for Publicis Sapient commentedHello,
I am not able to apply the patch given in #42. as the function theme_system_modules_uninstall($variables){} doesn't exists in /core/modules/system/system.admin.inc , drupal 8 latest version.
Comment #48
cilefen CreditAttribution: cilefen commented@priya.chat When that happens, you tag the issue "Needs reroll", and if you like, you can reroll it.
Comment #49
cilefen CreditAttribution: cilefen commentedComment #50
cilefen CreditAttribution: cilefen commentedThis was semi-fixed at the theme layer in #2151113: Convert theme_system_modules_uninstall() to Twig. But, the exception message still needs fixing.
Comment #51
cilefen CreditAttribution: cilefen commentedI do not think that UI tests can trigger this exception but unit tests can. There is one unit test that verifies the exception text so we have coverage.
Comment #52
priya.chat CreditAttribution: priya.chat at Publicis Sapient for Publicis Sapient commentedThanks @cilefen, as I follow the patch I thing the grammar issue (mention in #46) that we face from the beginning can be solve from my patch.
I think for single reason the line should be "reason that prevents" and,
for multiple reason it should be "reasons that prevents" .
Comment #53
deepakaryan1988Comment #54
joshi.rohit100@priya.chat - Please try to upload the interdiff as well (though its a small patch) so that it be easy to review what changes you have made.
Comment #55
deepakaryan1988@joshi.rohit100 +1
Comment #56
cilefen CreditAttribution: cilefen commentedThank you for your continued interest, however, "The following reasons that prevents" is not English.
Comment #57
cilefen CreditAttribution: cilefen commented"The following reasons prevent the modules from being uninstalled:" is proper English. You can reference the fixed system-modules-uninstall.html.twig if you do not believe me.
That template file contains both the plural and singular cases:
In this simple exception that will not be seen in the UI, the plural case is enough. Therefore, the patch I posted in #51, which reads "The following reasons prevent the modules from being uninstalled", is sufficient.
Comment #58
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedAdding conclusion from all the patches.
Please review attached patch.
Comment #59
cilefen CreditAttribution: cilefen commentedRepeating what I wrote in #56, "The following reasons that prevent ..." does not make sense.
Comment #60
cilefen CreditAttribution: cilefen commentedIt should be "The following reasons prevent the modules from being uninstalled:".
Comment #61
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedDone as suggested by cilefen.
Comment #62
cilefen CreditAttribution: cilefen commentedThis is good to go. As I wrote in #57, I think we don't need to handle singular vs. plural for the exception.
Comment #64
andypostComment #65
xjmAgreed that the patch in #61 is the correct fix.
Comment #68
catchCommitted/pushed to 8.1.x and cherry-picked to 8.0.x, thanks!
Comment #69
xjm(Removed the screenshots from the summary because they were misleading. This was a change only to the exception message, not to the message in the user interface.)