DX: Remove module actions on uninstall

We now have an API just for this purpose! :) Please use hook_modules_uninstalled() and add it to system.module.

Included in the list in #306151: Automatically install/uninstall schema.

Shouldn't this be in module.inc?

@Xano Nope. .inc files are API *only.* Hooks are only called from module files. Plus, the actipns code lives in system.module so it makes perfect sense to have the related hook_modules_uninstalled there as well.

Hooks are not only called from modules. How did you think hook_modules_installed() and friends were called? Exactly, in module_disable(). Personally I believe every installation/unstallatio-related work that is not part of any module (There is no Actions.module) should be put in there.

API files are free to call modules_invoke_all(), which triggers hooks, but API (.inc) files do *not* contain hooks themselves. The whole reason I pushed to get hook_modules_uninstalled, etc() into D7 core was to be able to do things like this and *not* have a crap ton of crud in drupal_uninstall_modules(). Since the system module is reponsible for the actions administration, it can now react when other modules are uninstalled and remove their actions automatically.

I'm not talking about putting hook implementations in .inc files (which wouldn't even work with the current hook system). I'm talking about module_enable() and module_disable() to do this kind of cleanup directly. If there would be a separate Action module, that module should cleanup the actions, but since actions don't belong to any module I believe it is nonsense to us system_modules_disabled() for this.

If system.module already aministrates actions, then doesn't it make sense to put the clean-up there too?

Is there a real performance cost associated with implementing the hook?

There is a DX component to hook_modules_disabled() as well.

If we handle the removal of actions, menus, node types, and so forth in core, then your module never has to implement hook_disable(), which saves us one more thing we have to worry about when writing a module.

So putting cleanup functions in system_modules_disabled() is a good things, because it provides consistency: If you use the API properly, your actions are installed/uninstalled automatically. Good DX!

Before calling actions_delete ensure drupal_function_exists and this new hook should be called somewhere

+ foreach($modules as $module){
missing spaces.

Putting this in system_modules_uninstalled() makes sense. andypost is there any situation where actions_delete() wouldn't be available here?

@catch actions_delete() available everytime cos they live in actions.inc

Im scared about:
1) trigger assignments which possible stay in DB
2) another modules which uninstalled same time but not loaded this moment

For example trigger_actions_delete() - sits in trigger.module and cleans trigger assingments

Case:
if trigger module is disabled when another module been uninstalled so no trigger assignments will be deleted - suppose it's wrong because actions from module can have assignments before trigger.module was disabled. Maybe it's a another issue so we need wise trigger_enable.

example:
unstalation of modules somemodule and trigger
1) in loop drupal_load(somemodule), retrive its actions_info, process delete
2) drupal_load(trigger), but its trigger_actions_delete() is never called for actions from module somemodule

so I think we should load all modules and only then call actions_delete

Oh I see what you mean, hook_actions_delete() for disabled modules might not be available. I think that's probably outside the scope of this patch - if modules remove their own actions manually they can't account for that either.

#306540: Orphaned assigned actions still triggered and cannot be removed is fixed and now tests are working after #601398: Simpletest does not allow assigning actions to triggers

Let's separate actions which lives in system.module and triggers ({trigger_assignments} table)

When a module is about to be installed |enabled|disabled system.module should reflect and sync module's actions whick already done in system_modules_submit() but nothing is happen in system_modules_uninstall_submit()

Also actions_synchronize() call action_delete() so if trigger.module enabled it will hook trigger_actions_delete()

If trigger.module enabled same things should happen with {trigger_assignments} table! So I changed trigger_install() to trigger_enable() to sync {trigger_assignments} because hook_enable() is called when trigger.module installed and enabled too.

Also this patch extends a bit test from commited #306540: Orphaned assigned actions still triggered and cannot be removed

Not all modules have hook_schema() or hook_uninstall() their actions and triggers are live in DB but now not called because #306540: Orphaned assigned actions still triggered and cannot be removed

This patch just make things little clear.

Could correlate with #403446: Increase default weight of trigger module

#20: 420124-sync-actions.patch queued for re-testing.

Re roll to trace latest changes

subscribing

Why does trigger_enable() not use db_delete()?

@agentrickard because I don't know how to add this condition to db_delete()

With some help from Crell, we have:

$select = db_select('actions')
  ->fields('actions', array('aid'));
db_delete('trigger_assignments')
  ->condition('aid', $select, 'NOT IN')
  ->execute();

I think we probably need a test that does with this, that verifies that actions get removed.

+++ modules/system/system.admin.inc	29 Mar 2010 00:29:06 -0000
@@ -1384,6 +1384,9 @@ function system_modules_uninstall_submit
+    // Synchronize to catch any actions that were removed.
+    actions_synchronize();

What about just calling actions_synchronize(TRUE) and get rid of the orphans? Or maybe we need to make a distinction between "uninstalled orphans" and "disabled orphans".

      $link = l(t('Remove orphaned actions'), 'admin/config/system/actions/orphan');

I have clicked the link for admin/config/system/actions/orphan a number of times... Does it do anything? Or does it just take you to the actions page? Maybe this is another bug that I just don't want to admit.

131 critical left. Go review some!

@rfay this already have tests for orphants #306540: Orphaned assigned actions still triggered and cannot be removed

Patch re-rolled with proposed changes for db_delete()

EDIT: testActionsContent() class already have tests for unassign, and testActionsOrphaned()

Hm, patch was lost

#30: 420124-sync-actions.patch queued for re-testing.

maybe we need to make a distinction between "uninstalled orphans" and "disabled orphans".

This is a good idea but I cant imagine implementation... this approach require details about actions from "disabled" modules (table "system" with "status" == 0 and "schema_version" != -1 )

But we could delete module actions in system_modules_uninstall_submit() and implement hook_modules_uninstalled() for trigger module

Re-roll, suppose no need to separate "disabled" and "uninstalled" because removing orphans is manual task

@rfay see system_actions_remove_orphans()

Addition to #20
- Added comment to trigger_enanble()
- Removed system_action_delete_orphans_post() because unused

#33: 420124-sync-actions.patch queued for re-testing.

#33: 420124-sync-actions.patch queued for re-testing.

Moving.

#33: 420124-sync-actions.patch queued for re-testing.

Strange, patch been applied with offset, anyway reroll

>patch -p0 < 420124-sync-actions_2.patch
patching file modules/system/system.admin.inc
Hunk #1 succeeded at 1322 (offset -66 lines).
patching file modules/trigger/trigger.install
Hunk #1 succeeded at 45 with fuzz 2 (offset 2 lines).
patching file modules/trigger/trigger.test
Hunk #1 succeeded at 729 (offset 301 lines).
Hunk #2 succeeded at 737 (offset 301 lines).

+++ b/modules/system/system.admin.inc
@@ -1322,6 +1322,9 @@ function system_modules_uninstall_submit($form, &$form_state) {
+    // Synchronize actions to remove all related to uninstalled modules.
+    actions_synchronize();

I was about to say, this should go in actions_modules_uninstalled() ...

(#1008166: Actions should be a module)

Code looks great, but I didn't try it out.

Powered by Dreditor.

#40: 420124-sync-actions-triggers.patch queued for re-testing.

Since trigger module now lives in contrinb D7 still should be fixed