You shouldn't have to do this before rendering a link:

    $action = 'flag';
    if ($flag->isFlagged($entity)) {
      $action = 'unflag';
    }

The renderLink() method should figure this out for itself. There is no situation where you'd want to to choose whether you want a flag or unflag link based on any other criterion -- because the link wouldn't work.

CommentFileSizeAuthor
#8 2568135-8.flag_.get-link-action.patch4.81 KBjoachim
#5 2568135-5.flag_.get-link-action.patch4.81 KBjoachim
#2 renderLink-2568135-2.patch5.23 KBmartin107
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Status: Active » Needs review
FileSize
renderLink-2568135-2.patch5.23 KB

Here is the initial version

A natural consequence of this is that we can also remove the $action parameter from

AJAXLinkController::generateResponse()

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Postponed

This will probably need a reroll due to #2568129: Move lazy builder callback into a service..

Also, I figured #2488856: add a wrapper around buildLink() that checks access should probably get done first. I maybe linked these two issues the wrong way round...

And I have also been wondering whether we should retain the method that doesn't check anything -- just in case there are use cases I've not envisaged. So we'd have:

getLink(): main API: does all the things for you: checks access, figures out $action. Wraps around:
renderLink(), which lets you hang yourself if you really want to.

Possible use case: you want a token in an email that you're sending out, and you want it to be a specific action, which might not be the right one at the time you're sending the email.

joachim’s picture

joachim CreditAttribution: joachim commented

Hmm also renderLink() is probably not the right name -- if it returns a render array, it's a builder, not a renderer.

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Postponed » Needs review
FileSize
2568135-5.flag_.get-link-action.patch4.81 KB

Status: Needs review » Needs work

The last submitted patch, 5: 2568135-5.flag_.get-link-action.patch, failed testing.

The last submitted patch, 5: 2568135-5.flag_.get-link-action.patch, failed testing.

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs work » Needs review
FileSize
2568135-8.flag_.get-link-action.patch4.81 KB

Whoops.

The last submitted patch, 5: 2568135-5.flag_.get-link-action.patch, failed testing.

martin107’s picture

martin107 CreditAttribution: martin107 commented

Here is what I checked.

a) The idea behind the issue is sound.
b) The change has been made in all the appropriate places.
c) I cant see any negative consequences... down the line.

I have supplied patches here so this is just a +1 form me.

joachim’s picture

joachim CreditAttribution: joachim commented
Title: renderLink() should figure out $action for itself » getLink() should figure out $action for itself
Status: Needs review » Fixed

I think that's good enough! Thanks!

Committed.

  • joachim committed dfc3bad on 8.x-4.x 
    Issue #2568135 by joachim, martin107: Removed $action parameter from...
martin107’s picture

martin107 CreditAttribution: martin107 commented

sorry fixed a typo in #10

I CAN NOT see any negative consequences...

Status: Fixed » Closed (fixed)

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