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.

CommentFileSizeAuthor
#61 interdiff-58-61.txt1.82 KBsnehi
#61 improve_the-2392533-61.patch1.81 KBsnehi
#58 interdiff-2392533-52-58.txt1.83 KBsnehi
#58 improve_the-2392533-58.patch1.82 KBsnehi
#52 improve_the-2392533-52.patch1.82 KBpriya.chat
#51 improve_the-2392533-51.patch1.81 KBcilefen
#46 grammar_module_uninstall.png10.87 KBfil00dl
#45 module_uninstall.png8.77 KBfil00dl
#42 Screen Shot 2015-07-31 at 9.15.18 AM.png54.11 KBcilefen
#42 Screen Shot 2015-07-31 at 9.16.36 AM.png45.86 KBcilefen
#42 interdiff.txt2.1 KBcilefen
#42 improve_the-2392533-42.patch2.85 KBcilefen
#41 improve_the-2392533-41.patch4.19 KBcilefen
#38 Screen Shot 2015-07-31 at 7.22.58 AM.png221.3 KBcilefen
#38 improve_the-2392533-38.patch6.53 KBcilefen
#30 interdiff-2392533-21-30.txt0 bytesdeepakaryan1988
#30 2392533-module-message-30.patch4.14 KBdeepakaryan1988
#25 2392533-module-message-22.patch3.13 KBvenkatadapa
#21 interdiff-2392533-18-21.txt4.74 KBOleksiy
#21 2392533-module-message-21.patch4.07 KBOleksiy
#18 2392533-module-message-18.patch4.93 KBSerShevchyk
#16 2392533-16.patch2.61 KBrpayanm
#12 2392533-11.patch2.81 KBbircher
#9 2392533-9.patch2.84 KBbircher
#6 2392533-6.patch1.9 KBbircher
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Issue summary: View changes
catch’s picture

Issue summary: View changes
catch’s picture

Issue summary: View changes
catch’s picture

Issue summary: View changes
bircher’s picture

Assigned: Unassigned » bircher

will fix it on the plane

bircher’s picture

Status: Active » Needs review
FileSize
1.9 KB

1) 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.

Status: Needs review » Needs work

The last submitted patch, 6: 2392533-6.patch, failed testing.

fago’s picture

Shouldn't we do a message per module here? Otherwise it'll be a bit of detective work to figure out which module is responsible for what - not always obvious.

Usually, 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.

bircher’s picture

Status: Needs work » Needs review
FileSize
2.84 KB

of 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.

YesCT’s picture

added some tasks to summary, also needs a title to capture the actual problem or change.

bircher’s picture

Title: Follow-up for #2278017 » Improve the ModuleUninstallValidatorException (Follow-up for #2278017)
Issue summary: View changes

rewrite the issue summary

bircher’s picture

FileSize
2.81 KB

attach the re-rolled patch

Status: Needs review » Needs work

The last submitted patch, 12: 2392533-11.patch, failed testing.

bircher’s picture

Assigned: bircher » Unassigned
Issue tags: +Novice
joshi.rohit100’s picture

Issue tags: +Needs reroll

Needs re-roll.

rpayanm’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.61 KB

Status: Needs review » Needs work

The last submitted patch, 16: 2392533-16.patch, failed testing.

SerShevchyk’s picture

Assigned: Unassigned » SerShevchyk
Status: Needs work » Needs review
FileSize
4.93 KB

Fix for 2392533-11.patch

Status: Needs review » Needs work

The last submitted patch, 18: 2392533-module-message-18.patch, failed testing.

