WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 4 weeks 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)

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

Download all attachments as: .zip

Change History (35)

@audrasjb
2 months ago

First step - adds needed functions in formatting.php

@audrasjb
2 months ago

Adds 5.3.0 @since information to the new functions DocBlocks

#1 @audrasjb
2 months 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
2 months 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
2 months ago

@dkarfa
2 months ago

FInal fix.

@dkarfa
2 months ago

#3 @audrasjb
2 months 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
2 months 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
2 months ago

Adding unit tests for rel gc

#5 @audrasjb
2 months ago

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

#6 @SergeyBiryukov
2 months ago

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

#7 @SergeyBiryukov
2 months 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
2 months ago

  • Milestone changed from 5.3 to 5.4

#9 @audrasjb
2 months ago

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

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

#11 @SergeyBiryukov
7 weeks 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
7 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 @SergeyBiryukov
7 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.

#16 @SergeyBiryukov
7 weeks ago

In 46396:

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

See #48022.

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

@desrosj
5 weeks ago

@desrosj
5 weeks ago

Include test updates

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

@desrosj
5 weeks ago

Fix a few tests missed.

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


5 weeks ago

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

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

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


5 weeks ago

#22 @desrosj
4 weeks ago

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

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

#23 @audrasjb
4 weeks 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.


4 weeks ago

#25 @SergeyBiryukov
4 weeks 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.