Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 16 months ago

#54703 closed enhancement (fixed)

Remove comment feed link if get_post_comments_feed_link() returns empty

Reported by: barryceelen's profile barryceelen Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.0 Priority: normal
Severity: normal Version:
Component: Feeds Keywords: has-patch needs-testing has-unit-tests
Focuses: template Cc:

Description

The feed_links_show_comments_feed filter allows you to remove the comment feed URL for a single post. Even with the feed_links_show_comments_feed filter returning false, core still adds a feed link (albeit with an empty href attribute) via the feed_links_extra() function.

https://github.com/WordPress/WordPress/blob/master/wp-includes/general-template.php#L3158

Change History (16)

#1 @audrasjb
2 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 6.0
  • Version 5.8.2 deleted

This ticket was mentioned in PR #2089 on WordPress/wordpress-develop by audrasjb.


2 years ago
#2

  • Keywords has-patch added; needs-patch removed

#3 @audrasjb
2 years ago

  • Keywords needs-testing added

#4 @costdev
2 years ago

I'm writing unit tests for feed_links_extra() to include testing against the issue(s) raised in this ticket.

The PR targets a post that has comments, but the current functionality is that a feed link is output regardless of the number of comments, provided that comments_open() or pings_open() pass.

The output of a feed link for a post that has no comments isn't the source of the issue(s) raised in this ticket. It may also break something out in the wild, although I'm not sure about this, it may need its own investigation.

Ultimately, if the feed_links_show_comments_feed filter returns false, it should not output the comments feed link, regardless of whether a post has comments or not.

This ticket appears to flag two issues:

  1. When feed_links_show_comments_feed returns false, the comment feed link is still output.
  2. When get_post_comments_feed_link() returns '', the comment feed link is still output.

This appears to resolve both issues and the unit tests pass:

<?php

// src/wp-includes/general-template.php:3156

/**
 * Issue #1
 *
 * This reuses the 'feed_links_show_comments_feed' filter.
 * Multiple use of a single filter in Core has precedent with 'link_category', for example.
 */
/** This filter is documented in wp-includes/general-template.php */
$show_comments_feed = apply_filters( 'feed_links_show_comments_feed', true );

if ( $show_comments_feed && ( comments_open() || pings_open() || $post->comment_count > 0 ) ) {
        $title = sprintf( $args['singletitle'], get_bloginfo( 'name' ), $args['separator'], the_title_attribute( array( 'echo' => false ) ) );

        /**
         * Issue #2
         *
         * This guards against an empty string from get_post_comments_feed_link().
         */
        $feed_link = get_post_comments_feed_link( $post->ID );
        if ( '' !== $feed_link ) {
                $href = $feed_link;
        }
}

@audrasjb Can you review the above snippet and if it looks good, consider putting it in the PR? (without the comments, of course).

Last edited 2 years ago by costdev (previous) (diff)

#5 @rachelbaker
2 years ago

@costdev Thanks for working on unit tests for feed_links_extra(). They will be a great addition to this ticket, or separately.

Is it possible for $href to be an empty string in the output of the other template uses of this function? (post type, taxonomy, author, etc). Trying to understand the implications of the empty string check in one condition of this MEATY function.

It feels off that we would duplicate the feed_links_show_comments_feed filter but NOT the feed_links_show_posts_feed filter. For posts post_type archives wouldn't the same issue be present?

Last edited 2 years ago by rachelbaker (previous) (diff)

#6 @costdev
2 years ago

@rachelbaker Excellent point.

Taking a look at the post type equivalent, it returns string|false. However, at the end, it returns the result of a filter which handles the link - therefore it's possible that this could be an empty string.

So, instead of the Issue #2 conditional above, we can instead change the conditional further down this function (L3201) from isset() to ! empty() to cover both undefined and empty values:

<?php

if ( isset( $title ) && isset( $href ) } {
    //...

to:

if ( ! empty( $title ) && ! empty( $href ) ) {

Since the $title can be set via $args, we should be checking that it isn't empty either. A custom title doesn't require any of the sprintf() tokens to be included in it.

Last edited 2 years ago by costdev (previous) (diff)

#7 @costdev
2 years ago

@rachelbaker It feels off that we would duplicate the feed_links_show_comments_feed filter but NOT the feed_links_show_posts_feed filter. For posts post_type archives wouldn't the same issue be present?

Yeah this makes sense. If either of those filters returns false, the feed link shouldn't be output.

So we'd be looking at something like this then:

<?php

/** This filter is documented in wp-includes/general-template.php */
$show_posts_feed = apply_filters( 'feed_links_show_posts_feed', true );

/** This filter is documented in wp-includes/general-template.php */
$show_comments_feed = apply_filters( 'feed_links_show_comments_feed', true );

if ( $show_posts_feed && $show_comments_feed && ( comments_open() || pings_open() || $post->comment_count > 0 ) ) {
    //...
 

#8 @costdev
2 years ago

For committers:

#54713, milestoned for 6.0, introduces unit tests for feed_links_extra().

I have followup unit tests to cover whatever solution we apply for this ticket, but just noting that 54713 should be reviewed/committed first, to introduce unit tests for this function.

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


2 years ago

#10 @Boniu91
2 years ago

@barryceelen Thank you for creating the ticket. Could you post the exact steps that should be done in order to reproduce the issue and confirm the patch?

#11 @peterwilsoncc
2 years ago

  • Keywords needs-refresh added

I think the PR will need a refresh, for much the same reason that @costdev identifies above.

I think the if condition ought to be changed to something like this:

<?php
if (
  ( comments_open() || pings_open() || $post->comment_count > 0 ) &&
  '' !== get_post_comments_feed_link( $post->ID ) 
) {
  // Display the feed.
}

This ticket was mentioned in PR #2539 on WordPress/wordpress-develop by costdev.


2 years ago
#12

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

feed_links_extra() outputs the comments feed link even if the feed_links_show_comments_feed filter returns false.

This PR:

  • Adds unit tests which test that feed_links_extra() respects the filter, including one that will demonstrate the issue.
  • Adds a fix for the issue.

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

peterwilsoncc commented on PR #2539:


2 years ago
#13

Are you able to do a test that the feed link isn't included with add_filter( 'post_comments_feed_link', '__return_empty_string' )

I think that's how folks are currently disabling links to the feed but the html element is still showing in the header.

#14 @peterwilsoncc
2 years ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

In 53125:

Feeds: Remove comment feed HTML headers when empty.

Remove the link[rel=alternate] element from the HTML header when the comment feeds are disabled. Previously the HTML element was output with an empty href attribute.

The element is removed if get_post_comments_feed_link() returns an empty string or the feed is disabled via the feed_links_show_comments_feed filter.

Props barryceelen, audrasjb, costdev, rachelbaker, Boniu91.
Fixes #54703.

Note: See TracTickets for help on using tickets.