Have hook_flag_validate()

+1
I am using Flags + rules as a registration feature and right now I have to add the flag then remove it if I am over the number of registrations. hook_flag_validate() would make the process much simpler.

@mooffie: I don't have the time necessary to fully implement (or even review) this proposal. However I trust in your judgment and it does sound like an excellent idea.

+1 subscribing. This would be great in allowing a maximum of 50 users to flag an event node as 'attend'. So if someone unflags, the link will then show again and another user will be able to flag the node.

subscribing

Hello!

I've tried the patch for a short while...seems to be working great except that the failure message doesn't fade out the same way flagged/unflagged messages do. :o (or is this by design?)

Haven't had the chance to investigate further yet. Just reporting if anybody else experienced this behavior. :)

I see...yes, I agree that it should be configurable. If I may suggest, instead of returning a string being the failure message, an array may be returned containing the message and the duration of the display.

Maybe something like:

<?php
function MYMODULE_flag_validate($action, $flag, $content_id, $account) {
  if ($flag->content_type == 'node') {
    $node = $flag->fetch_content($content_id);
    if (strpos($node->title, 'days') !== FALSE) {
      return array(
        'message' => t("You may not flag a node with the word 'days' in its title."),
        'duration' => 10000 //milliseconds; 0 = do not fade out
      );
    }
  }
}
?>

I haven't looked into the code much if this would impact other things so forgive me if this isn't viable. :) I just think that this would be an easier/faster solution for people using this hook on his/her custom module, not to mention they can set different timeouts/behavior for the messages.

I didn't run into any issues with it working against the 6.0-2.0-beta5. I would vote to skip the validate hook if $skip_permission_check is true, since I could see that being confusing or a pain in the future. Thanks for a great module!

One more comment: Doesn't it make sense to keep the function signature consistent with hook_flag_access, since it's just a difference of reordering the arguments?

Subscribe. Any idea as to whether we'd get this into D7 soon too?

Hi,

I was interested in having this in Drupal 7 as well and I wrote a small patch which allows to add a simple check within hook_flag_validate, returning a boolean...

Note that I'm using this in Commerce Credits Flag

Does anyone have a working patch for Beta6? The patch above seems to only work up to beta 4

Marking as needs work. Also, needs to be applied to D7 first, then backported.

Also, any new hook needs documenting in the api.php file.

Tagging.

Rerolled for D7 dev.

Ugh, sorry I should have read all of this issue before tagging it as Novice.

The patch at #17 is a cut-down version of the original patch, which only allows failure of validation and no messages.

Bumping version to 3.x. This is going to need involved work from someone who needs this feature! :)

I need this functionality, and am attaching a re-roll of the patch in #6 against 7.x-3.x-dev - it's working nicely for me, but obviously needs review. If it's ok with the maintainers, I'd like it if the "7.x-3.0 release blocker" tag could be added to this issue - thoughts?

Thanks for working on this!

Regarding blocker status, I'm not sure I think this qualifies: we've been living without it for 2 years now, and while it opens the door to lots of cool new uses of Flag, I don't think it's an essential for a 3.0 release.

However... I doubt I'll be releasing 3.0 right away, as we still have a few blockers and things aren't moving fast. So you've definitely got a good few weeks (at least!) to get this patch ready. In other words, I'll consider this a 'nice to have' for 3.0 rather than a 'must have'.

It does need a bit of work though...

First up, a new hook needs documenting in the api.php file :)

I've given it a run through with a couple of dummy hooks returning errors, and while it works, it seems to me there's a problem when we return multiple error messages. One line of message text sneaks into the padding around a node content in most themes, but more than one and it overflows and looks messy. I think a bit of CSS is needed here -- maybe something like a box with a border around these do the trick?

Here's a few things I've spotted that need cleaning up or seem a bit funny:

