WordPress.org

Make WordPress Core

Opened 10 months ago

Closed 2 weeks ago

Last modified 2 weeks ago

#51189 closed defect (bug) (fixed)

comments_pagination_base missing in get_comment_reply_link() function

Reported by: MrPauloEn Owned by: 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 7 weeks ago.
51189.2.diff (976 bytes) - added by joedolson 3 weeks 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.


10 months ago

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


9 months ago

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


7 months ago

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


2 months ago

#5 @joedolson
2 months ago

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

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


2 months ago

#7 @ryokuhi
2 months 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.


8 weeks ago

#9 @ryokuhi
8 weeks 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 8 weeks ago by ryokuhi (previous) (diff)

@paaggeli
7 weeks ago

#10 @paaggeli
7 weeks 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.


7 weeks ago

#12 @joedolson
7 weeks 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 7 weeks ago by joedolson (previous) (diff)

#13 @alexstine
7 weeks 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.


6 weeks ago

#15 @Boniu91
5 weeks ago

  • Keywords has-testing-info added

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


5 weeks ago

#17 @joedolson
5 weeks ago

  • Keywords commit added; needs-testing removed

#18 @joedolson
4 weeks ago

  • Type changed from enhancement to defect (bug)

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


3 weeks ago

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


3 weeks ago

@joedolson
3 weeks ago

Updated patch - build comment link using existing comment link function

#21 @engahmeds3ed
3 weeks 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
2 weeks 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
2 weeks 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
2 weeks 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.