Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
.
Comment | File | Size | Author |
---|---|---|---|
#21 | interdiff_18-21.txt | 1.02 KB | Neslee Canil Pinto |
#21 | 2969190-21.patch | 1.2 KB | Neslee Canil Pinto |
#18 | whitespace-error.jpg | 232.93 KB | ranjith_kumar_k_u |
#18 | 2969190-18.patch | 1.22 KB | ranjith_kumar_k_u |
#12 | change_moduleInstallerInterface_uninstall_method_doc_comment-2969190-12.patch | 1.19 KB | manish.upadhyay |
Comments
Comment #2
mtodor CreditAttribution: mtodor at Thunder commentedHere is a proposal for doc comment in the interface.
Comment #3
joachim CreditAttribution: joachim as a volunteer commentedGood catch!
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:
Comment #5
manish.upadhyay CreditAttribution: manish.upadhyay as a volunteer and at Publicis Sapient commentedHi,
I have added a new patch for this please apply this.
Thanks,
Comment #6
manish.upadhyay CreditAttribution: manish.upadhyay as a volunteer and at Publicis Sapient commentedComment #7
joachim CreditAttribution: joachim as a volunteer commentedThanks for the updated patch! This is getting close I think!
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.
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.
Comment #8
manish.upadhyay CreditAttribution: manish.upadhyay as a volunteer and at Publicis Sapient commentedPlease find the updated patch.
Thanks,
Comment #9
Shamsher_Alam CreditAttribution: Shamsher_Alam as a volunteer and at Publicis Sapient commentedHi,
I have applied this patch and tested for review. it's working fine and also code as per standard with well written.
Thanks
Comment #10
Shamsher_Alam CreditAttribution: Shamsher_Alam as a volunteer and at Publicis Sapient commentedComment #11
joachim CreditAttribution: joachim as a volunteer commentedNearly there, but not quite!
This should mention the constant, rather than repeat the hardcoded value.
This line needs to be wrapped.
Comment #12
manish.upadhyay CreditAttribution: manish.upadhyay as a volunteer and at Publicis Sapient commentedHi,
Please find the updated patch.
Thanks,
Comment #18
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedFixed one whitespace error in the last patch
Comment #19
joachim CreditAttribution: joachim as a volunteer commentedLGTM.
Comment #20
larowlanThe english is off here
suggest
%s/length exceeds than/is longer than/
Same here, suggest
"Thrown when validation prevented the module from being uninstalled"
I think this is a task more than a bug
Comment #21
Neslee Canil PintoUpdated as per #20
Comment #22
joachim CreditAttribution: joachim as a volunteer commentedThanks for the reroll.
Missing documentation is a documentation bug, isn't it?
Comment #23
larowlanUpdating issue credits
Comment #25
larowlanCommitted b9f132b and pushed to 9.3.x. Thanks!
Backported to 9.2.x