| Project: | Flag |
| Version: | 6.x-2.x-dev |
| Component: | Flag core |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
Right now we have a hook_flag_acees() hook that gives us functionality somewhat akin to that of Drupal's node access.
That feature isn't bad. However, it turns out that there are a lot of use-cases that this "node access" paradigm can't solve:
There are cases where we want to show the flag link, and, after clicking this link, if we determine that flagging (or unflagging) shouldn't be allowed, we want to print, in a graceful manner, an error message to the user.
This can be solved by introducing a new hook, hook_flag_validate(), that will be called by the $flag->flag() method.
Here are several open issues that can be solved with, or with the help of, this proposed feature:
#951260: Flag as Registration / Signup .. and beyond
#814960: limiting how many nodes/content types a user can flag.
#855880: How to limit number of votes per day?
#885568: lock node from further flagging (changes)
#918508: Filter by date flagged (but he also wants to restrict flagging)
#812372: +1 vote system question
#913612: Like and dislike?
#285196: create multiple flags?
#472764: Limit flaggable nodes using views
#678812: Load flag template even for users without access
(Some of these issues on first glance may seem irrelevant to this discussion; One has to read through them to see that they do.)
Implementation
- The $flag->flag() method will call this hook. Modules wishing to [conditionally] abort the flagging operation will implement this hook; They will return a message to show to the user in this case.
- We will show this error message to the user in the same manner we show the "flagging message". This is important: we won't use a crud alert() box for this but display this message in nice HTML.
Rationale
These two paradigms, the "node access" one and the validation one complement each other. Both paradigms are used in Drupal itself. (There's another rationale, related to #871064: Making flaggings fieldable, but I want to keep this introduction short.)
Comments
#1
I'll now be marking as "duplicate" many of the issues I linked to, because we can't give these people any other solution (the current hook_flag_access() isn't always applicable).
So if you see that most of the issues I listed are stricken out it doesn't mean they were solved: they are just linking here.
#2
+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.
#3
There's another advantage to this feature:
Implementing hook_flag_validate() will be considerably easier than implementing hook_flag_access(), for two reasons:
#4
@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.
#5
+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.
#6
I'm attaching the patch. It's a simple one (it's not very short, but it's not complex).
1. Example for implementing hook_flag_validate()
Let's begin with an example. Here's how to forbid a flagging operation:
<?php/**
* Implements hook_flag_validate().
*/
function MYMODULE_flag_validate($action, $flag, $content_id, $account) {
if ($flag->content_type == 'node') {
if ($content_id == 666 && $action == 'flag') {
return t("You're not allowed to flag evil nodes!");
}
}
}
?>
This hook is called using
module_invoke_all()so it's also possible to return an array of error messages. (One can use meaningful identifiers for the keys of this array.)2. Extension to the Flag API
When a flagging operation cannot be carried out, the
$flag->flag()method now returns a list of error messages. Among these are the errors returned byhook_flag_validate()implementations. These errors are eventually printed out for the user in the flag template. This list is returned via a passed-by-reference variable.This extension to the API is backward compatible. Old code will not break.
Comments:
- In modern code such errors are returned in an exception object, but raising exceptions will break existing code.
- The code
"function whatever(&$var = 'something')"is PHP 5 only, but we've already settled on moving to PHP 5.3. The JavaScript side
Previously we used a 'status' boolean field in the JSON structure to signal an error, which would be shown using alert(), but this is not needed anymore.
If custom JavaScript code wants to know whether there was an error, it can inspect the 'flagSuccess' field.
4. Our flag.tpl.php
The flag template hasn't really changed. The message text variable (
$message_text) is used on both success and error. To differentiate between these two cases we use CSS classes on the message container:$message_classes. This allows the themer to show errors in red and success notifications in green, for example.(Additionally, I've deprecated the
$last_actionvariable in favor of$status, which is easier to understand. (Why? I'll explain later, if asked.))5. The theme preprocess function
The code added here deals with building the
$message_classesvariable.6. Testing the patch
After applying the patch (but apply this one first) and clearing Drupal's cache, put the following code (after renaming "MYMOUDLE") in a custom module:
<?php/**
* Implements hook_flag_validate().
*/
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 t("You may not flag a node with the word 'days' in its title.");
}
}
}
?>
You may also want to add the following to your stylesheet:
/**
* We don't want success and failure to look the same.
*/
.flag-success-message {
background: #aaffaa; /* greenish */
}
.flag-failure-message {
background: #ffaaaa; /* redish */
}
(Why am I using the word "failure", instead of "error", in the CSS? "error" seemed too harsh to me. But I'll eventually change it to "error" for uniformity.)
7. Ideas for contrib modules (not for Flag core)
#7
yaz085 wrote:
You say "the link will then show again". No: the link will always show. It's just that an error message (e.g., "No more vacancies for this event") will be shown upon clicking.
(Though the link could be themed (or the link text be modified) to suggest full attendance. It's not hard. But let's not talk about this here: it'd only add noise to this thread.)
#8
Marking #974254: How to avoid flagging based on condition that a node with same term is flagged a duplicate of this issue.
#9
subscribing
#10
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. :)
#11
Yes, it's by design.
I felt the out-of-the-box behavior should not be to make it disappear after 3 seconds, because the failure message may require the user attention. But I made it possible for a themer to override this decision (e.g., by adding the "flag-auto-remove" CSS class in a theme preprocess function; Or, for example, by using our JavaScript API to show this message in a modal dialog box (I planned to write a mini-module to do this)).
Now,
We can start a deep philosophical discussion about this. We can divide ourselves into three camps: those that believe we should show the message for 5 seconds, those that believe in 9 seconds, and those that believe it should be configurable from Flag's UI. But let's not do this: The only question is whether a themer/programmer is given the tools to do whatever he wishes, and I think the answer is "Yes".
#12
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:
<?phpfunction 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.
#13
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!
#14
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?
#15
Rob, thanks for the review.
For hook_flag_validate() I followed the signature of hook_flag().
(Interesting. I'll later investigate why hook_flag_access() has a different signature.)
Thanks for paying attention to this. I don't yet have an opinion on what's the "best" thing to do. (But we can postpone the decision on this so this shouldn't block us from committing this feature.)
#16
Subscribe. Any idea as to whether we'd get this into D7 soon too?
#17
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
#18
Does anyone have a working patch for Beta6? The patch above seems to only work up to beta 4