Admin panel module uninstallation feature #1755
Conversation
There was a problem hiding this comment.
I left a handful of comments on the changes where I'd like to see changes.
Additionally, I think the UI would be better if the deactivate and delete buttons weren't separated by the settings icon. I'd likely move the settings icon to either be first or last.
We should also consider preventing people from deleting modules that come with FOSSBilling. The deactivate function will prevent core modules from being disabled or uninstalled, but there's no checks against modules that are included, but not considered core. In theory deleting them shouldn't cause any issues, but it may cause confusion when updating as they will return.
| /** | ||
| * Recursively remove a directory at a specific path. | ||
| */ | ||
| public function removeDirectory(string $dir): void |
There was a problem hiding this comment.
We actually already have a function to empty a directory:
https://github.com/FOSSBilling/FOSSBilling/blob/main/src/library/FOSSBilling/Tools.php#L93
$this->di['tools']->emptyFolder($dir);
I don't recall if it will delete a directory and it's contents or just the contents of the directory. However, if it only deletes the contents then all you need to do this:
$this->di['tools']->emptyFolder($dir);
rmdir($dir);(If you figure out if the function deletes the directory and the contents or just the contents, would you mind adding to a PHPDoc to the function explaining that?)
| */ | ||
| public function deleteExtension($type, $id): bool | ||
| { | ||
| switch ($type) { |
There was a problem hiding this comment.
Seeing as how this duplicates the switch block in the install function and that both locations will need to be updated at the same time, I'd prefer it if we created a private helper function to return the path for a given type and ID. Then there's only one location to update.
| @@ -203,5 +213,8 @@ | |||
| bb.reload(); | |||
| } | |||
| } | |||
| function onAfterModuleUninstalled() { | |||
There was a problem hiding this comment.
This shouldn't be needed.
You should be able to add data-api-reload="1" to have the page reload
| { | ||
| $this->deactivate($ext); | ||
|
|
||
| switch ($ext->type) { | ||
| $this->di['db']->trash($ext); |
There was a problem hiding this comment.
This is unneeded, deactivate already does this:
FOSSBilling/src/modules/Extension/Service.php
Line 387 in 0fcac8d
I agree, this ordering is odd. I think that settings first makes the most sense but either way is fine, just not in the middle. |
I disagree. The ordering "enable / disable" - "settings" - "uninstall" feels intuitive to me. Also, having the deactivate and uninstall buttons right next to each other makes for easy misclicks. |
|
Ideally it should only an remove button on deactivated plugins |
That is a good point, agreed. |
admdly
force-pushed
the
main
branch
3 times, most recently
from
b08f65b
to
b94d932
Compare
admdly
force-pushed
the
feature/530_uninstall_modules
branch
from
0fcac8d
to
7f81551
Compare
admdly
added
merge-hold
Fixes: #530
This PR introduces a new feature that allows modules to be uninstalled directly from the admin panel. This enhances the user experience by providing a more straightforward and efficient method for module management.