WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 7 weeks ago

#30210 reopened defect (bug)

WordPress generates unnecessary RSS feeds

Reported by: sergej.mueller Owned by: johnbillion
Milestone: Future Release Priority: normal
Severity: normal Version: 4.0
Component: Feeds Keywords: has-patch has-unit-tests
Focuses: Cc:

Attachments (9)

30210.patch (465 bytes) - added by JRGould 2 years ago.
Error if $wp_query doesn't contain any posts
30210.unit-test.patch (953 bytes) - added by lyubomir_popov 20 months ago.
Unit test for the JRGould's patch
30210.2.patch (526 bytes) - added by stevenkword 20 months ago.
30210.3.patch (871 bytes) - added by stevenkword 20 months ago.
30210.4.patch (834 bytes) - added by stevenkword 20 months ago.
Adds support for custom post archives
30210.5.patch (1.0 KB) - added by stevenkword 19 months ago.
30210.unit-tests.patch (2.1 KB) - added by stevenkword 19 months ago.
30210.7.patch (9.6 KB) - added by stevenkword 9 months ago.
Updates tests to include coverage for valid feeds
30210.8.patch (8.5 KB) - added by stevenkword 8 months ago.
No more global $wp_query

Download all attachments as: .zip

Change History (46)

#1 @helen
3 years ago

I feel pretty sure this is a ticket somewhere already - have noticed it before, and coincidentally I talked to somebody about exactly this sometime this past week.

#2 @stevenkword
2 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Helen,

I've poked around trac a bit and haven't been able to find a duplicate ticket for this problem. I'm going to leave it open for now.

#3 @ocean90
2 years ago

#31916 was marked as a duplicate.

@JRGould
2 years ago

Error if $wp_query doesn't contain any posts

#4 @JRGould
2 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#5 @staze
2 years ago

Patch seems to fix bad urls (/foo/feed/), and things like /wp-content/feed/

Does not resolve /pagename/feed/ (where pagename is a page, rather than a page of posts). Not sure why, since I would think a page doesn't have posts. Guessing have_posts must return true for pages... ?

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

#6 @JRGould
2 years ago

@staze: WordPress provides a comments feed anywhere where there might be comments, so showing an empty feed for /pagename/feed/ where /pagename/ is a valid url seems to me like expected behavior since it's just showing that there no comments for that page.

#7 @staze
2 years ago

oh, that explains it. Wonder if it would be worth adjusting the patch to check if there are comments or not... probably not. Seems like the patch now reflects expected behavior.

Thanks! Seems to work then!

@lyubomir_popov
20 months ago

Unit test for the JRGould's patch

#8 @lyubomir_popov
20 months ago

Added a unit test for JRGould's patch.

#9 @johnbillion
20 months ago

  • Keywords needs-unit-tests added

Chatting with @lyubomir_popov at WordCamp Sofia contributor day, it look like this patch will cause empty but otherwise valid feeds to return a 404 when they shouldn't. For example, a category feed for a valid category that doesn't yet contain any posts.

Needs some more test coverage.

#10 @lyubomir_popov
20 months ago

With the latest version, when opening a 'category/NAME' it redirects you to the first post in the category. If 'feed/' is added (even if there are posts), it returns an empty RSS feed. When there are no posts in the category, it is not shown in the sidebar.
I'll add more tests for the 'do_feed()' function.

#11 follow-up: @stevenkword
20 months ago

I believe 30210.2.patch solves the problem, but the solution isn't perfect. Because of the way the WP_Query is parsed in $wp_query->parse_query(), both $wp_query->is_feed() and $wp_query->is_tax() can not simultaneously be true. Currently, is_tax() will return false for all feeds. This means in order to solve the case of empty but valid taxonomy archive feeds, we are not able to use the typical core functions. As a work around, I'm checking to see if $wp_query->tax_query is an instance of the WP_Tax_Query object.

The unit tests proposed in 30210.unit-test.patch will struggle with the same is_tax symptoms and will need to be updated if we decide 30210.2.patch is valid.

Last edited 20 months ago by stevenkword (previous) (diff)

#12 @stevenkword
20 months ago

  • Owner set to stevenkword
  • Status changed from new to assigned

#13 in reply to: ↑ 11 @stevenkword
20 months ago

Please disregard everything I've said in my previous comment and 30210.2.patch. I had completely misunderstood how is_tax() was working. I've submitted a new and improved patch, 30210.3.patch, that does the exact same thing, but is now more inline with the correct way of doing things. I think I'm happy with this, now.

@wonderboymusic -- Would you mind giving this a once over?

@stevenkword
20 months ago

Adds support for custom post archives

#14 @stevenkword
19 months ago

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

