WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 4 weeks ago

#46227 reopened enhancement

Add Rel-Feed Link to Header

Reported by: dshanske Owned by:
Milestone: 5.3 Priority: low
Severity: trivial Version:
Component: Feeds Keywords: has-patch has-unit-tests dev-feedback needs-testing 2nd-opinion
Focuses: Cc:

Description

When the front page is not the posts page, proposing that Core add a link in the header to the actual posts page, using the rel=feed designator.

https://blog.whatwg.org/feed-autodiscovery

Example

<link rel="feed" type="text/html" href="/posts" title="Posts Page" />

This would allow a system to figure out where the posts page is.

Attachments (8)

46227.diff (1.3 KB) - added by cameronamcintyre 8 weeks ago.
46227.2.diff (4.8 KB) - added by bogerekanzu 7 weeks ago.
Added the unit tests for the Add Rel link to feed in the header
46227.3.diff (4.5 KB) - added by garrett-eclipse 7 weeks ago.
Added @since to docblock and fixed the Unit Tests as they were failing.
46227.4.diff (3.1 KB) - added by chetan200891 7 weeks ago.
Refreshed patch.
46227.5.diff (1.3 KB) - added by adhitya03 5 weeks ago.
Move get_option( 'page_for_posts' ) into the "if"
46227.6.diff (1.3 KB) - added by adhitya03 5 weeks ago.
Correcting the indentation, sorry
46227.7.diff (1.3 KB) - added by adhitya03 5 weeks ago.
If users do not select any static page to display the posts page.
46227.8.diff (3.4 KB) - added by garrett-eclipse 4 weeks ago.
Reintroduce Unit Tests from 46227.4.diff that got lost. Note: These probably shouldn't live in document-title.php though.

Download all attachments as: .zip

Change History (27)

#1 @desrosj
2 months ago

  • Keywords good-first-bug 2nd-opinion added
  • Milestone changed from Awaiting Review to Future Release

Thanks for this, @dshanske.

This seems fine to add, but I'd like to do a bit more reading. Is there any more up to date documentation? Everything I am finding is really old.

I'll mark as good-first-bug.

#3 @cameronamcintyre
8 weeks ago

Added new action to wp_head that checks the current page status and adds the link element if required.

#4 @dshanske
8 weeks ago

  • Keywords has-patch added

@bogerekanzu
7 weeks ago

Added the unit tests for the Add Rel link to feed in the header

#5 @bogerekanzu
7 weeks ago

  • Keywords has-unit-tests dev-feedback added; 2nd-opinion removed
  • Resolution set to worksforme
  • Status changed from new to closed

#6 @dshanske
7 weeks ago

  • Milestone changed from Future Release to 5.2.3

Shouldn't be closed till it is merged.

#7 @dshanske
7 weeks ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

@garrett-eclipse
7 weeks ago

Added @since to docblock and fixed the Unit Tests as they were failing.

#8 @garrett-eclipse
7 weeks ago

  • Keywords needs-testing added; good-first-bug removed

Thanks for the patch @bogerekanzu I've uploaded 46227.3.diff to fix the Unit Tests as they were failing.

I also updated the docblock for rel_feed_for_posts to include the @since and a reference to wp_head.

In the Unit Tests the dom functions were failing and some CS issues addressed as well.

Please take a look and test.
Cheers

#9 @SergeyBiryukov
7 weeks ago

  • Keywords needs-refresh added

46227.3.diff has some unrelated changes from #44878, needs a refresh.

@chetan200891
7 weeks ago

Refreshed patch.

#10 @chetan200891
7 weeks ago

  • Keywords needs-refresh removed

#11 @audrasjb
5 weeks ago

Hey there, thanks for the patch :-)

Shouldn't we check if there is something returned by get_option( 'page_for_posts' ); before displaying any bit of HTML code?

@adhitya03
5 weeks ago

Move get_option( 'page_for_posts' ) into the "if"

@adhitya03
5 weeks ago

Correcting the indentation, sorry

#12 @adhitya03
5 weeks ago

move get_option( 'page_for_posts' ); into the condition where it is used and ensure that get_option( 'page_for_posts' ); will always return the ID of the static page because it will run only if the posts page use static page.