andypost’s picture

  1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -325,12 +325,12 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
    -    // Use the validators and throw an exception with the reasons.
    +   // Use the validators and throw an exception with the reasons.
    

    unneeded indent

  2. +++ b/core/lib/Drupal/Core/Field/FieldModuleUninstallValidator.php
    @@ -53,7 +53,7 @@ public function validate($module_name) {
    -                $reasons[] = $this->t('There is data for the field @field-name on entity type @entity_type', array(
    +                $reasons[] = $this->t('@field-name. There is data for the field @field-name on entity type @entity_type', array(
    
    +++ b/core/modules/system/src/Tests/Field/FieldModuleUninstallValidatorTest.php
    @@ -83,7 +83,7 @@ public function testUninstallingModule() {
    -      $this->assertEqual($e->getMessage(), 'The following reasons prevents the modules from being uninstalled: There is data for the field extra_base_field on entity type Test entity');
    +      $this->assertEqual($e->getMessage(), 'The following reasons prevent the modules from being uninstalled: entity_test_extra: extra_base_field. There is data for the field extra_base_field on entity type Test entity');
    

    no reason for that - *extra_base-field* is displayed twice and confusing

  3. +++ b/core/modules/system/system.admin.inc
    @@ -316,8 +316,8 @@ function theme_system_modules_uninstall($variables) {
           $disabled_header = \Drupal::translation()->formatPlural(count($form['modules'][$module]['#validation_reasons']),
    -        'The following reason prevents @module from being uninstalled:',
    -        'The following reasons prevents @module from being uninstalled:',
    +        'The following reason prevent @module from being uninstalled:',
    +        'The following reasons prevent @module from being uninstalled: @reasons',
             array('@module' => $form['modules'][$module]['#module_name']));
    

    no @reasons is passed for formatting function

Oleksiy’s picture

Status: Needs work » Needs review
FileSize
4.07 KB
4.74 KB
andypost’s picture

Assigned: SerShevchyk » Unassigned
Status: Needs review » Reviewed & tested by the community

Finally looks good

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 2392533-module-message-21.patch, failed testing.

andypost’s picture

Issue tags: +Needs reroll
venkatadapa’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll +#DHCSprint
FileSize
3.13 KB

This patch is again having some problem, I will upload the updated patch again. Don't use this for now

Status: Needs review » Needs work

The last submitted patch, 25: 2392533-module-message-22.patch, failed testing.

dawehner’s picture

+1

andypost’s picture

Issue tags: +Needs reroll

incomplete re-roll, still needs new one

deepakaryan1988’s picture

Assigned: Unassigned » deepakaryan1988
deepakaryan1988’s picture

Re-rolling it as per #21

deepakaryan1988’s picture

Status: Needs work » Needs review
bircher’s picture

+++ b/core/modules/system/system.admin.inc
@@ -316,9 +316,9 @@ function theme_system_modules_uninstall($variables) {
+        'The following reason prevent @module from being uninstalled: @reasons',

The 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.

andypost’s picture

@bircher so what is proper grammar?
"reason prevents" - single reason
"reasons prevent" - multiple reasons

bircher’s picture

I 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).

cilefen’s picture

bircher’s picture

@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.

cilefen’s picture

Also 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.

cilefen’s picture

I 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.

Status: Needs review » Needs work

The last submitted patch, 38: improve_the-2392533-38.patch, failed testing.

cilefen’s picture

I 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.

cilefen’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.19 KB
cilefen’s picture

Changing 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):

cilefen’s picture

Let 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.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -316,10 +316,10 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
-      throw new ModuleUninstallValidatorException('The following reasons prevents the modules from being uninstalled: ' . implode('; ', $reason_message));
+      throw new ModuleUninstallValidatorException('The following reasons prevent the modules from being uninstalled: ' . implode('; ', $reason_message));

I'm curious whether we should use the human readable versions of the module?

fil00dl’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
8.77 KB

The grammar issue is visible on this screenshots

fil00dl’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
FileSize
10.87 KB

The grammar issue are visible on this screenshots

priya.chat’s picture

Hello,
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.

cilefen’s picture

@priya.chat When that happens, you tag the issue "Needs reroll", and if you like, you can reroll it.

cilefen’s picture

Issue tags: +Needs reroll
cilefen’s picture

This was semi-fixed at the theme layer in #2151113: Convert theme_system_modules_uninstall() to Twig. But, the exception message still needs fixing.

cilefen’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.81 KB

I 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.

priya.chat’s picture

Thanks @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" .

deepakaryan1988’s picture

joshi.rohit100’s picture

@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.

deepakaryan1988’s picture

@joshi.rohit100 +1

cilefen’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -344,7 +344,7 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
-      throw new ModuleUninstallValidatorException('The following reasons prevents the modules from being uninstalled: ' . implode('; ', $reason_message));
+      throw new ModuleUninstallValidatorException('The following reasons that prevents the modules from being uninstalled: ' . implode('; ', $reason_message));

Thank you for your continued interest, however, "The following reasons that prevents" is not English.

cilefen’s picture

"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:

              {%- trans -%}
                The following reason prevents {{ module.module_name }} from being uninstalled:
              {%- plural module.reasons_count -%}
                The following reasons prevent {{ module.module_name }} from being uninstalled:
              {%- endtrans %}

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.

snehi’s picture

Status: Needs work » Needs review
FileSize
1.82 KB
1.83 KB

Adding conclusion from all the patches.
Please review attached patch.

cilefen’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/src/Tests/Field/FieldModuleUninstallValidatorTest.php
@@ -83,7 +83,7 @@ public function testUninstallingModule() {
-      $this->assertEqual($e->getMessage(), 'The following reasons prevents the modules from being uninstalled: There is data for the field extra_base_field on entity type Test entity');
+      $this->assertEqual($e->getMessage(), 'The following reasons that prevent the modules from being uninstalled: There is data for the field extra_base_field on entity type Test entity');

Repeating what I wrote in #56, "The following reasons that prevent ..." does not make sense.

cilefen’s picture

It should be "The following reasons prevent the modules from being uninstalled:".

snehi’s picture

Status: Needs work » Needs review
FileSize
1.81 KB
1.82 KB

Done as suggested by cilefen.

cilefen’s picture

Status: Needs review » Reviewed & tested by the community

This is good to go. As I wrote in #57, I think we don't need to handle singular vs. plural for the exception.

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.

The last submitted patch, 52: improve_the-2392533-52.patch, failed testing.

xjm’s picture

Title: Improve the ModuleUninstallValidatorException (Follow-up for #2278017) » Grammatical error in ModuleUninstallValidatorException message

Agreed that the patch in #61 is the correct fix.

  • catch committed cf27624 on 8.1.x
    Issue #2392533 by cilefen, bircher, snehi, deepakaryan1988, Oleksiy,...

  • catch committed 2de82cf on
    Issue #2392533 by cilefen, bircher, snehi, deepakaryan1988, Oleksiy,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x and cherry-picked to 8.0.x, thanks!

xjm’s picture

Issue summary: View changes

(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.)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.