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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yoroy’s picture

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.

xjm’s picture

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!

Cameron Tod’s picture

Assigned: Unassigned » Cameron Tod
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?

ankur’s picture

Status: Postponed (maintainer needs more info) » Needs review
FileSize
617 bytes

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:


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.

August1914’s picture

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

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

xjm’s picture

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

xjm’s picture

Title: Url path /admin/structure/trigger causes error » URL path /admin/structure/trigger causes error
sun’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.58 KB

More like this.

ankur’s picture

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

sun’s picture

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)

xjm’s picture

+++ 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.
tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.98 KB
1.22 KB

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

tim.plunkett’s picture

FileSize
2.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.

tim.plunkett’s picture

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

Jody Lynn’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

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!

David_Rothstein’s picture

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

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

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.77 KB

D6 backport.

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.