Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
A proposed API change
The stated purpose of FlaggingEvent::action is to distinguish between two subtypes :- flag or unflag
but the name of the FlagEvent::name already encompasses this distinction.
The FlagEvent::name is one of :-
FlagEvent::ENTITY_FLAGGED
FlagEvent::ENTITY_UNFLAGGED
FlagEvent::FLAG_DELETE
Here is a code snippet from an upcoming issue (#2467413: Refactor count API into FlagCountService) where the event subscriber registers
the appropriate function callback without using FlaggingEntity::action
+ public static function getSubscribedEvents() {
+ $events = array();
+ $events[FlagEvents::ENTITY_FLAGGED][] = array('incrementFlagCounts', -100);
+ $events[FlagEvents::ENTITY_UNFLAGGED][] = array('decrementFlagCounts', -100);
+ return $events;
Comment | File | Size | Author |
---|---|---|---|
#10 | action-2473423-10.patch | 1.82 KB | martin107 |
#3 | interdiff-2-3.txt | 950 bytes | martin107 |
#3 | action-2473423-3.patch | 1.81 KB | martin107 |
#2 | action-2473423-2.patch | 907 bytes | martin107 |
Comments
Comment #1
joachim CreditAttribution: joachim commentedAdding link to the mentioned issue.
Comment #2
martin107 CreditAttribution: martin107 commentedThe devil inside me is busy trying to create an attack that uses.
FlagEvent::ENTITY_FLAGGED with $action set to 'unflag'
or
FlagEvent::ENTITY_UNFLAGGED with $action set to 'flag'
but I am fairly sure that is imposible
Comment #3
martin107 CreditAttribution: martin107 commentedThis is better.
Comment #5
joachim CreditAttribution: joachim commentedThis seems reasonable to me, but at first glance, I don't see where an event class gets the FlagEvents::ENTITY_FLAGGED from.
Comment #6
martin107 CreditAttribution: martin107 commentedI will fix the patch tomorrow..
but for now can I answer the question by saying
$action and $name are always set with a line of the form
Comment #7
joachim CreditAttribution: joachim commentedWell, with this patch it would be:
$this->eventDispatcher->dispatch(FlagEvents::ENTITY_FLAGGED, new FlaggingEvent($flag, $entity));
without the $action param, surely?
What I meant though is how does an event subscriber discover it's being called for FlagEvents::ENTITY_FLAGGED?
Comment #8
martin107 CreditAttribution: martin107 commentedThe code below can be read as
It is the bindings that are used to determine context so the callback does not have to discover what subtype of event they are.
I have worked in environments where the coding standard explicitly bans if/switch statements for context determination within methods..
Say a function like incrementFlagCounts and pseudo code like...
An alternative way of expressing the same thing...
$action is redundant because the flag/unflag information can be retrieved from the name of the event
[ FlaggingEvent::getName() - well getName returns 3 possible strings 'flag.entity_flagged' / 'flag.entity_unflagged' / 'flag.flag_deleted' ]
getName is inherited from Symfony\Component\EventDispatcherEvent::getName()
and as you can see from the comments this function is deprecated in favour of the binding approach.
Language can be such a funny thing ... I hope this answers your question honestly [ if I comes across like the iron fist of Putin saying yesterday there are not russian troops in the Ukraine period.... then I have only embarrassed myself :) ]
Comment #9
joachim CreditAttribution: joachim commented> FlaggingEvent::getName()
Ah yes, that's what I was getting at.
Thanks!
Comment #10
martin107 CreditAttribution: martin107 commentedNeeded reroll.
From the issue summary I have removed the text "I will post an patch..."
Comment #11
joachim CreditAttribution: joachim commentedLooks good. Thanks!
Comment #13
martin107 CreditAttribution: martin107 commentedFlagSimpleTest is broken for me after pulling this change ...
There are been lots of changes to D8 Core today ... I am about to trigger a retest.
Comment #15
joachim CreditAttribution: joachim commentedYou can't retest it now I've committed it -- well, you can, but it'll keel over because the patch won't apply.
According to the branch test, the failure is at 'Link with label Flag this item not found.'. The test report unfortunately doesn't give the passing assertions, so we're lacking in context, but it does look like the test fails before any actual flagging is even attempted. So I think this issue is in the clear :)
Comment #16
socketwench CreditAttribution: socketwench commentedWow. How did I miss that?
Comment #18
joachim CreditAttribution: joachim commentedBad testbot, no biscuit.