Took a look at this again over WCUS and think this is ready for commit. 30210.unit-tests.patch provides test coverage for the issue mentioned in this ticket (invalid feed endpoints), as well as adding test coverage for feed template loading.

The former is expected to fail in trunk (before applying 30210.5.patch), demonstrating the original bug report. After applying 30210.5.patch, both new tests should pass, demonstrating proper error handling in the event of invalid feed endpoints.

@johnbillion

#15 @johnbillion
19 months ago

  • Milestone changed from Future Release to 4.5
  • Owner changed from stevenkword to johnbillion
  • Status changed from assigned to reviewing

#16 @stevenkword
18 months ago

#13049 was marked as a duplicate.

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


18 months ago

#18 @johnbillion
18 months ago

  • Keywords commit removed

This also needs some tests to ensure that all valid feed URLs are served correctly (ie. we're not unintentionally blocking valid feeds). I started on this and then bumped into #35452. I'll work on it more later this week.

#19 @stevenkword
17 months ago

Until #35452 is resolved, I've added tests to verify the query argument versions function properly in 30210.6.tests.patch . Anything after &feed is useless, but having these tests is better than nothing and might help nudge this ticket along.

#20 @stevenkword
17 months ago

#13049 was marked as a duplicate.

#21 @stevenkword
16 months ago

I'm taking another look at this in hopes of finding an appropriate solution for validating expected feeds.

#22 @jorbin
16 months ago

  • Milestone changed from 4.5 to Future Release

Since the unit tests are blocked by #35452, I'm punting this from 4.5. I also punted the semi-related #31935.

#23 follow-up: @johnbillion
9 months ago

  • Keywords needs-unit-tests added; has-unit-tests removed
  • Milestone changed from Future Release to 4.7

As per [38654], the unit tests for this need to set up the permalink structure and then re-register the initial taxonomies in the correct order:

$this->set_permalink_structure( $foo );
create_initial_taxonomies();

@stevenkword Want to take another stab at this?

Last edited 9 months ago by johnbillion (previous) (diff)

#24 in reply to: ↑ 23 @stevenkword
9 months ago

Replying to johnbillion:

@stevenkword Want to take another stab at this?

Absolutely! Thanks for the bump!

@stevenkword
9 months ago

Updates tests to include coverage for valid feeds

#25 @stevenkword
9 months ago

30210.7.patch back to you for review @johnbillion

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


8 months ago

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


8 months ago

#28 @johnbillion
8 months ago

In 38924:

Feeds: Greatly reduce the number of dummy posts that are generated for the RSS2 tests. This speeds the tests up.

See #35160, #30210

@stevenkword
8 months ago

No more global $wp_query

#29 @stevenkword
8 months ago

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

#30 @johnbillion
8 months ago

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

In 38929:

Feeds: Don't attempt to generate RSS feeds for invalid feed URLs such as wp-content/feed.

Props stevenkword, JRGould, lyubomir_popov, johnbillion
Fixes #30210

#31 @peterwilsoncc
5 months ago

In 40030:

Feeds: Prevent empty feeds from returning 404 errors.

Partial revert of [38929].

This reverts the source code from the commit for #30210 to prevent valid but empty feeds from returning 404 file not found errors for the home and custom feeds.

The tests in the commit that remain applicable are retained.

Props pavelevap for reporting, stevenkword for initial patch.
See #30210.
Fixes #39157.

#32 @peterwilsoncc
5 months ago

  • Milestone changed from 4.7 to 4.8
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening following the commit above.

This was in 4.7 - 4.7.2 but will be reverted in 4.7.3 as it returns 404s for some valid feeds.

Putting on the 4.8 milestone for fine tuning the fix.

#33 @stevenkword
5 months ago

After the revert, 30210.8.patch should be combined with https://core.trac.wordpress.org/attachment/ticket/39157/39157.4.patch for a start on the post 4.7.3 solution.

#34 @dd32
4 months ago

In 40085:

Feeds: Prevent empty feeds from returning 404 errors.

Partial revert of [38929].

This reverts the source code from the commit for #30210 to prevent valid but empty feeds from returning 404 file not found errors for the home and custom feeds.

The tests in the commit that remain applicable are retained.

Props pavelevap for reporting, stevenkword for initial patch, peterwilsoncc.
See #30210.
Merges [40030] to the 4.7 branch.
Fixes #39157.

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


3 months ago

#36 @peterwilsoncc
7 weeks ago

I milestoned this for 4.8 after the partial revert, I'm inclined to bump it as there is no time to do properly before 4.8.

@johnbillion I'll leave it to you to make the final decision.

#37 @johnbillion
7 weeks ago

  • Milestone changed from 4.8 to Future Release
Note: See TracTickets for help on using tickets.