Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#21 | add_nofollow_on_flag_links-2832509-21.patch | 452 bytes | socketwench |
#11 | uirel-2832509-10.patch | 3.26 KB | martin107 |
Comments
Comment #2
cedric_aThis patch fixes the problem for Ajax links only.
Comment #3
cedric_aOk, I fightted my lazyness and found where to add nofollow to the normal link...
Comment #4
cedric_aSorry, a typo in the comment.
Comment #5
andypostI think better to keep that for all flag links
having that set in parent is enough
Comment #6
martin107 CreditAttribution: martin107 as a volunteer commentedI 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.
Comment #7
andypostThe 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
this is wrong because some preprocess can attach rel attribute and you end up with 2 rel attributes
Comment #8
andypostI mean this option should be default to enabled and configurable via UI
Comment #9
martin107 CreditAttribution: martin107 as a volunteer commentedThanks 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.
Comment #10
joachim CreditAttribution: joachim commented> 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 :)
Comment #11
martin107 CreditAttribution: martin107 as a volunteer commentedRegarding #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.
Comment #12
andypost@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 testComment #13
martin107 CreditAttribution: martin107 as a volunteer commentedI 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
Comment #14
joachim CreditAttribution: joachim commented> @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.
Comment #15
martin107 CreditAttribution: martin107 as a volunteer commentedOk lets discuss that elsewhere.
Comment #16
martin107 CreditAttribution: martin107 as a volunteer commentedI 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
Comment #17
socketwench CreditAttribution: socketwench commented+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.
Comment #18
martin107 CreditAttribution: martin107 as a volunteer commentedI opened up a follow up in #15
Comment #19
socketwench CreditAttribution: socketwench commentedThis 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.
Comment #20
joachim CreditAttribution: joachim commentedPatch #6 is wrong for reasons given in #7...
Comment #21
socketwench CreditAttribution: socketwench commentedRight...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.
Comment #22
martin107 CreditAttribution: martin107 as a volunteer commentedThanks socketwench
the patch looks good - It implements the consensus position.
so +1 from me.
Comment #24
joachim CreditAttribution: joachim commented> This also adds a comment to the twig template to make it obvious why we need nofollow.
Nice touch :)
Committed.
Comment #25
cedric_aThank you for the interest and the work on this issue, expecially on the week end.
Comment #26
andypostsomehow strange that my patch #5 is lost
Anyway, thanx a lot for fixing
Comment #27
martin107 CreditAttribution: martin107 as a volunteer commented@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.