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.
In the Navigation menu, the item Structure > Triggers > Unassign, which points to '/admin/structure/trigger' and redirects to '/admin/structure/trigger/node' causes this error:
Warning: Missing argument 3 for trigger_unassign() in trigger_unassign() (line 50 of /Applications/MAMP/htdocs/mediation-d7/modules/trigger/trigger.admin.inc).
I have no triggers assigned, but deleted just one trigger. Don't know if it also occures on a fresh install.
Comment | File | Size | Author |
---|---|---|---|
#18 | trigger_path-1306424-18.patch | 1.77 KB | Albert Volkman |
#13 | interdiff.txt | 2.01 KB | tim.plunkett |
#12 | drupal-1306424-12-test.patch | 1.22 KB | tim.plunkett |
#12 | drupal-1306424-12-combined.patch | 2.98 KB | tim.plunkett |
#8 | drupal.trigger-unassign.8.patch | 1.58 KB | sun |
Comments
Comment #1
yoroy CreditAttribution: yoroy commentedSomeone confirming this on a clean install would help though :)
Bugs get fixed in D8 first, so moving over.
Comment #2
xjm#764558: Remove Trigger module from core has been committed, so moving back to D7.
Steps to reproduce from a clean install would indeed be helpful here. Setting active and tagging for some manual testing. See if you can reproduce this starting from a clean install of D7 from git. Document the steps used, starting from "1. Install Drupal core." If it can't be reproduce, then set the issue back to postponed (maintainer needs more info). Thanks!
Comment #3
Cameron Tod CreditAttribution: Cameron Tod commentedOK, tried testing:
1. Install Drupal Core (from 7.x trunk)
2. Enable Trigger module at admin/modules
3. Navigate via UI to admin/structure, then admin/structure/triggers
No warning...
So I tried creating a 'Change the author of content' action, assigning it to a 'Node Published' trigger, and then deleting the action - still no such warning.
capono, could you post some steps to reproduce?
Comment #4
ankur CreditAttribution: ankur commentedThe error only occurs when you go directly to admin/structure/trigger/unassign.
On a fresh install, there is no way to navigate to this path. However, if you have the admin_menu module enabled, it is possible to navigate to this URL.
The page callback for this path is
drupal_get_form()
. The function that builds the form array istrigger_unassign()
, defined in trigger.admin.inc.The logic in this function seems intended to account for the possibility that someone might come directly to this URL, with an if-statement at the beginning of it to redirect the user to the main trigger admin page at admin/structure/trigger:
However, the function signature doesn't allow the
$module
parameter to be optional, as it does with the$hook
and$aid
parameters.I think the attached patch should resolve the issue.
Comment #5
August1914 CreditAttribution: August1914 commentedpatch applies cleanly against 7.14 , and resolves the issue described in the issue summary.
Comment #6
xjmThanks @August1914!
Since this patch resolves a functional bug, we need to add an automated test that fails when the bug is present and passes when combined with the patch in #4. Reference: http://drupal.org/core-gates#testing. (See also: http://xjm.drupalgardens.com/blog/core-mentoring-and-xjms-guide-patch-re...).
Comment #7
xjmComment #8
sunMore like this.
Comment #9
ankur CreditAttribution: ankur commented@sun
I'm wondering if loading all the actions straight from the database is the best way to check access from a menu item's access callback function.
Might it make sense to have
actions_get_all_actions()
cache the value (maybe usingdrupal_static()
) so we're not repeating that work twice if we end up having to load the actions again in order to build the form for unassigning triggers?The patch in #8, as it is written now, would result in querying the database twice for all actions. I'd be more than happy to re-roll a patch in which
actions_get_all_actions()
uses some kind of caching if it sounds like we should.Comment #10
sunYou're right. Let's change the check in the menu access callback into a quick and simple
$has_actions = db_query_range('SELECT 1 FROM {actions}', 0, 1)->fetchField();
(or whatever query is needed to determine the existence of custom configured actions that can be unassigned)
Comment #11
xjmAdditionally, two minor things with this docblock:
Comment #12
tim.plunkettHere's a test, and it addresses the points made in #10 and #11.
Comment #13
tim.plunkettOh, I forgot the interdiff.
actions_get_all_actions is unrelated in a sense, since it just lists available actions, and doesn't care if they are assigned or not.
So, I went with a count query on trigger_assignments.
Comment #14
tim.plunkett_node_revision_access is the only access callback I could see that uses drupal_static. And AFAICS, this is only called once per request.
Comment #15
Jody LynnI tested and reviewed and couldn't find any problems with #12.
Comment #16
webchickThis looks like a pretty straight-forward bug fix to me.
Committed and pushed to 7.x. Thanks!
Comment #17
David_Rothstein CreditAttribution: David_Rothstein commentedIn theory I think this exists in Drupal 6 too, right?
Comment #18
Albert Volkman CreditAttribution: Albert Volkman commentedD6 backport.