Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#48022 closed task (blessed) (fixed)

Support "ugc" rel attribute value in comments

Reported by: audrasjb's profile audrasjb Owned by: sergeybiryukov's profile 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:

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)

48022.diff (2.2 KB) - added by audrasjb 5 years ago.
First step - adds needed functions in formatting.php
48022.1.diff (2.2 KB) - added by audrasjb 5 years ago.
Adds 5.3.0 @since information to the new functions DocBlocks
48022.2.patch (3.3 KB) - added by dkarfa 5 years ago.
48022.3.patch (3.3 KB) - added by dkarfa 5 years ago.
FInal fix.
48022.4.patch (3.3 KB) - added by dkarfa 5 years ago.
48022.5.diff (6.6 KB) - added by audrasjb 5 years ago.
Adding unit tests for rel gc
48022.2.diff (431 bytes) - added by desrosj 5 years ago.
48022.3.diff (21.2 KB) - added by desrosj 5 years ago.
Include test updates
48022.4.diff (21.4 KB) - added by desrosj 5 years ago.
Fix a few tests missed.
48022.6.diff (2.9 KB) - added by SergeyBiryukov 5 years ago.

Download all attachments as: .zip

Change History (35)

@audrasjb
5 years ago

First step - adds needed functions in formatting.php

@audrasjb
5 years ago

Adds 5.3.0 @since information to the new functions DocBlocks

#1 @audrasjb
5 years 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 @joostdevalk
5 years 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.

@dkarfa
5 years ago

@dkarfa
5 years ago

FInal fix.

@dkarfa
5 years ago

#3 @audrasjb
5 years 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 with nofollow ugc
  • If the link is already containing ugc tag, we want to replace that with nofollow ugc
  • If the link is already containing ugc nofollow tag, we want to replace that with nofollow ugc

I guess we would also need some unit tests.
Will try to work on these tomorrow :)

Cheers,
Jb

#4 @audrasjb
5 years 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>

👍

@audrasjb
5 years ago

Adding unit tests for rel gc

#5 @audrasjb
5 years ago

  • Keywords has-unit-tests dev-feedback added; needs-unit-tests removed

#6 @SergeyBiryukov
5 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#7 @SergeyBiryukov
5 years 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() and wp_rel_ugc_callback() could wrap?
  • Perhaps we could even just add ugc to the existing wp_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.

#8 @audrasjb
5 years ago

  • Milestone changed from 5.3 to 5.4

#9 @audrasjb
5 years ago

Punting to next major.
5.3 beta 1 is about to be released.

#10 @SergeyBiryukov
5 years 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.

#11 @SergeyBiryukov
5 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 46349:

Comments: Add rel="nofollow ugc" attribute to links in 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.

See https://webmasters.googleblog.com/2019/09/evolving-nofollow-new-ways-to-identify.html.

Props audrasjb, joostdevalk, dkarfa, SergeyBiryukov.
Fixes #48022.

#12 follow-up: @audrasjb
5 years 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 @SergeyBiryukov
5 years 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.

#16 @SergeyBiryukov
5 years ago

In 46396:

Comments: Remove a one-time variable in wp_rel_nofollow() and wp_rel_ugc().

See #48022.

#17 @desrosj
5 years 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.

@desrosj
5 years ago

@desrosj
5 years ago

Include test updates

#18 follow-up: @desrosj
5 years 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.

@desrosj
5 years ago

Fix a few tests missed.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


5 years ago

#20 in reply to: ↑ 18 @SergeyBiryukov
5 years 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.

Last edited 5 years ago by SergeyBiryukov (previous) (diff)

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


5 years ago

#22 @desrosj
5 years ago

  • Keywords commit dev-reviewed added; needs-testing removed

48022.6.diff looks good to me, @SergeyBiryukov! +1.

#23 @audrasjb
5 years ago

For what it worth, 48022.6.diff works fine on my side as well.

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


5 years ago

#25 @SergeyBiryukov
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 46564:

Comments: Add rel="nofollow ugc" attribute when converting plain URLs to <a> tags in comments via make_clickable().

Introduce make_clickable_rel filter for the rel value that is added to URL matches converted to links.

This is a follow-up to [46349], which added the rel="nofollow ugc" attribute to existing <a> tags in comments via wp_rel_ugc().

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.

See https://webmasters.googleblog.com/2019/09/evolving-nofollow-new-ways-to-identify.html.

Props blogginglife, SergeyBiryukov.
Reviewed by desrosj, audrasjb.
Fixes #48022.

Note: See TracTickets for help on using tickets.