Make WordPress Core

Opened 8 years ago

Closed 3 years ago

Last modified 3 years ago

#37267 closed defect (bug) (fixed)

Don't output a cancel comment reply link if comments aren't threaded

Reported by: henrywright's profile henry.wright Owned by: hellofromtonya's profile hellofromTonya
Milestone: 5.9 Priority: normal
Severity: normal Version: 2.9
Component: Comments Keywords: has-patch has-screenshots has-unit-tests commit
Focuses: Cc:

Description

cancel_comment_reply_link() is used inside comment_form() to output a cancel comment reply link. If comments aren't threaded, the cancel comment reply link isn't needed. It currently appears in the HTML source regardless of the status of thread_comments.

Attachments (6)

37267.patch (729 bytes) - added by jignesh.nakrani 8 years ago.
@rachelbaker @henry.wright here is a patch to skip rendering cancel comment reply link if comments aren't threaded.
37267.diff (493 bytes) - added by henry.wright 8 years ago.
37267-1.patch (756 bytes) - added by jignesh.nakrani 4 years ago.
here is a patch to skip rendering cancel comment reply link if comments aren't threaded.
37267.2.diff (741 bytes) - added by audrasjb 3 years ago.
Comments: Don’t output the cancel comment link when thread_comments option is not enabled
Capture d’écran 2021-09-26 à 22.59.51.png (247.7 KB) - added by audrasjb 3 years ago.
Before patch: the link exists (I removed the display none CSS rule for the example)
Capture d’écran 2021-09-26 à 23.06.12.png (312.9 KB) - added by audrasjb 3 years ago.
After patch: the link is not generated at all when threaded_comments option is not enabled

Download all attachments as: .zip

Change History (26)

#1 @rachelbaker
8 years ago

  • Component changed from General to Comments
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Version changed from 4.5.3 to 2.9

@henry.wright Thanks for the report. While cancel_comment_reply_link() is always called, if you look at the code in get_cancel_comment_reply_link() the reply link is still output, but hidden unless the replytocom param is set:

$style = isset($_GET['replytocom']) ? '' : ' style="display:none;"';

If threaded comments aren't allowed, I don't see why we need to output anything. Would you be willing to submit a patch?

Introduced in [12810]

@jignesh.nakrani
8 years ago

@rachelbaker @henry.wright here is a patch to skip rendering cancel comment reply link if comments aren't threaded.

#2 @jignesh.nakrani
8 years ago

  • Keywords has-patch added; needs-patch removed

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


8 years ago

#4 @henry.wright
8 years ago

Thanks for the patch. Personally, I'd just bail from the function early if comments aren't threaded.

function get_cancel_comment_reply_link( $text = '' ) {
    if ( ! get_option( 'thread_comments' ) ) {
        return '';
    }
    // Rest of code.
Last edited 8 years ago by henry.wright (previous) (diff)

@henry.wright
8 years ago

#5 @henry.wright
8 years ago

37267.diff is an alternative to @jignesh.nakrani's patch.

37267.diff won't stop cancel_reply_before and cancel_reply_before output, but it will stop the actual cancel comment reply link output.

#6 @desrosj
5 years ago

  • Keywords needs-refresh added

The latest patch no longer applies to trunk.

@jignesh.nakrani
4 years ago

here is a patch to skip rendering cancel comment reply link if comments aren't threaded.

#7 @jignesh.nakrani
4 years ago

  • Keywords needs-refresh removed

37267.diff won't stop cancel_reply_before and cancel_reply_before output, but it will stop the actual cancel comment reply link output.

I think if cancel_reply_link is not being added then we don’t have to print cancel_reply_before and cancel_reply_after

@henrywright @desrosj

#8 @audrasjb
3 years ago

  • Keywords needs-refresh added

The patch doesn't apply cleanly against trunk anymore. Refreshing and testing right now.

@audrasjb
3 years ago

Comments: Don’t output the cancel comment link when thread_comments option is not enabled

@audrasjb
3 years ago

Before patch: the link exists (I removed the display none CSS rule for the example)

@audrasjb
3 years ago

After patch: the link is not generated at all when threaded_comments option is not enabled

#9 @audrasjb
3 years ago

  • Keywords has-screenshots added; needs-refresh removed
  • Milestone changed from Future Release to 5.9

37267.2.diff refreshes the previous patch against trunk.
It's tested and it work fine: see screenshots above.

Moving for 5.9 consideration.

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


3 years ago

#11 @audrasjb
3 years ago

  • Keywords commit added

Marking this for commit as per today's bug scrub

#12 @audrasjb
3 years ago

  • Keywords needs-unit-tests added; commit removed

Removing commit keyword as it looks like it needs some unit tests

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


3 years ago

This ticket was mentioned in PR #1892 on WordPress/wordpress-develop by hellofromtonya.


3 years ago
#14

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

Applies 37267.2.diff patch and adds tests.

Trac ticket: https://core.trac.wordpress.org/ticket/37267

#15 @hellofromTonya
3 years ago

  • Keywords commit added

37267.2.diff works as expected. PR 1892 adds unit tests to it.

Marking for commit.

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

#16 @hellofromTonya
3 years ago

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

Preparing commit.

#17 @hellofromTonya
3 years ago

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

In 52175:

Comments: Don't output "cancel comment reply link" if comments aren't threaded.

Though hidden via style="display:none;", if the comments aren't threaded, this commit doesn't output the cancel comment reply link (skips over that logic). Change in comment_form().

Adds tests.

Follow-up to [12810], [38959].

Props henrywright, jigneshnakrani, rachelbaker, desrosj, audrasjb, hellofromTonya.
Fixes #37267.

#19 @hellofromTonya
3 years ago

Thank you everyone for contributing! The fix has been committed and will ship with 5.9 Beta 1 tomorrow. Cheers

#20 @hellofromTonya
3 years ago

Note: Changeset [52176] was incorrectly noted as fixing this ticket when actually it fixes #32315. The changeset has been moved to the right ticket.

Note: See TracTickets for help on using tickets.