Add types to @param's and add a @see.

CommentFileSizeAuthor
#8 interdiff.txt2.46 KBLiam Morland
#6 drupal-doc_module_disable-2645544-6-D7.patch2.47 KBLiam Morland
#2 drupal-doc_module_disable-2645544-2-D7.patch806 bytesLiam Morland
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Liam Morland created an issue. See original summary.

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland at University of Waterloo commented
Assigned: Liam Morland » Unassigned
Status: Active » Needs review
FileSize
drupal-doc_module_disable-2645544-2-D7.patch806 bytes
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon at Poplar ProductivityWare for Drupal Association commented
Status: Needs review » Needs work

Thanks! These look like reasonable changes to make.

One small thing to fix and one suggestion for improvement:

  1. +++ b/includes/module.inc
@@ -505,12 +505,14 @@ function module_enable($module_list, $enable_dependencies = TRUE) {
+ * @param array $module_list
  *   An array of module names.

    The type on this should be string[] not the generic array. 

  2. +++ b/includes/module.inc
@@ -505,12 +505,14 @@ function module_enable($module_list, $enable_dependencies = TRUE) {
+ * @see drupal_uninstall_modules()

    Maybe also add a @see for module_enable()?

Shreya Shetty’s picture

Shreya Shetty CreditAttribution: Shreya Shetty at Trigyn Technologies Ltd commented
Assigned: Unassigned » Shreya Shetty
Shreya Shetty’s picture

Shreya Shetty CreditAttribution: Shreya Shetty at Trigyn Technologies Ltd commented
Assigned: Shreya Shetty » Unassigned
Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland at University of Waterloo commented
Title: Improve documentation for module_disable() » Improve documentation for module_disable() and related functions
Status: Needs work » Needs review
FileSize
drupal-doc_module_disable-2645544-6-D7.patch2.47 KB

Thanks. Updated patch attached.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon at Poplar ProductivityWare for Drupal Association commented
Status: Needs review » Reviewed & tested by the community

When you attach a new patch on an existing review, you really really really should an interdiff file. Thanks!

Anyway, this patch is fine. Thanks!

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland at University of Waterloo commented
FileSize
interdiff.txt2.46 KB

Thanks. I wish d.o. took care of interdiffs automatically.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

However, I'm a little unsure about this:

- * @param array $module_list
+ * @param string[] $module_list

It's in the new coding standards at https://www.drupal.org/coding-standards/docs#types but Drupal 7 is literally using this nowhere else, I think. So I'm not sure if we should introduce it here when everything else just does "array".

But I committed this anyway, since it's not that big of a deal either way and certainly possible to go back later if necessary.

  • David_Rothstein committed 98229c4 on 7.x 
    Issue #2645544 by Liam Morland: Improve documentation for module_disable...

Status: Fixed » Closed (fixed)

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