Problem/Motivation

There's some apparent inconsistency in API function names for working with modules, which makes them difficult to guess:

  • module_enable() and...
  • module_disable() but...
  • drupal_uninstall_modules()

Are there others?

The first two functions live in module.inc, and the last one lives in install.inc, which probably accounts for the difference—and I can understand that, but it's still confusing, and I wonder if it couldn't be different. Also, I wonder if there would be a convenient way to group the functions somehow in the API docs. Maybe that's silly, but I expected to be able to see all the functions for manipulating module status together.

Proposed resolution

Make the names consistent. :)

Remaining tasks

  • Discuss and reach consensus.
  • Write a patch. (I'll write a patch if a change is agreed-upon.)

API changes

To come.

CommentFileSizeAuthor
#17 1587642-followup-17.patch326 bytesandypost
#10 drupal-drupal_uninstall_modules-1587642-10.patch12.23 KBtraviscarden
#6 drupal-drupal_uninstall_modules-1587642-6.patch6.47 KBtraviscarden
#8 drupal-drupal_uninstall_modules-1587642-8.patch12.06 KBtraviscarden

Comments

traviscarden’s picture

traviscarden commented
Title: Make enable, install, uninstall API function names consistent » Rename drupal_uninstall_modules() to module_uninstall()?

Renaming per chx's foreboding prophecy of doom-bringing flame wars over the original title. ;)

Note: This issue is not about function prefixing or namespacing in Drupal in general. It is about three specific, related functions being named alike and nothing more.

Ethos’s picture

Ethos commented
Status: Active » Reviewed & tested by the community

Just checking the ever exciting coding standards for you:

Does it fully comply with Drupal coding standards?
Clearly. In fact, it seems to bring these particular functions much closer to the consistency valued by drupal users. So not only does it comply, it actually improves :)

If the patch includes tests, are they both necessary and sufficient?
Not really applicable, but any test should be relatively simple to do, so a yes on this.

Does the patch actually solve the problem? Absolutely. in provides a clear, simple, and concise solution.

In conclusion, This should absolutely be implemented and should be supported by the community. Changed status to reviewed and tested.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Reviewed & tested by the community » Active

RTBC means there is a working patch. Setting back to active.
That said, this totally makes sense.

Also, I wonder if there would be a convenient way to group the functions somehow in the API docs.

In theory, the fact that they live in module.inc should provide such a grouping. De facto a lot of functions that should be in there aren't and vice-versa, so...

traviscarden’s picture

traviscarden commented
Issue tags: +API consistency

Adding tag.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: Rename drupal_uninstall_modules() to module_uninstall()? » Rename drupal_uninstall_modules() to module_uninstall()
Issue tags: +Novice

This just bit me today and was really, really strange. I'd say go for it!

Marking as "Novice" since this should be a pretty easy patch to roll.

traviscarden’s picture

traviscarden commented
StatusFileSize
new drupal-drupal_uninstall_modules-1587642-6.patch6.47 KB

Awesome. Here's a patch. Moving the function into module.inc broke tests, so I left it where it was. If folks think it would be worth moving it and can enlighten me as to what would need to be done to make it work, I'd happily re-roll the patch. Thanks!

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Active » Needs review

Looks good. Maybe you could upload a patch with the function moved, so we can see what tests break.
Maybe we can also just commit this as a first step, though. Hmm...

traviscarden’s picture

traviscarden commented
StatusFileSize
new drupal-drupal_uninstall_modules-1587642-8.patch12.06 KB

Sure @tstoeckler. Here's a patch moving the function from install.inc to module.inc.

Status: Needs review » Needs work

The last submitted patch, drupal-drupal_uninstall_modules-1587642-8.patch, failed testing.

traviscarden’s picture

traviscarden commented
Status: Needs work » Needs review
StatusFileSize
new drupal-drupal_uninstall_modules-1587642-10.patch12.23 KB

Oh. module.inc just needed the DatabaseStorage namespace. That's easy enough. :) Here's an updated patch.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community

This is RTBC. Awesome!
Also tagging DX.

traviscarden’s picture

traviscarden commented
Issue tags: +DX (Developer Experience)

Thanks, @tstoeckler.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: Rename drupal_uninstall_modules() to module_uninstall() » Change notice: Rename drupal_uninstall_modules() to module_uninstall()
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

Love it! :D

Committed and pushed to 8.x. Thanks!

We need a change notice for this.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Active » Needs review

http://drupal.org/node/1675008

Tor Arne Thune’s picture

Tor Arne Thune commented
Title: Change notice: Rename drupal_uninstall_modules() to module_uninstall() » Rename drupal_uninstall_modules() to module_uninstall()
Priority: Critical » Normal
Status: Needs review » Fixed

Looks good :)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Fixed » Needs work 
+++ b/core/includes/module.inc
@@ -6,6 +6,7 @@
+use Drupal\Core\Config\DatabaseStorage;

The use statement was added to module.inc but not removed from install.inc. :)

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new 1587642-followup-17.patch326 bytes

Seem this namespace is not used in install.inc, what the bot say...

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community

RTBC if it comes back green. Sorry for overlooking this.

dries’s picture

dries commented

Nice API clean-up. Let's wait and see if this turns green.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

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