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;
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim’s picture

Issue summary: View changes

Adding link to the mentioned issue.

martin107’s picture

Assigned: martin107 » Unassigned
Status: Active » Needs review
FileSize
907 bytes

The 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

martin107’s picture

FileSize
1.81 KB
950 bytes

This is better.

Status: Needs review » Needs work

The last submitted patch, 3: action-2473423-3.patch, failed testing.

joachim’s picture

This seems reasonable to me, but at first glance, I don't see where an event class gets the FlagEvents::ENTITY_FLAGGED from.

martin107’s picture

I 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

$this->eventDispatcher->dispatch(FlagEvents::ENTITY_FLAGGED, new FlaggingEvent($flag, $entity, 'flag'));
joachim’s picture

Well, 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?

martin107’s picture

How does an event subscriber discover it's being called for FlagEvents::ENTITY_FLAGGED

The code below can be read as

Bind the callback to the corresponding event. Increment for flagging event, and decrement for unflagging events.

+  public static function getSubscribedEvents() {
+    $events = array();
+    $events[FlagEvents::ENTITY_FLAGGED][] = array('incrementFlagCounts', -100);
+    $events[FlagEvents::ENTITY_UNFLAGGED][] = array('decrementFlagCounts', -100);
+    return $events;

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...

switch($action){
  case::'flag'
      ... do flaggy things.
  case::'unflag'
     ..do unflaggy things.
  default
    break error unknown action.

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 :) ]

joachim’s picture

> FlaggingEvent::getName()

Ah yes, that's what I was getting at.

Thanks!

martin107’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.82 KB

Needed reroll.

From the issue summary I have removed the text "I will post an patch..."

joachim’s picture

Status: Needs review » Fixed

Looks good. Thanks!

  • joachim committed b9ad0af on 8.x-4.x authored by martin107
    Issue #2473423 by martin107: Removed redundant FlaggingEnity::action...
martin107’s picture

FlagSimpleTest 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.

Status: Fixed » Needs review

martin107 queued 10: action-2473423-10.patch for re-testing.

joachim’s picture

Status: Needs review » Fixed

You 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 :)

socketwench’s picture

Wow. How did I miss that?

Status: Fixed » Needs work

The last submitted patch, 10: action-2473423-10.patch, failed testing.

joachim’s picture

Status: Needs work » Fixed

Bad testbot, no biscuit.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.