Posted by zzolo on October 21, 2008 at 1:54pm
9 followers
| Project: | Trigger |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | actions, Needs tests, triggers |
Issue Summary
The menu item for custom triggers uses the default "access callback" (user_access), and uses the module name as the "access argument".
The menu item, $items["admin/build/trigger/$module"] on/around line 95 in trigger.module invokes the "hook_info" hook which is where modules describe how they would like to define new triggers.
There should exist support for the hook to define both the "access callback" and "access argument". The predefined triggers use different permissions, so triggers provided by other modules should not be so restricted as well.
I can (and will soon) provide a patch for this. It would simply involve looking for certain elements in the "hook_info" array provided by a module.
Comments
#1
Note: when referring to hook_info above, I mean "hook_hook_info"
#2
Note: This can be worked around by using "hook_menu_alter".
#3
Looking into patching this, it is more complicated that originally expected. Due to the structure of the array that "hook_hook_info" provides, more than just the menu item will need to be changed. I will still try to create a patch but it will take me some time.
#4
I'm interested in this issue as well, having lost a day trying to define my first custom triggers last week only to discover the User 1 could see everything but my "regular" admin account could not. My short-term workaround is to define an extra perm named for the module implementing the trigger, but this is (to say the least) unsatisfying.
@zzolo: if there's anything I can do to help with the patch, please let me know :)
#5
Feature requests go in the 7.x-dev queue...
#6
I am not sure what scared me so much before, but this was a pretty easy fix. I just allowed for two new options in the hook_hook_info() (this is a bad name, by the way). These are names as the same items in the menu array.
trigger.module in trigger_menu():
<?php// We want contributed modules to be able to describe
// their hooks and have actions assignable to them.
$hooks = module_invoke_all('hook_info');
foreach ($hooks as $module => $hook) {
// We've already done these.
if (in_array($module, array('node', 'comment', 'user', 'system', 'taxonomy'))) {
continue;
}
$info = db_result(db_query("SELECT info FROM {system} WHERE name = '%s'", $module));
$info = unserialize($info);
$nice_name = $info['name'];
$items["admin/build/trigger/$module"] = array(
'title' => $nice_name,
'page callback' => 'trigger_assign',
'page arguments' => array($module),
'access arguments' => isset($hook['access arguments']) ? $hook['access arguments'] : array($module),
'access callback' => isset($hook['access callback']) ? $hook['access callback'] : 'user_access',
'type' => MENU_LOCAL_TASK,
);
}
?>
Also changed documentation in trigger.api.php. An example hook_hook_info() implementation for the node module if it needed to use these:
<?phpfunction hook_hook_info() {
return array(
'node' => array(
'node' => array(
'presave' => array(
'runs when' => t('When either saving a new post or updating an existing post'),
),
'insert' => array(
'runs when' => t('After saving a new post'),
),
'update' => array(
'runs when' => t('After saving an updated post'),
),
'delete' => array(
'runs when' => t('After deleting a post')
),
'view' => array(
'runs when' => t('When content is viewed by an authenticated user')
),
),
'access callback' => 'user_access',
'access arguments' => 'administer actions',
),
);
}
?>
#7
I just had an idea. The default access callback for any modules that implement this hook, should be
traigger_access()(I believe that is it, not looking at code). As I imagine this is why the default access argument is the module name. I can't believe I am just seeing this.I'll make another patch.
#8
It was the
trigger_access_check()function. New patch. Also, now that it finally clicked why the default access argument would be the module name, I definitely think this is a bug and putting it back to such.#9
#10
Looks sensible but I think we need a test for this.
#11
The last submitted patch failed testing.
#12
I think this issue is the root cause of an issue I've been trying to solve for a while. #368573: Bug in trigger module behavior
#13
I created a new patch with proper tests. In doing this, I came across a few new things:
* Bug: #551002: Bad Query in Menu Creation in Trigger
* in trigger_forms, we need to make a check for the new array indexes.
#14
The last submitted patch failed testing.
#15
#296322: Tests for abort of actions firing in a loop + trigger module API is completely broken + fix changed around the trigger module. Got rid of
trigger_access_check()so I put the default callback for hook implemented triggers asuser_access(). Uploading new patch that accounts for changes.#16
The last submitted patch failed testing.
#17
Ugh! Replacing the default access callback to
user_access()basically recreated the original issue. So, now, the default call back isuser_access()but the default access argument is 'administer actions'. Should pass tests now.#18
back to needs review.
#19
The last submitted patch failed testing.
#20
HEAD is broken.
#21
looked over patch, a few things:
1. there are some whitespace changes which you didn't need to fix, but considering that you probably ran this through coder, i'm willing to let that slide (though curious to learn more about the code cleanup process in general)
2. you should change "test to test permissions " to "tests to test permissions "
3."Menu access callback as ran through hook_hook_info()." seems gramatically incorrect. -- maybe change to 'as run through' (though that is still weird too) - 'run menu access callback through...' seems better, but i'm not sure if that's what you are doing...
otherwise, my tests locally ran smoothly.
#22
patch needs work (grammatical changes)
#23
updated. re-rolled.
#24
all better. looks good. still works locally, too.
someone else should also review this patch as well.
#25
all better. looks good. still works locally, too.
someone else should also review this patch as well.
#26
The last submitted patch failed testing.
#27
Tagged. patch very outdated
#28
I had put this on the back burner because it is a bug and I heard there was some major work going into this module, so I was just going to wait until after the slush to redo the patch. There is already tests in the patch, fyi, but yes the last patch is way dated. Will pick back up soon.
#29
This should wait til #601398: Simpletest does not allow assigning actions to triggers
#30
This no longer a bug. The current version of the trigger module just uses "administer actions" for all access to trigger pages instead of the broken module name. Though I do not think this is a good way to handle it, it is adequate and I have never really run into anyone that needs the functionality of providing menu access with custom triggers (the menu alter hooks are a get around anyway). Also, sine is now a feature request, it will not get into D7. Putting as D8 and postponing.
#31
Sorry folks, but it would be really handy if you could update the documentation at...
http://drupal.org/node/375833
... because this issue applies to Drupal 6.x as well. Thanks.
#32
@moritzz, as you have an ccount on drupal.org, you can edit handbook pages instead of just leaving a comment.
#33
Based on #764558: Remove Trigger module from core, this issue will not be fixed since Trigger has been removed from D8. I'd suggest the Rules module, since the D8 contrib version of trigger is likely to be maintenance only.
#34
Let's re-assign this issues to the trigger module