public function hasActionAccess($action, AccountInterface $account = NULL) {
    if ($action === 'flag' || $action === 'unflag') {
      $account = $account ?: \Drupal::currentUser();
      return $account->hasPermission($action . ' ' . $this->id);
    }
    else {
      // @todo: Is this the correct response?
      return FALSE;
    }
  }

I'm pretty sure $action can ONLY be one of flag or unflag -- but I can't be entirely sure because the docs are broken: #2415413: errors in docs for hasActionAccess().

I'd say we either don't need the else clause, or we should throw an exception, and I'm leaning towards the former.

CommentFileSizeAuthor
#7 2415417.7.removeElseInHasActionAccess.patch449 bytessocketwench
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webflo’s picture

webflo CreditAttribution: webflo commented

You could throw an InvalidArgumentException or LogicException.

socketwench’s picture

socketwench CreditAttribution: socketwench commented

$action is a bit tricky. Some modules, like flag_friend, will use a different action (friend/unfriend). This was possible in 7.x-3.x, but it's an open question for 8.x-4.x. Modules like flag_friend would either implement event handling for the action, or provide their own link type -- which is a lot easier now.

socketwench’s picture

socketwench CreditAttribution: socketwench commented

So really the issue here is if hasActionAccess() is the right API or not. If we only allow flag and unflag, we should have canFlag() and canUnflag() instead. Then we'd need to change flag_entity_view() to use the method.

joachim’s picture

joachim CreditAttribution: joachim commented

> Some modules, like flag_friend, will use a different action (friend/unfriend)

That sounds... crazy :/
Also, I've had a quick look at the code for flag_friend, and I can't actually see any use of that.

socketwench’s picture

socketwench CreditAttribution: socketwench commented

Oh, right! I refactored FF so it doesn't do that any longer.

socketwench’s picture

socketwench CreditAttribution: socketwench commented

Created https://www.drupal.org/node/2417925

socketwench’s picture

socketwench CreditAttribution: socketwench commented
Status: Active » Needs review
FileSize
2415417.7.removeElseInHasActionAccess.patch449 bytes

Returning nothing would probably be the better course of action here.

joachim queued 7: 2415417.7.removeElseInHasActionAccess.patch for re-testing.

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Fixed

Committed! Thanks!

  • joachim committed c22cde5 on 8.x-4.x authored by socketwench 
    Issue #2415417 by socketwench: Removed unneeded else clause in...

Status: Fixed » Closed (fixed)

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