Opened 7 weeks ago
Closed 10 days ago
#48022 closed task (blessed) (fixed)
Support "ugc" rel attribute value in comments
Reported by: | audrasjb | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 5.3 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Comments | Keywords: | has-patch has-unit-tests has-dev-note commit dev-reviewed |
Focuses: | Cc: | ||
PR Number: |
Description
As announced by Google few hours earlier, rel="ugc"
link attribute is now supported for user generated content like comments.
UGC stands for User Generated Content, and the ugc attribute value is recommended for links within user generated content, such as comments and forum posts.
Source: https://webmasters.googleblog.com/2019/09/evolving-nofollow-new-ways-to-identify.html
Let's explore a way to add this attribute value into comment links.
Attachments (10)
Change History (35)
#1
@
7 weeks ago
- Keywords dev-feedback has-patch needs-testing needs-unit-tests added; 2nd-opinion removed
- Milestone changed from Awaiting Review to 5.3
In 48022.1.diff
, fixing DocBlock @since value.
Milestoning the ticket to 5.3, since I think the implementation looks ok.
Indeed, let's first add the possibility to add that attribute value as we did with nofolllow attribute value.
We could look on real implementation in WordPress comments later if strongly needed.
Pinging @SergeyBiryukov for dev-feedback
.
#2
@
7 weeks ago
In general, we should be adding rel="nofollow ugc"
and not just rel="ugc"
. While Google now supports rel="ugc"
, other search engines only support rel="nofollow"
, so we should really combine them and deliver both.
Other than that the patch looks good @audrasjb but I'm wondering where you're hooking it to comment content? We also need to change the attributes on comment author links from nofollow
to nofollow ugc
.
#3
@
7 weeks ago
Good catch @joostdevalk, thanks. We should definitely use nofollow ugc
.
Thanks for the refresh @dkarfa but I think we'll encounter some edge cases:
- If the link is already containing
nofollow
tag, we want to replace that withnofollow ugc
- If the link is already containing
ugc
tag, we want to replace that withnofollow ugc
- If the link is already containing
ugc nofollow
tag, we want to replace that withnofollow ugc
I guess we would also need some unit tests.
Will try to work on these tomorrow :)
Cheers,
Jb
#4
@
6 weeks ago
- Keywords needs-dev-note added; dev-feedback needs-testing removed
I tested @dkarfa ’s patch and it looks totally fine to me.
Here is the edge cases I tested:
<a href="https://google.com">Nothing special</a> <a href="https://google.com" rel="nofollow">Rel nofollow</a> <a href="https://google.com" rel="ugc nofollow">Rel ugc</a> <a href="https://google.com" rel="sponsored nofollow">Rel sponsored</a> <a href="https://google.com" rel="nofollow ugc">Rel nofollow ugc</a> <a href="https://google.com" rel="nofollow sponsored ugc">Rel nofollow sponsored ugc</a> <a href="https://google.com" rel="nofollow ug">Rel nofollow ug</a> <a href="https://google.com" rel="nofollow ugc sponsored">Rel nofollow ugc sponsored</a> <a href="https://google.com" rel="ugc nofollow">Rel ugc nofollow</a> <a href="https://google.com" rel="external nofollow">Rel external</a> <a href="https://google.com" rel="stuffy nofollow">Other rel</a>
And here is the result with 48022.4.patch
:
<a href="https://google.com" rel="nofollow ugc">Nothing special</a> <a href="https://google.com" rel="nofollow ugc">Rel nofollow</a> <a href="https://google.com" rel="ugc nofollow">Rel ugc</a> <a href="https://google.com" rel="sponsored nofollow ugc">Rel sponsored</a> <a href="https://google.com" rel="nofollow ugc">Rel nofollow ugc</a> <a href="https://google.com" rel="nofollow sponsored ugc">Rel nofollow sponsored ugc</a> <a href="https://google.com" rel="nofollow ug ugc">Rel nofollow ug</a> <a href="https://google.com" rel="nofollow ugc sponsored">Rel nofollow ugc sponsored</a> <a href="https://google.com" rel="ugc nofollow">Rel ugc nofollow</a> <a href="https://google.com" rel="external nofollow ugc">Rel external</a> <a href="https://google.com" rel="stuffy nofollow ugc">Other rel</a>
👍
#7
@
6 weeks ago
48022.5.diff is a good start, though I'm not a fan of duplicating the entire wp_rel_nofollow_callback()
function just for a different attribute :)
- Could we introduce a helper function that both
wp_rel_nofollow_callback()
andwp_rel_ugc_callback()
could wrap? - Perhaps we could even just add
ugc
to the existingwp_rel_nofollow_callback()
function? Core only uses it for comments, though I guess plugins and themes could use it for other purposes, which would need to be explored.
#10
@
5 weeks ago
- Milestone changed from 5.4 to 5.3
- Type changed from enhancement to task (blessed)
This is a small but important patch that needs to make it into 5.3.
Going to ensure this gets into Beta 2.
#12
follow-up:
↓ 14
@
5 weeks ago
@SergeyBiryukov Great! thanks for the new patch and commit.
One small thought: shouldn't we add new unit tests (see 48022.5.diff) or edit existing unit tests to handle rel=ugc
attribute value?
#14
in reply to:
↑ 12
@
4 weeks ago
Replying to audrasjb:
One small thought: shouldn't we add new unit tests (see 48022.5.diff) or edit existing unit tests to handle
rel=ugc
attribute value?
[46349] did add the tests from 48022.5.diff: trunk/tests/phpunit/tests/formatting/WPRelUgc.php?rev=46349, let me know if I've missed something :)
Dev note draft: https://docs.google.com/document/d/1yWniQr5AbMLQjcvRm9dUW4ynKFqutN-jYfOg0e5CS3o/edit?usp=sharing
Thanks for the note! It looks good to me, except for this example:
Add external rel attribute value to a given link:
$link = '<a href="example.com">External link example</a>'; $external_link = wp_rel_callback( $link, 'external' ); echo $external_link;
wp_rel_callback()
is meant to be used as a callback for preg_replace_callback()
, just like wp_rel_nofollow_callback()
was used before, and won't be able to handle a full <a>
tag.
This would be the correct example:
$link = '<a href="example.com">External link example</a>'; $external_link = preg_replace_callback( '|<a (.+?)>|i', function( $matches ) { return wp_rel_callback( $matches, 'external' ); }, $link ); echo $external_link;
We could probably make it a bit easier by introducing a generic wp_rel()
or wp_rel_attr()
function, however that seems out of scope for this ticket.
#17
@
2 weeks ago
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening this to investigate some reports that ugc
is not being added correctly.
I have done some testing and can confirm what this person has reported.
This feature is only working when the link actually has an <a>
tag (like when a link is added when editing a comment. If a user pastes a URL link into the comment without surrounding <a>
tags, only nofollow
is added to the rel
attribute.
#18
follow-up:
↓ 20
@
2 weeks ago
- Keywords needs-testing added
Dug in a bit here.
wp_rel_ugc()
is attached to the pre_comment_content
filter which happens before the comment is saved. However, the function that makes plain text URLs clickable (make_clickable()
) is hooked onto comment_text
, which happens when the comment is output.
It looks like rel="nofollow"
is hardcoded within make_clickable()
. This could be fixed by simply adding ugc
to that (which 48022.2.diff does).
On first glance, make_clickable()
is only used for comment content in Core. However, I'm not sure how it is being used by plugins and themes in the wild.
This ticket was mentioned in Slack in #core by desrosj. View the logs.
2 weeks ago
#20
in reply to:
↑ 18
@
2 weeks ago
Replying to desrosj:
On first glance,
make_clickable()
is only used for comment content in Core. However, I'm not sure how it is being used by plugins and themes in the wild.
Thanks for the patch!
Looking at plugins and themes, there are 1292 and 216 matches, respectively, vs. 101 and 3 matches for wp_rel_nofollow()
, per results from comment:7.
At a glance, not all of the make_clickable()
matches need the rel="ugc"
attribute. If the content in question is not user-generated, it might not be appropriate.
What about just adding it for comments for now by checking if comment_text
is the current filter, and adding a make_clickable_rel
filter to customize this behavior? 48022.6.diff implements that.
This ticket was mentioned in Slack in #core by david.baumwald. View the logs.
2 weeks ago
#22
@
10 days ago
- Keywords commit dev-reviewed added; needs-testing removed
48022.6.diff looks good to me, @SergeyBiryukov! +1.
First step - adds needed functions in formatting.php