Arbitrary actions/events instead of 'flag' and 'unflag'

Applied this patch (and the patch for flag_abuse) and after some testing I give it a thumps-up. No negative interference with the normal flag behaviour. No ugly messages in the watchdog. Seems fine.

So it seems that this is a bit more complex than I originally thought. The problem with the current feature patch is that there is no consideration for the 'messages' for each flag event. So if we add a 'reset' event, we also need messages, i.e. 'reset_short', 'reset_long', 'reset_confirmation'

While these can be thrown into the flag definition, they are not currently picked up by the flag_form() within flag.module. So this needs work.

Ok, here is an updated patch. Important things to note on this:

1) Introduction of a $flag->events array in order to list 'flag', 'unflag', and any other event needed. This is added to the serialized options and default_options() method of the flag_flag() class.

2) module_invoke_all()s for each flag type's default options so that they can subsequently be extended by other modules with hook_flag_[type]_default_options()

3) Usage of this new events array option within the form builder when creating/editing a flag.

4) Validation of multiple event _confirmation messages if this link type is used.

Worked for me

Love the new hooks, could use a shared flag_default_options though

I like the code clean-up with this patch, but I'm not a fan of the introduction of 3 new hooks:

These seems awfully redundant. We could easily combine them into one hook and just pass in a $type parameter.

Conceptually this makes me a bit worried also. "Arbitrary" events? Should we really support anything more than a defined list? It seems like if #327901: Action: clearing a node's flaggings (or #547082: Make "Trim flag" an API function) were completed and we just added "reset" as a standard event, then we wouldn't really need this additional flexibility. I'd like Amitaibu to weigh in on this issue if possible, sounds like sirkitree and Amitaibu have been solving the same problem in different ways.

So the problem with trying to use #547082: Make "Trim flag" an API function for this, is that the goal of that function is to simply reset a particular user's flagging.
+  $result = db_query("SELECT * FROM {flag_content} WHERE fid = %d AND (uid = %d OR uid = 0) ORDER BY timestamp DESC", $flag->fid, $uid);
Meaning that if we want to reset ALL flags upon a particular piece of content (be it node, comment or user) we would have to first get a list of all the users who have flagged this content and then call this function for every single user. not only are we running the above SELECT for each user, we would also be running the flag('unflag'... operation for each user resulting in a whole lot of queries.

To address your concern about supporting "Arbitrary" events, you're not really doing so. You're supporting the ability of other modules to implement arbitrary events that they would create, putting the support of such upon them. You only have to support the 'flag' and 'unflag' events. You can see an example of this here: #478490: Reset event for Flag where flag_abuse would implement the event 'reset' on it's own.

As for the redundant hooks, I've updated the patch to only make one call and pass a type as you suggest and have also changed this to be against the 2x branch as I believe that is where you're making any changes now.

The option to alter flag options was added in #533424: Add hook_flag_options_alter() (create a 2.x branch)., does this make the addition of your new hooks unnecessary?

See this code in the options() method in the flag_flag class:

Other than that, I really wonder if the ability to reset flaggings should be included directly in Flag, rather than being implemented by Flag Abuse (or worse, multiple modules implementing the "reset" action).

I realized that drupal_alter('flag_options', $options); probably isn't satisfactory, we should pass in the $this variable so that the entire flag object is available, then implementing modules can check $flag->type from there.

Yes, as long as the $this variable is passed in so that we can do type detection, I think it would negate the need for the additional hook. New issue opened to extend that: #577076: Extend hook_flag_options_alter()

Applying the patch failed for Hunk#1 at line 211. Is this patch up to date?

No, I'm in transit this week but will try to get to get this re-rolled soon.

Marking as needs work

Thx. Sorry for coming back to this so late.

Ok, finally took the time to re-roll (since Nate decided to push on with a beta I got my ass in gear ;)

Currently there seems to be a problem with the introduction of the new hooks 'flag_default_options'... on my local I have a node flag and the module_invoke_all() in the flag_node class seems to throw the admin page into a loop. No clue why just yet, will dig more tomorrow.

I removed the extra module_invoke_all()s as they're not needed since we're altering the original options in flag_flag().

All seems to work alright - however, in trying to utilize hook_flag_options_alter() - it seems as though my alterations are not being effected... I'll open a separate issue for that.

I'm still not a fan of "arbtirary" actions. There's a lot of logic in Flag (and I imagine other Flag-dependent modules) that assume something like $setting = $action == 'flag' ? $x : $y. These modules will all work just fine until some other module adds in a third state and starts triggering unexpected flag/unflag event behaviors within those other modules.

Additionally there's still the problem that other states besides "Flag" and "Unflag" are not accommodated for in the UI, nor do we really have a place to put them. I'd really, really prefer to make a separate function/API for "Reset" (just like Trim) rather than abstracting an API that is not meant to be abstracted. I'm fine with adding a new function such as flag_reset_flag() and making a new hook within that module for hook_flag_reset_flag(). I just can't see any other possible actions that would be required other than reset, and the need to customize this message seems extraordinarily small. While adding this functionality, it would make sense to provide administrators with a "Reset" link on admin/build/flags.

Here's a patch that implements a "reset" function and hook. I'd much much rather have such an API than extending flag() to do multiple actions (hook_nodeapi() anyone?). This implementation leaves it pretty wide open how it will be used, similar to the flag_trim_flag() API function, which AFAIK is only in use by Rules.

Alright, I can understand your concerns about the other states not being supported in the UI. And simply having a reset function takes care of my problem. Sometimes the best solution is the simplest. :)

I guess I first came to the conclusion that this was the way to go because when preprocessing a flag, one of the $vars is 'action' and changing it really has no effect. So I made so that it does.

However, there is still the question of the messages and role permissions. I have no problem simply form_alter()ing the flag_form in order to add reset_ messages and access['role']s, but it should be noted somewhere that while you can alter the existing flag options, you cannot add to them. @see my hook_flag_options_alter() implementation in my patch which will not work now if this is not added.

So all in all, without this feature it's a little more work on the flag_abuse end, but that module is already so tiny I don't think it matters a whole heck of a lot :)

Thanks for your continued attention Nate! I'll hack out support for 2.x in flag_abuse with this new function in mind and report back ASAP.