Prompt the user to delete the module once it was successfully uninstalled.

Potential challenges:

  • Finding the location of the module correctly and reliably.

On deleted_modules.drush.inc#L115

    // @TODO remove the downloaded project from the filesystem?
    drush_log("Module {$name} was properly uninstalled and should be now safe to remove from the file system.", 'success');
CommentFileSizeAuthor
#6 interdiff.txt1.14 KBtatarbj
#6 3006935-6.patch1.99 KBtatarbj
#3 3006935-3.patch1.89 KBtatarbj

Comments

Manuel Garcia created an issue. See original summary.

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Assigned: Unassigned » tatarbj
Status: Active » Needs work
tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Status: Needs work » Needs review
StatusFileSize
new 3006935-3.patch1.89 KB

It was quite complex to implement, but here we come!
The problem was with the original implementation of how the module collects the projects that are not correctly removed in _get_deleted_modules() as it overlooks the Drupal way that sets -1 in the schema_version when uninstallation happened on an extension (module/theme) - https://api.drupal.org/api/drupal/includes%21install.inc/constant/SCHEMA....
So I extended that query to check that parameter too, as after removing the files with drush_delete_dir() I was not able to remove the item from the database. Basically, the uninstall process only marks the module with this schema_version that will tell the garbage collector during a cache-clear process, it is safe to be totally removed.
When I tried to implement a classic cache-clear at the end of the whole dmc command, it has never done the expected behavior, most probably as we are not in the same bootstrap level or drush commands are simply atomic (?).
Anyway, when you execute the whole command now, answering the very last question a 'y', it will remove the folder with its content from the filesystem but as expected (I would say correct Drupal behavior ¯\_(ツ)_/¯ ) the module will remain in db with its -1 as schema_version - that will be removed after a simple drush cc all.
Qed :)
Bests,
Balazs.

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Assigned: tatarbj » Unassigned
manuel garcia’s picture

manuel garcia commented
Status: Needs review » Needs work

Thanks @tatarbj for tackling this, awesome work so far!

Here is my review:

  1. +++ b/deleted_modules.drush.inc
@@ -112,8 +112,13 @@ function drush_deleted_modules_cleanup() {
+    drush_print(dt('Delete modules can remove the previously downloaded files from the site as it belongs to a properly uninstalled module!'));

    I don't think this is necessary :) 

  2. +++ b/deleted_modules.drush.inc
@@ -112,8 +112,13 @@ function drush_deleted_modules_cleanup() {
+    $path = drush_extension_get_path($module);    ¶

    Nit: Lets get the path only if the user confirms it wants to delete it. Also, trailing whitespaces :)

  3. +++ b/deleted_modules.drush.inc
@@ -112,8 +112,13 @@ function drush_deleted_modules_cleanup() {
+      drush_log("Module {$name} and its files from {$path} was properly removed from the file system.", 'success');

    Lets check if the deletion was successful and log accordingly:

    if (drush_delete_dir($path, TRUE)) {
  drush_log("Module {$name} and its files from {$path} was properly removed from the file system.", 'success');
}
else {
  return drush_set_error("Unable to remove {$path} from the filesystem.");
}

  4. +++ b/deleted_modules.drush.inc
@@ -125,8 +130,8 @@ function drush_deleted_modules_cleanup() {
+  // Get all modules system knows about and not marked uninstalled already.
+  $result = db_query("SELECT * FROM {system} WHERE type = 'module' AND schema_version > -1 ORDER BY name ASC");

    Great find!

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Status: Needs work » Needs review
StatusFileSize
new 3006935-6.patch1.99 KB
new interdiff.txt1.14 KB

Nice review, thanks for it @Manuel!

Let me answer your points:
1. correct, I've removed it from the new patch.
2. trailing whitespaces are removed, but to ask the confirmation and I believe it's important for the user, we should present the path, so it should be asked before the confirm. Then I'm keeping the line there (without the trailing spaces :))
3. correct, I've changed it accordingly to your suggestion - nice improvement!
4. it took a while to realize why things were not working as expected :P I know it's quite an overplanning and not that much needed on d7 contribs, but I would say after the stable release, we should think about some test coverage for the module. Wouldn't you mind to create a feature request issue in the queue to remind us later for this with minor priority?

Bests,
Balazs.

manuel garcia’s picture

manuel garcia commented
Status: Needs review » Fixed

Re
#6.2 Ah you're right, I agree.
#6.4 I was thinking the same, opened #3020325: Automated tests for it.

+++ b/deleted_modules.drush.inc
@@ -112,8 +112,16 @@ function drush_deleted_modules_cleanup() {
+    drush_log("Module {$name} was properly uninstalled and should be now safe to remove from the file system.", 'success');    ¶

Rerolled and fixed trailing spaces on commit.

Status: Fixed » Closed (fixed)

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