+++ b/flag.module
@@ -1604,9 +1604,17 @@ function flag_theme() {
+function theme_flag_errors($errors) {
+  // Note: by default we can't use <div> because it's not valid inside <span>.
+  return join('<br />', $errors);
+}

I don't think we need a whole new theme function just for joining up some strings. Should anyone really need this, they can do the work in the flag theme preprocessor.

+++ b/includes/flag.pages.inc
@@ -25,45 +26,40 @@ function flag_page($action, $flag, $entity_id) {
-    print drupal_json_encode(flag_build_javascript_info($flag, $entity_id));
+    print drupal_json_encode(flag_build_javascript_info($flag, $entity_id, $errors));

I'd like a comment here to say that the JS handles outputting errors, since there's no obvious handling of them there with JS enabled.

+++ b/includes/flag.pages.inc
@@ -122,20 +118,27 @@ function flag_confirm_submit(&$form, &$form_state) {
     drupal_set_message(t('You are not allowed to flag, or unflag, this content.'));

We removed this message in flag_page() (AFAICT!), so it should be removed in the other places too presumably?

+++ b/includes/flag.pages.inc
@@ -122,20 +118,27 @@ function flag_confirm_submit(&$form, &$form_state) {
+    $flag->flag($action, $entity_id, NULL, FALSE, NULL, $errors);

We've just got a $result from a flag() call, so why are we doing it all over again here?

Finally, I'm a bit concerned about adding yet another parameter to flag(). We've got $skip_permission_check, we recently added $flagging. The more parameters, the more unwieldy this gets, but that's just the first consequence.

Technically we should be adding $errors to the main API function, flag(), but then that means we need to tackle #1689530: flag() should support $skip_permission_check to make all the long list of parameters consistent. But then the long list of parameters means that using flag() and getting the errors means you need to pass in the defaults for all the optional params that come before it, which is rubbish DX: they effectively cease to be optional.

Hence I am wondering whether the errors should be handled using exceptions: we throw a FlagException in flag_flag::flag() if we get some errors from either our own code or the hook invocation.

On the other hand, using exceptions make using the API function fiddly too: with an $errors parameter you can just choose to ignore it if you don't want to return error output, but with exceptions you always have to use a try/catch block just in case.

It's still a bit too early in the morning to think this one through, so I'll ponder this one some more -- though any opinions welcome :)

PS. On the subject of lots of parameters:

    // @todo: Should we skip this if $skip_permission_check == TRUE ?

We should probably pass $skip_permission_check to the hook for implementations to figure it out.

Also, passing the $flagging to the hook is necessary I think. I can see implementations wanting to work with field values on that.

I've thought of a third option: we store an array of errors in the flag, and add a method to retrieve it.

Thus:

With $errors param:

// Don't care about errors.
$flag = flag_get_flag(...);
$flag->flag('flag', ID)

// Need to get errors.
$flag = flag_get_flag(...);
$errors = array();
$flag->flag('flag', ID, NULL, FALSE, NULL, $errors);
if ($errors) {
  // Output.
}

With exceptions param:

// Don't care about errors.
$flag = flag_get_flag(...);
try {
  $flag->flag('flag', ID)
}
catch (Exception $e) {}

// Need to get errors.
$flag = flag_get_flag(...);
try {
  $flag->flag('flag', ID)
}
catch (Exception $e) {}
  // Output.
}

With get_errors() method:

// Don't care about errors.
$flag = flag_get_flag(...);
$flag->flag('flag', ID)

// Need to get errors.
$flag = flag_get_flag(...);
$flag->flag('flag', ID, NULL, $errors);
if ($errors = $flag->get_errors()) {
  // Output.
}

It actually saves us a line of code when getting errors out, and requires no extra work if you don't care about errors, so it seems like the best way to me. It also means we get the $errors array documented -- I've not managed to spot where it's explained what the array keys do ;)

Many thanks for the detailed feedback - I'll work on a re-roll in the next couple of days with the get_errors method, which seems like a nice way to go IMO.

patch attached incorporating the feedback in #25 and #26. FWIW, the hook documentation in flag.api.php includes an example implementation of the use case in #814960: limiting how many nodes/content types a user can flag.. Thanks for helping move this forward!

Looking good. Just a few more minor things to tidy up.

+++ b/flag.api.php
@@ -121,6 +121,36 @@ function hook_flag_unflag($flag, $entity_id, $account, $flagging) {
+ *  The action to test. Either 'flag' or 'unflag'.

Instead of 'test', can we say 'The action about to be carried out' here?

+++ b/flag.api.php
@@ -121,6 +121,36 @@ function hook_flag_unflag($flag, $entity_id, $account, $flagging) {
+ *  The id of the entity the flag is on.

'The id of the entity the user is trying to flag or unflag.'

+++ b/flag.api.php
@@ -121,6 +121,36 @@ function hook_flag_unflag($flag, $entity_id, $account, $flagging) {
+ * @param $flagging
+ *  The flagging entity.
+ */
+function hook_flag_validate($action, $flag, $entity_id, $account, $skip_permission_check, $flagging) {

Good stuff on the example code. We need a @return here too.

+++ b/includes/flag.pages.inc
@@ -25,45 +25,40 @@ function flag_page($action, $flag, $entity_id) {
+    $flag->errors['token'] = t('Bad token. You seem to have followed an invalid link.');

I think I'd rather keep the errors array internal to the flag, so the errors we build up in flag_page() we just put in a local $errors array, like this:

$errors = array();
$errors['foo'] = 'bar';

$flag->stuff();
$errors += $flag->get_errors();

if ($errors) {
 // stuff
}

+++ b/theme/flag.css
@@ -9,6 +9,12 @@
+.flag-message.flag-failure-message {
+  border: 1px solid black;
+  background-color: #F5DCE6;
+  padding: 2px;
+}
+

I'm actually wondering now whether we could borrow the 'warning' class that Drupal core defines. That gives us a red border and a pale red background.

Regarding the CSS, could you compare with #1620614: .flag-message styling?

re-roll against latest HEAD attached, addressing the issues in #29. As regards the CSS, unfortunately I am not a CSS expert either. I've copied the colors from the core warning class, but I think that adding the warning class to the failure message div could make over-riding a bit messy. Would it be possible to get the hook in on its own merits and address the CSS issues in a follow-on issue? I'd be happy to ask for input from a front-end dev at my company...

whoops, forgot to actually attach the patch after all that!

+++ b/includes/flag.pages.inc
@@ -24,46 +24,43 @@ function flag_page($action, $flag, $entity_id) {
   // If successful, return data according to the request type.
   if ($js) {
     drupal_add_http_header('Content-Type', 'text/javascript; charset=utf-8');
     $flag->link_type = 'toggle';
+    // Any errors that have been set will be output below the flag link with javascript.
     print drupal_json_encode(flag_build_javascript_info($flag, $entity_id));
     drupal_exit();

The $errors we've already accumulated aren't taken into account -- so for example, even if the token is invalid, the flagging works.

I guess that means we have to set the errors we have here in the flag object's array after all.

  else {
    $flag->flag($action, $entity_id);
  }

I would rather this got its own 'if (!$errors)' -- as it is, it's tacked on the end of a multiple if/else statement, when really this is an operation in its own right. Would also be nice to have a comment that states that this attempt to perform the flagging may itself set errors.

    // Any errors that have been set will be output below the flag link with javascript.

This comment needs to be wrapped.

(Also, removing the 'needs backport to D6'. I strongly doubt anyone is going to work on backporting this to D6!)

re-rolled.

re-rolled, should pass tests this time.

Looks good. Let's get this in!

I think we need a follow-on to look at CSS styling of the messages. Though having just tried both the before and after versions of error messages, I do think the new way with the message box below the flag is a big UI improvement over the previous JS alert box.

Another thing to consider is that if you implement hook_flag_validate(), and add fields to your flag, and set your flag to use the form link type, you have a potential UX problem:

1. user clicks link to flag a node
2. user is prompted to enter field data for the flagging entity they are about to create
3. user is then told they can't flag the node -- they are returned to the original page they were at in step 1, and the data they have just spent time entering is lost.

Now it may be the case that field values form part of the checks in hook_flag_validate(). So the ideal solution of telling the user in step 2 that they can't flag isn't always desirable (and also it would be a total pain and a huge re-engineering undertaking to implement, as we'd need a way to 'test' the node is flaggable without actually flagging it).

But we still have the problem that we have lost the user's data: if the field values are checked as part of hook_flag_validate(), then the user should be given a chance to change them.

What this boils down to is that the call to $flag->get_errors() in flag_confirm_submit() should be in a flag_confirm_validate() instead. But as I say, I'm happy for that to be a follow-up, as it doesn't affect core flag operation, only a particular use case when implementing hook_flag_validate().