Administer triggers produces the following errors:

Notice: Undefined index: triggers in trigger_assign_form() (line 134 of /var/www/foobar/modules/trigger/trigger.admin.inc).
Warning: in_array(): Wrong datatype for second argument in trigger_assign_form() (line 134 of /var/www/foobar/modules/trigger/trigger.admin.inc).

An bug fix are attached.

CommentFileSizeAuthor
#10 trigger.admin_.inc_.patch638 byteschx
trigger.admin_.inc_.patch788 bytesquiptime
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jpmckinney’s picture

Status: Needs work » Needs review

CNR for the testing bot.

Do we need the call to count()? I can't replicate the bug, btw. How did you do it?

quiptime’s picture

- How did you do it?

I call the URLs
admin/structure/trigger/node
admin/structure/trigger/system
and so on

- Do we need the call to count()?

I think yes.

There are 2 situations with $metadata['triggers']

  1. $metadata['triggers'] not exist
  2. $metadata['triggers'] exist and is an empty array

Alternatively, if you do not want count($metadata['triggers']), you can even use !empty($metadata['triggers'])

jpmckinney’s picture

But you can call in_array() on an empty array -- it will just return FALSE.

I don't see those errors when I access those pages. Can you replicate it on a clean install of HEAD?

quiptime’s picture

Yes, i will replicate it on an fresh HEAD installation.

quiptime’s picture

Test result with an fresh HEAD install.

The reported Bug no exist.

I have found the cause for the reported error: It is the module "Views Bulk Operations".

Sorry for my hasty error report. And thank you believed not immediately my error report. I have informed the maintainer of the Views Bulk Operations module about this problem.

infojunkie’s picture

Hi, I'm the maintainer of Views Bulk Operations. If I understand this report correctly, actions in D7 should all contain a 'triggers' attribute. Given that there are dozens of modules that declare actions, I suggest that it would be better to keep the guard in this patch as a robust way against actions that might neglect to contain this attribute (which is, after all, optional).

jpmckinney’s picture

Status: Needs review » Reviewed & tested by the community

Checking the rest of core, it looks like isset() && count() is a common pattern so I take back my only note.

Dries’s picture

It is not clear to me whether this needs to be fixed here, or in the calling code. I guess it is OK to fix here.

jpmckinney’s picture

You think it should be actions_list()'s responsibility to clean up the array of actions (by assuming one of either array() or array('any') as the intended 'triggers' value)?

chx’s picture

FileSize
638 bytes

Iterating the array twice to find $hook or any? Ouch. Using count? Why on earth? If either $hook either 'any' is in the array then it's already nonempty.

catch’s picture

Was going to ask for a test for this but it's not reproducable in HEAD, so we just need to confirm the existing tests don't break with the patch, which they don't. RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.