Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#51189 closed defect (bug) (fixed)

comments_pagination_base missing in get_comment_reply_link() function

Reported by: mrpauloen's profile MrPauloEn Owned by: joedolson's profile joedolson
Milestone: 5.8 Priority: normal
Severity: normal Version: 5.5
Component: Comments Keywords: has-screenshots good-first-bug has-patch has-testing-info commit
Focuses: accessibility Cc:

Description

Hi guys.

I noticed that get_comment_reply_link() does not create a valid reply link when comments are paginated.

When I say: "not valid" I mean without comments_pagination_base part like this is in previous_comments_link or next_comments_link or get_comment_link function.

I reffer exactly to this line:
https://core.trac.wordpress.org/browser/tags/5.5/src/wp-includes/comment-template.php#L1738

This issue is hard to notice until javacsript is disabled or the comment-reply script is not loaded or works incorrectly.

Only then this link is an ordinary clickable link. If you click it, the page reloads as usually to show us such a reply form:

https://i.imgur.com/sDKsmNTg.png
with:

You reply to: Jane Doe:

Username is then linked to his comment. So when you click it, it should move you to this comment. It works correctly if you have only one comment page.

But…

It won't work, if you split comments into pages.
Option: page_comments is enabled an there are more than one comment page.

It won't move you, if comment is on another page.

Why?

Because there is comments_pagination_base missing in that url.

Consider this with an example.

Now my comment reply link is this:

http://example.com/about/page-with-comments/?replytocom=179#respond

And the comment has this link:

http://example.com/about/page-with-comments/comment-page-2/#comment-179

If I want to jump for a moment to this comment, I can't. It just produce this link and nothing happens:

http://example.com/about/page-with-comments/?replytocom=179#comment-179

But when I slightly modify it:

http://example.com/about/page-with-comments/comment-page-2/?replytocom=182#respond

So, the mentioned author anchor has this link:
http://example.com/about/page-with-comments/comment-page-2/?replytocom=179#comment-179

And so, the comment has this link:
http://example.com/about/page-with-comments/comment-page-2/#comment-179

It skip to this comment when I clik user name anchor and come back when I click Reply link, becaouse username anchor has comment pagination url part.

So whan I did.

I looked into get_comment_reply_link() function, I copied some of a piece of code and I made a new filter, hooked to comment_reply_link filter.

I just added this to url in add_query_arg function:

get_permalink( $post->ID ) . $wp_rewrite->comments_pagination_base . '-' . $page . '/'

Whole filter looks like this:

add_filter( 'comment_reply_link', 'the_bootstrap_blog__filter_comment_reply_link', 10, 4 );
function the_bootstrap_blog__filter_comment_reply_link( $link, $args, $comment, $post ){
	global $wp_rewrite;

	$page = get_page_of_comment( $comment->comment_ID );

// Copied from get_comment_reply_link() function
	if ( get_option( 'comment_registration' ) && ! is_user_logged_in() ) {
		$link = sprintf(
			'<a rel="nofollow" class="comment-reply-login" href="%s">%s</a>',
			esc_url( wp_login_url( get_permalink() ) ),
			$args['login_text']
		);
	} else {
		$data_attributes = array(
			'commentid'      => $comment->comment_ID,
			'postid'         => $post->ID,
			'belowelement'   => $args['add_below'] . '-' . $comment->comment_ID,
			'respondelement' => $args['respond_id'],
			'replyto'        => sprintf( $args['reply_to_text'], $comment->comment_author ),
		);

		$data_attribute_string = '';

		foreach ( $data_attributes as $name => $value ) {
			$data_attribute_string .= " data-${name}=\"" . esc_attr( $value ) . '"';
		}

		$data_attribute_string = trim( $data_attribute_string );

		$link = sprintf(
			"<a rel='nofollow' class='comment-reply-link' href='%s' %s aria-label='%s'>%s</a>",
			esc_url(
				add_query_arg(
					array(
						'replytocom'      => $comment->comment_ID,
						'unapproved'      => false,
						'moderation-hash' => false,
					),
					get_permalink( $post->ID ) . $wp_rewrite->comments_pagination_base . '-' . $page . '/'
				)
			) . '#' . $args['respond_id'],
			$data_attribute_string,
			esc_attr( sprintf( $args['reply_to_text'], $comment->comment_author ) ),
			$args['reply_text']
		);
	}
	 return $args['before'] . $link . $args['after'];
}

And it works fine.

I know that this is only for my testing purposes, but if I have to put the theme in the WordPress repository, it should be more like in this (get_comment_link()) function.

I use:

  • WordPress v5.5

I noticed this issue a long time ago, but somehow I ignored it, didn't know how to fix it.

