Closed (fixed)
Project:
Flag
Version:
8.x-4.x-dev
Component:
Flag core
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
7 Jun 2016 at 18:14 UTC
Updated:
14 Jul 2016 at 16:24 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
socketwench commentedIt's not hard to add this hook to FlagService alone, but the hook should also be called in the case when someone tries to flag/unflag by working with the entity classes alone.
Comment #3
socketwench commentedComment #4
socketwench commentedWhitespace fix.
Comment #5
joachim commentedI'm not sure on the difference between hook_flag_validate() and the flag access system... might be worth looking at the git log and the issues that added these hooks.
Comment #6
socketwench commentedComment #7
socketwench commentedYeah, it *is* similar, but the difference is when the hook is invoked. An access hook would control even the display of the flag link, where as validate only allows failure *on action*.
Comment #8
joachim commentedYeah... showing someone a flag link, only to say 'nananana fooled you!' when they click it seems wrong to me.
Hmmm though #952114: Have hook_flag_validate() claims lots of use cases for precisely that.
Comment #9
socketwench commentedIt really depends on what the 3rd party module does when validation fails. Instead of just kicking you back to the destination page, they could redirect to another page -- like user/event registration. It still sounds valid point to alter the workflow, but how it's used is important.
Comment #10
joachim commented> It really depends on what the 3rd party module does when validation fails. Instead of just kicking you back to the destination page, they could redirect to another page -- like user/event registration.
That to me is an abuse of a validation/access system. If you want to redirect users after they flag something, then an access system is not the right place to act. (Also, I think registration is pushing the limits of what Flag is meant to do.)
I've had a read through some of the issues cited on #952114: Have hook_flag_validate() as use cases, and honestly, I don't see why this can't be done with the access system. The one proviso is that we need to check access both when displaying the link AND when the link is actually used, in case something about the user/flag/flaggable/counts has changed during the intervening time.
The other thing cited on that issue is being able to view a flag link, but not use it. That's covered by #1857300: permission to view a flag's status, but not use it, but that issue is now 4 years old, and while there's the odd new comment now and again from another person interested in the feature, nobody's cared enough about it to do any work.
Comment #11
socketwench commentedI think the problem with #1857300: permission to view a flag's status, but not use it is that the requirements are kinda vague. If it were "view a flag but not use it" that is much clearer. It suggests defining a new view permission. Not hard to do; I'm tempted to go make that issue now.
Comment #12
socketwench commentedAfter discussion on IRC, it seems the best course of action is just to remove the hook, as this duplicates access checks expected to be implemented in #2584647: Flagging access system not extensible.
Comment #13
socketwench commentedReroll.
Comment #15
socketwench commentedRetesting now that HEAD is fixed.
Comment #17
socketwench commented