#13 @audrasjb
5 weeks ago

Hmm, we also need to check if a blog page has been defined. That was the idea of my previous comment.

@adhitya03
5 weeks ago

If users do not select any static page to display the posts page.

#14 @adhitya03
5 weeks ago

Hi @audrasjb

I am sorry if I hard to understand your point, I this is my first contribution to the core. I have updated the patch.

@garrett-eclipse
4 weeks ago

Reintroduce Unit Tests from 46227.4.diff that got lost. Note: These probably shouldn't live in document-title.php though.

#15 @garrett-eclipse
4 weeks ago

  • Keywords 2nd-opinion reporter-feedback added
  • Milestone changed from 5.2.3 to 5.3

Hello All, Thank you for the patches to date on this.

I took a moment to read through the reference material (https://blog.whatwg.org/feed-autodiscovery) and feel we're going in the wrong direction here.

The 'feed' relationship is to indicate that the reference document is a syndication feed NOT a page.
"we have introduced a new feed relationship which indicates that the referenced document is a syndication feed. This now allows you to link to several different feeds containing different content which are not necessarily alternate versions of the page."

In WP feed <link>'s are introduced in several places with the alternate relationship attribute in many cases the issue with them is that they aren't 'necessarily alternate representations of the page' which is where the feed relationship comes in as it 'allows you to link to several different feeds containing different content which are not necessarily alternate versions of the page.'

"The feed keyword can also be used in combination with alternate to say that it is specifically the feed for the current document."

So to my understanding the existing feed <link>s should be updated to use 'rel="feed"' with the exception of the blog page itself which should use 'rel="feed alternate"' as the feed is actually an alternate representation of the page.

The existing locations which should be looked at;
Ones I feel should just be 'rel="feed"' unless they reside on the blog page;

Ones I feel should be 'rel="feed alternate"';

In no way am I a RSS/Atom Guru so would love some insight from a more experienced dev, @dshanske any thoughts?
*I very well may be wrong in which case sorry for the scare.

As well this is an enhancement and not a bug so shouldn't be on a minor milestone as it's not a reversion introduced in 5.2 so moving into 5.3 for further discussion.

I apologize to those who already submitted a patch as I had done, I should have delved deeper initially.

*Note: All quoted text comes from the reference material - https://blog.whatwg.org/feed-autodiscovery

#16 @dshanske
4 weeks ago

  • Keywords reporter-feedback removed

Your statement is correct. However, I only scoped this ticket to adding the link for rel-feed when the front page of the site is not the posts page, as this adds a defined benefit we don't have now...allowing for feed discovery in a not unpopular case.

Suggesting the second issue...adding it to rel-alternate situations, should be a separate ticket.

#17 @garrett-eclipse
4 weeks ago

Thanks @dshanske I appreciate you feeding back there.

So I understand a bit more the issue here and make sure I'm on the same page...

In a default WP install with a Homepage, which isn't the posts page, that homepage already has a link to the feed - <link rel="alternate" type="application/rss+xml" title="Rel Feed » Feed" href="http://rel-feed.local/feed/">. This is a rel="alternate" link but should instead be updated to a rel="feed" throughout the site except when it's on the posts page where it should be a rel="feed alternate".

So this request would be to take this line - https://github.com/WordPress/WordPress/blob/master/wp-includes/general-template.php#L2835
And update it as follows;
echo '<link rel="feed' . ( is_home() ? ' alternate' : '' ) . '" type="' . feed_content_type() . '" title="' . esc_attr( sprintf( $args['feedtitle'], get_bloginfo( 'name' ), $args['separator'] ) ) . '" href="' . esc_url( get_feed_link() ) . "\" />\n";
*Which changes that feed link from alternate to feed for all pages and then appends alternate if it's on the actual blog page.

Am I correct in that assumption? Just want to be sure before heading down that path.
Thanks

#18 @dshanske
4 weeks ago

Actually no. I was proposing a link to the html feed page, not the rss one.

#19 @dshanske
4 weeks ago

You are correct about the application of rel-feed otherwise, by the way. Although my concern in removing the alternate is that it has been there for so long that it would break RSS readers expecting it.

Note: See TracTickets for help on using tickets.