I tested this on default theme (twentytwenty), with all plugins deactivated and on my theme with all plugins activated. In each cases is the same.

Only when I add that filter, thoses links works fine.

Attachments (2)

51189.diff (1.9 KB) - added by paaggeli 3 years ago.
51189.2.diff (976 bytes) - added by joedolson 3 years ago.
Updated patch - build comment link using existing comment link function

Download all attachments as: .zip

Change History (26)

This ticket was mentioned in Slack in #accessibility by azhiyadev. View the logs.


4 years ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


4 years ago

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


4 years ago

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


4 years ago

#5 @joedolson
4 years ago

  • Owner set to joedolson
  • Status changed from new to accepted

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


4 years ago

#7 @ryokuhi
4 years ago

  • Milestone changed from Awaiting Review to 5.8

The ticket was reviewed today during the accessibility team's bug-scrub. For the moment, we'll move the ticket to 5.8 to try to include it in next release and eventually punt it later.

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


4 years ago

#9 @ryokuhi
4 years ago

  • Keywords good-first-bug added

This ticket was discussed today during the accessibility team's bug-scrub.
Given it might be an easy fix, it can be a good first bug: if any new contributor wants to jump in, feel free to do it!

Last edited 4 years ago by ryokuhi (previous) (diff)

@paaggeli
3 years ago

#10 @paaggeli
3 years ago

  • Keywords has-patch added; needs-patch removed

In addition to @MrPauloEn's solution, I added an if condition to check if a user has enabled the 'Break comments into pages' option from the settings.

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


3 years ago

#12 @joedolson
3 years ago

How to test:
1) Turn on 'Break comments into pages' at Settings > Discussion
2) Add comments and set the comments per page to a number that will generate at least two pages of comments.
3) Disable comment-reply.js

<?php

function test_disable_comment_js(){
    wp_deregister_script( 'comment-reply' );
}
add_action( 'init', 'test_disable_comment_js' );

4) Go to page 2 of comments on any post with multiple pages of comments.
5) Click on 'Reply' to a comment on this page.
6) Click on the commenter's name to return to comment you're replying to.

Without patch: link points to an anchor on the current page, but the comment does not appear on this page.

With patch: link should point to an anchor on the correct page of comments.

Note: this is a case where the link should open in a new tab if the user has started writing their comment, as otherwise their comment text can be lost.

Last edited 3 years ago by joedolson (previous) (diff)

#13 @alexstine
3 years ago

I tested the patch on my local dev site and it's working fine.

Thanks.

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


3 years ago

#15 @Boniu91
3 years ago

  • Keywords has-testing-info added

This ticket was mentioned in Slack in #accessibility by chaion07. View the logs.


3 years ago

#17 @joedolson
3 years ago

  • Keywords commit added; needs-testing removed

#18 @joedolson
3 years ago

  • Type changed from enhancement to defect (bug)

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


3 years ago

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


3 years ago

@joedolson
3 years ago

Updated patch - build comment link using existing comment link function

#21 @engahmeds3ed
3 years ago

I can confirm the issue before applying the patch and confirm also that the patch solved this issue.

But I have a question here:

Do we need to change this link also for the first page or leave the first page reply link to go to the post itself?

as current code will make the link for first page go to different url but with the same content, is it bad for SEO?

For the post page (first comments page)

before patch
http://localhost:8889/hello-world/?replytocom=6#respond

after patch
http://localhost:8889/hello-world/comment-page-3/?replytocom=6#respond

and I think before patch, first page link is the correct one, what do you think?

#22 @joedolson
3 years ago

No, I don't think that's a concern. The link that get_comment_link() will produce for the comment is a more permanent link; you'll always be able to find it there barring changes to the pagination settings. The other link is not permanent, as adding additional comments could push it off the front page.

get_comment_link() is the correct way to produce a comment link, and I don't think we should attempt to determine whether a given comment should currently be on the front page.

The SEO concerns should be nominal, given that this link is overridden with JavaScript in normal usage & WordPress provides canonical URL references.

#23 @joedolson
3 years ago

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

In 51081:

Comments: Return valid comment reply link if comments are paginated.

Fix the link returned by get_comment_reply_link() so the link points to the correct page of comments when links are paginated. While this link is normally overridden by the comment-reply script, if that script is disabled, the link would point to a location that did not exist when comments were paginated.

props MrPauloEn, paaggeli, alexstine, engahmeds3ed.
Fixes #51189.

#24 @joedolson
3 years ago

In 51084:

Coding Standards: Extraneous white space at end of line.

Fixes a minor coding standards issue.

Follow up to [51082].

See #51189.

Note: See TracTickets for help on using tickets.