This issue is a follow up of this one #2475893 where I reported a bug after I tested the patch on a live site : if we make anonymous flagging possible we should add a 'nofollow' attribute to the flag links to prevent search bots from visiting the links and grow artificially the flagging counts.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cedric_a created an issue. See original summary.

cedric_a’s picture

This patch fixes the problem for Ajax links only.

cedric_a’s picture

Ok, I fightted my lazyness and found where to add nofollow to the normal link...

cedric_a’s picture

FileSize
1011 bytes

Sorry, a typo in the comment.

andypost’s picture

Status: Active » Needs review
FileSize
786 bytes
485 bytes

I think better to keep that for all flag links

+++ b/src/Plugin/ActionLink/AJAXactionLink.php
@@ -27,6 +27,7 @@ class AJAXactionLink extends Reload {
     $render = parent::buildLink($action, $flag, $entity);
...
+    $render['#attributes']['rel'] = 'nofollow';

having that set in parent is enough

martin107’s picture

I like the idea behind the issue, so +1 from me.

Please forgive me, I would like to take a different approach.

I want the themer to have control over the styling of the link.

I don't want them to need php knowledge to be able make any styling changes required.

andypost’s picture

The best is to have option in formatter to allow set this from UI
Themers should be happy to use add/remove Class as they wanna to

+++ b/templates/flag.html.twig
@@ -31,5 +31,5 @@ action_class
+<a{{ attributes }} rel="nofollow">{{ title }}</a>

this is wrong because some preprocess can attach rel attribute and you end up with 2 rel attributes

andypost’s picture

I mean this option should be default to enabled and configurable via UI

martin107’s picture

Thanks for #7, I agree, was wrong and that has just shown me that my work on another project is not as comprehensive as I would like. andypost++

While I am still thinking about this

I am looking at the other possible rel values. Here is a list:-

http://www.w3schools.com/tags/att_a_rel.asp

Here is the way to include multiple values.

https://dev.w3.org/html5/spec-preview/the-link-element.html#attr-link-rel

In the context of a flag link one or more of the following could be required :-

rel = "nofollow noreferrer noopener"

also the tag keyword is a possibility.

What I am suggesting is that if set from the UI then rel needs to be an admin controlled string.

joachim’s picture

> I mean this option should be default to enabled and configurable via UI

I don't think we need another UI option. What is the use case for not adding the nofollow to the links? Who would want bots to flag things?

#5 is the right approach. If someone does want to remove it, they can remove that from the attributes in the preprocessor, can't they?

However, should it in the base class? The field entry and confirm form links don't need it, as they lead you to forms. Though the bots can submit the forms too, I suppose. Hm, ignore, just thinking aloud :)

martin107’s picture

Regarding #10 I was 2 mins behind pressing the submit button

"I don't think we need another UI option"

I am just going to post this for historical purposes

this is what it might look like.

andypost’s picture

@joachim this mostly about semantics and seo-voodoo but without kidding it makes a lot of sense and #9 links explains why

Patch looks great - rtbc++

Also I think it needs to dig a11y and probably add some role attribute and maybe test

martin107’s picture

I will wait to hear what joachim thinks, before adding tests

Yep I needed to think about accessibility, and also as always as we are taking string input from a user ( admittedly one with admin access ) and changing a HTML element that will be seen by many - so I need to think more about data sanitization/security

joachim’s picture

> @joachim this mostly about semantics and seo-voodoo but without kidding it makes a lot of sense and #9 links explains why

I'm totally fine with more rel options being added for the benefit of robots and SEO.

I'm not ok with yet another setting being added when I've yet to see any use cases for why anyone would need anything other than a sane built-in default.

Design is about making decisions. Adding a setting is ducking out of doing that.

martin107’s picture

I'm totally fine with more rel options being added for the benefit of robots and SEO.

Ok lets discuss that elsewhere.

martin107’s picture

I would like to constructively discuss, the next iteration.

So taking a line from andypost's perspective....

We cannot anticipate what a pre-process function will want to do - So we cannot blindly set the entire rel attribute string to be "nofollow"

My proposal would be to ensure 'nofollow' is one of the elements in the space separated list of values that make up the rel attribute

twig has hasClass and addClass to make up an ensure like function that operates on class

does anyone, more familiar with twig, know a way of making up the ensure function for rel?

As my preference here is twig first php second

socketwench’s picture

As my preference here is twig first php second

+1. The PHP method adds a lot of overhead and complexity, and I don't see how the UI would ever be touched after initial setup. The template can be overridden for all flags, and even specific flags and flag/entity combinations. There's plenty of options to do this in the theme layer already.

Suggestion: Accept patch #6, and open a follow up issue to add a UI and see if that gets enough traction.

martin107’s picture

Suggestion: Accept patch #6, and open a follow up issue to add a UI and see if that gets enough traction.

I opened up a follow up in #15

socketwench’s picture

Status: Needs review » Reviewed & tested by the community

I opened up a follow up in #15

This is what I get for reading the issue queue top-down. :-D

Patch at #6 works for me, although there is a blank line deletion in ActionLinkTypeBase that appears unrelated.

joachim’s picture

Status: Reviewed & tested by the community » Needs work

Patch #6 is wrong for reasons given in #7...

socketwench’s picture

Right...right. Brain is still aching from a 20 hour day yesterday including a 4 hour immigration test.

This also adds a comment to the twig template to make it obvious why we need nofollow.

martin107’s picture

Status: Needs review » Reviewed & tested by the community

Thanks socketwench

the patch looks good - It implements the consensus position.

so +1 from me.

  • joachim committed 5b96626 on 8.x-4.x authored by cedric_a
    Issue #2832509 by cedric_a, martin107, socketwench: Added 'nofollow' on...
joachim’s picture

Status: Reviewed & tested by the community » Fixed

> This also adds a comment to the twig template to make it obvious why we need nofollow.

Nice touch :)

Committed.

cedric_a’s picture

Thank you for the interest and the work on this issue, expecially on the week end.

andypost’s picture

somehow strange that my patch #5 is lost
Anyway, thanx a lot for fixing

martin107’s picture

@andypost

Are you concerned about 'flag' overwriting the actions of another module's pre-process function - That wants to globally ensure that a particular rel value is set?

Or would you like to see a value controllable via the UI ?

Or are you suggesting that doing things in php is better than doing things in twig ?

Sorry I am just asking questions that are going to come up in the follow up issue.

Status: Fixed » Closed (fixed)

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