Problem/Motivation

Currenlty doc comment for interface function ModuleInstallerInterface::uninstall() provides information that bool value is returned, but actual implementation can also throw ModuleUninstallValidatorException. That should be documented in interface too.

Same as it's documented for ModuleInstallerInterface::install() that it can throw MissingDependencyException

And I guess everyone would prefer to not read the code of the function they want to execute. ;)

Proposed resolution

Adjust comment in the interface.

Way to test that method throws exception

Copy paste following code into .../docroot/test.php:

<?php

/**
 * @file
 * Make drupal coder happy! :)
 */

/* @var \Drupal\Core\Extension\ModuleInstallerInterface $moduleInstaller */
$moduleInstaller = \Drupal::service('module_installer');

$moduleInstaller->uninstall(['node']);

And run drush scr test.php.

CommentFileSizeAuthor
#21 interdiff_18-21.txt1.02 KBNeslee Canil Pinto
#21 2969190-21.patch1.2 KBNeslee Canil Pinto
#18 whitespace-error.jpg232.93 KBranjith_kumar_k_u
#18 2969190-18.patch1.22 KBranjith_kumar_k_u
#12 change_moduleInstallerInterface_uninstall_method_doc_comment-2969190-12.patch1.19 KBmanish.upadhyay
#8 change_moduleInstallerInterface_uninstall_method_doc_comment-2969190-8.patch1.14 KBmanish.upadhyay
#5 change_moduleInstallerInterface_uninstall_method_doc_comment-2969190-5.patch1.13 KBmanish.upadhyay
#2 2969190_2.patch720 bytesmtodor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mtodor created an issue. See original summary.

mtodor’s picture

mtodor CreditAttribution: mtodor at Thunder commented
Status: Active » Needs review
FileSize
2969190_2.patch720 bytes

Here is a proposal for doc comment in the interface.

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented
Category: Task » Bug report
Status: Needs review » Needs work

Good catch!

+++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
@@ -56,6 +56,9 @@ public function install(array $module_list, $enable_dependencies = TRUE);
+   *   Thrown when uninstall validation fails.

This should be a complete sentence - see https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...

Also, it looks like install() can also throw:

          throw new ExtensionNameLengthException("Module name '$module' is over the maximum allowed length of " . DRUPAL_EXTENSION_NAME_MAX_LENGTH . ' characters');

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

manish.upadhyay’s picture

manish.upadhyay CreditAttribution: manish.upadhyay as a volunteer and at Publicis Sapient commented
FileSize
change_moduleInstallerInterface_uninstall_method_doc_comment-2969190-5.patch1.13 KB

Hi,

I have added a new patch for this please apply this.

Thanks,

manish.upadhyay’s picture

manish.upadhyay CreditAttribution: manish.upadhyay as a volunteer and at Publicis Sapient commented
Status: Needs work » Needs review
joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented
Status: Needs review » Needs work

Thanks for the updated patch! This is getting close I think!

  1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
@@ -37,6 +37,9 @@
+   *   Thrown when the extension's name length exceeds the allowed maximum.

    Could we give the maximum here, as a reference to DRUPAL_EXTENSION_NAME_MAX_LENGTH. It looks like it won't form a link -- see #2988368: global and class constants are not linked to -- but it would still be useful. Sorry, should have thought of that in my earlier review. 

  2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
@@ -56,6 +59,9 @@ public function install(array $module_list, $enable_dependencies = TRUE);
+   *   Thrown when uninstalling a module that did not validate.

    This seems vague, but the exception class itself doesn't say any more.

    So I'd say that vagueness should be fixed on the exception class rather than here.

manish.upadhyay’s picture

manish.upadhyay CreditAttribution: manish.upadhyay as a volunteer and at Publicis Sapient commented
Status: Needs work » Needs review
FileSize
change_moduleInstallerInterface_uninstall_method_doc_comment-2969190-8.patch1.14 KB

Please find the updated patch.

Thanks,

Shamsher_Alam’s picture

Shamsher_Alam CreditAttribution: Shamsher_Alam as a volunteer and at Publicis Sapient commented
Status: Needs review » Reviewed & tested by the community

Hi,

I have applied this patch and tested for review. it's working fine and also code as per standard with well written.

Thanks

Shamsher_Alam’s picture

Shamsher_Alam CreditAttribution: Shamsher_Alam as a volunteer and at Publicis Sapient commented
joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented
Status: Reviewed & tested by the community » Needs work

Nearly there, but not quite!

  1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
@@ -37,6 +37,9 @@
+   *   Thrown when the extension's name length exceeds than 50 character.

    This should mention the constant, rather than repeat the hardcoded value. 

  2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
@@ -56,6 +59,9 @@ public function install(array $module_list, $enable_dependencies = TRUE);
+   *   Thrown when uninstalling a module that did not validate due to some reasons.

    This line needs to be wrapped.

manish.upadhyay’s picture

manish.upadhyay CreditAttribution: manish.upadhyay as a volunteer and at Publicis Sapient commented
Status: Needs work » Needs review
FileSize
change_moduleInstallerInterface_uninstall_method_doc_comment-2969190-12.patch1.19 KB

Hi,

Please find the updated patch.

Thanks,

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

ranjith_kumar_k_u’s picture

ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commented
FileSize
2969190-18.patch1.22 KB
whitespace-error.jpg232.93 KB

Fixed one whitespace error in the last patch

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented
Status: Needs review » Reviewed & tested by the community

LGTM.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Category: Bug report » Task
Status: Reviewed & tested by the community » Needs work
Issue tags: +Bug Smash Initiative 
  1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
@@ -37,6 +37,10 @@ interface ModuleInstallerInterface {
+   *   DRUPAL_EXTENSION_NAME_MAX_LENGTH.

    The english is off here

    suggest

    %s/length exceeds than/is longer than/ 

  2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
@@ -56,6 +60,10 @@ public function install(array $module_list, $enable_dependencies = TRUE);
+   *   Thrown when uninstalling a module that did not validate due to certain
+   *   reasons.

    Same here, suggest

    "Thrown when validation prevented the module from being uninstalled"

I think this is a task more than a bug

Neslee Canil Pinto’s picture

Neslee Canil Pinto
he/him
Primary language English
CreditAttribution: Neslee Canil Pinto at Ramsalt Lab commented
Status: Needs work » Needs review
FileSize
2969190-21.patch1.2 KB
interdiff_18-21.txt1.02 KB

Updated as per #20

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented
Category: Task » Bug report
Status: Needs review » Reviewed & tested by the community

Thanks for the reroll.

Missing documentation is a documentation bug, isn't it?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Updating issue credits

  • larowlan committed 29c6e25 on 9.2.x 
    Issue #2969190 by manish.upadhyay, ranjith_kumar_k_u, Neslee Canil Pinto...
  • larowlan committed b9f132b on 9.3.x
    Issue #2969190 by manish.upadhyay, ranjith_kumar_k_u, Neslee Canil Pinto...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed b9f132b and pushed to 9.3.x. Thanks!

Backported to 9.2.x

Status: Fixed » Closed (fixed)

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