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.

Files: 
CommentFileSizeAuthor
#18 trigger_path-1306424-18.patch1.77 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#13 interdiff.txt2.01 KBtim.plunkett
#12 drupal-1306424-12-test.patch1.22 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 39,120 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#12 drupal-1306424-12-combined.patch2.98 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 39,145 pass(es).
[ View ]
#8 drupal.trigger-unassign.8.patch1.58 KBsun
PASSED: [[SimpleTest]]: [MySQL] 39,109 pass(es).
[ View ]
#4 trigger_module_form_func_called_witout_arg-1306424-4.patch617 bytesankur
PASSED: [[SimpleTest]]: [MySQL] 39,100 pass(es).
[ View ]

Comments

Version:7.8» 8.x-dev
Status:Active» Postponed (maintainer needs more info)

Someone confirming this on a clean install would help though :)
Bugs get fixed in D8 first, so moving over.

Version:8.x-dev» 7.x-dev
Status:Postponed (maintainer needs more info)» Active
Issue tags:+Novice, +Needs manual testing

#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!

Assigned:Unassigned» cam8001
Status:Active» Postponed (maintainer needs more info)

OK, 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?

Status:Postponed (maintainer needs more info)» Needs review
StatusFileSize
new617 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,100 pass(es).
[ View ]

The 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 is trigger_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:

<?php
function trigger_unassign($form, $form_state, $module, $hook = NULL, $aid = NULL) {
  if (!(
$hook && $aid)) {
   
drupal_goto('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.

Assigned:cam8001» Unassigned
Status:Needs review» Reviewed & tested by the community

patch applies cleanly against 7.14 , and resolves the issue described in the issue summary.

Thanks @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...).

Title:Url path /admin/structure/trigger causes errorURL path /admin/structure/trigger causes error

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new1.58 KB
PASSED: [[SimpleTest]]: [MySQL] 39,109 pass(es).
[ View ]

More like this.

@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 using drupal_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.

Status:Needs review» Needs work

You'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)

+++ b/modules/trigger/trigger.module
@@ -73,6 +76,17 @@ function trigger_menu() {
+ * Menu access callback for admin/structure/trigger/unassign.

Additionally, two minor things with this docblock:

  1. The formatting should be changed slightly to be more in line with http://drupal.org/node/1354#menu-callback
  2. Crell very vehemently denounced adding the menu path to the docblocks of these callbacks. The compromise was that we add an @see to the hook_menu() implementation that registers each.

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new2.98 KB
PASSED: [[SimpleTest]]: [MySQL] 39,145 pass(es).
[ View ]
new1.22 KB
FAILED: [[SimpleTest]]: [MySQL] 39,120 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

Here's a test, and it addresses the points made in #10 and #11.

StatusFileSize
new2.01 KB

Oh, 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.

_node_revision_access is the only access callback I could see that uses drupal_static. And AFAICS, this is only called once per request.

Status:Needs review» Reviewed & tested by the community

I tested and reviewed and couldn't find any problems with #12.

Status:Reviewed & tested by the community» Fixed

This looks like a pretty straight-forward bug fix to me.

Committed and pushed to 7.x. Thanks!

Version:7.x-dev» 6.x-dev
Status:Fixed» Patch (to be ported)

In theory I think this exists in Drupal 6 too, right?

Status:Patch (to be ported)» Needs review
StatusFileSize
new1.77 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

D6 backport.