Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Actions should be removed during module un-installation.
Comment | File | Size | Author |
---|---|---|---|
#44 | 420124-sync-actions-triggers44.patch | 2.61 KB | andypost |
#40 | 420124-sync-actions-triggers.patch | 2.6 KB | andypost |
#33 | 420124-sync-actions.patch | 3.04 KB | andypost |
#30 | 420124-sync-actions.patch | 2.96 KB | andypost |
#23 | 420124-sync-actions.patch | 2.89 KB | andypost |
Comments
Comment #1
ultimateboy CreditAttribution: ultimateboy commentedAnd, here is the patch.
Comment #2
ultimateboy CreditAttribution: ultimateboy commentedIssue description should be...
"Actions should be removed during module un-installation."
Comment #3
Dave ReidWe now have an API just for this purpose! :) Please use hook_modules_uninstalled() and add it to system.module.
Comment #4
Dave ReidIncluded in the list in #306151: Automatically install/uninstall schema.
Comment #5
ultimateboy CreditAttribution: ultimateboy commentedThat is handy. Moved the delete to hook_modules_uninstalled() in system.module.
Comment #6
XanoShouldn't this be in module.inc?
Comment #7
Dave Reid@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.
Comment #8
XanoHooks 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.
Comment #9
Dave ReidAPI 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.
Comment #10
XanoI'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.
Comment #11
cburschkaIf 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?
Comment #13
webchickComment #14
agentrickardThere 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!
Comment #15
andypostBefore calling actions_delete ensure drupal_function_exists and this new hook should be called somewhere
Comment #16
catch+ 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?
Comment #17
andypost@catch
actions_delete()
available everytime cos they live in actions.incIm 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
Comment #18
catchOh 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.
Comment #19
andypost#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
Comment #20
andypostLet'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.
Comment #21
andypostCould correlate with #403446: Increase default weight of trigger module
Comment #22
andypost#20: 420124-sync-actions.patch queued for re-testing.
Comment #23
andypostRe roll to trace latest changes
Comment #24
rfaysubscribing
Comment #25
agentrickardWhy does trigger_enable() not use db_delete()?
Comment #26
andypost@agentrickard because I don't know how to add this condition to db_delete()
Comment #27
agentrickardWith some help from Crell, we have:
Comment #28
rfayI think we probably need a test that does with this, that verifies that actions get removed.
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".
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!
Comment #29
andypost@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()
Comment #30
andypostHm, patch was lost
Comment #31
andypost#30: 420124-sync-actions.patch queued for re-testing.
Comment #32
andypostThis 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
Comment #33
andypostRe-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
Comment #35
andypost#33: 420124-sync-actions.patch queued for re-testing.
Comment #36
andypost#33: 420124-sync-actions.patch queued for re-testing.
Comment #37
agentrickardMoving.
Comment #38
andypost#33: 420124-sync-actions.patch queued for re-testing.
Comment #40
andypostStrange, patch been applied with offset, anyway reroll
Comment #41
tstoecklerI 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.
Comment #42
fgm#40: 420124-sync-actions-triggers.patch queued for re-testing.
Comment #44
andypostSince trigger module now lives in contrinb D7 still should be fixed