Make WordPress Core

Opened 10 years ago

Last modified 9 months ago

#30210 assigned defect (bug)

WordPress generates unnecessary RSS feeds

Reported by: sergejmueller's profile sergej.mueller Owned by: pbearne's profile pbearne
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 10 years ago.
Error if $wp_query doesn't contain any posts
30210.unit-test.patch (953 bytes) - added by lyubomir_popov 9 years ago.
Unit test for the JRGould's patch
30210.2.patch (526 bytes) - added by stevenkword 9 years ago.
30210.3.patch (871 bytes) - added by stevenkword 9 years ago.
30210.4.patch (834 bytes) - added by stevenkword 9 years ago.
Adds support for custom post archives
30210.5.patch (1.0 KB) - added by stevenkword 9 years ago.
30210.unit-tests.patch (2.1 KB) - added by stevenkword 9 years ago.
30210.7.patch (9.6 KB) - added by stevenkword 8 years ago.
Updates tests to include coverage for valid feeds
30210.8.patch (8.5 KB) - added by stevenkword 8 years ago.
No more global $wp_query

Download all attachments as: .zip

Change History (50)

#1 @helen
10 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
10 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
10 years ago

#31916 was marked as a duplicate.

@JRGould
10 years ago

Error if $wp_query doesn't contain any posts

#4 @JRGould
10 years ago

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

#5 @staze
10 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 10 years ago by staze (previous) (diff)

#6 @JRGould
10 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
10 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
9 years ago

Unit test for the JRGould's patch

#8 @lyubomir_popov
9 years ago

Added a unit test for JRGould's patch.

#9 @johnbillion
9 years 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
9 years 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.

@stevenkword
9 years ago

#11 follow-up: @stevenkword
9 years 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 9 years ago by stevenkword (previous) (diff)

#12 @stevenkword
9 years ago

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

#13 in reply to: ↑ 11 @stevenkword
9 years 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
9 years ago

@stevenkword
9 years ago

Adds support for custom post archives

@stevenkword
9 years ago

#14 @stevenkword
9 years 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
9 years ago

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

#16 @stevenkword
9 years ago

#13049 was marked as a duplicate.

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


9 years ago

#18 @johnbillion
9 years 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
9 years 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
9 years ago

#13049 was marked as a duplicate.

#21 @stevenkword
9 years ago

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

#22 @jorbin
9 years 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
8 years 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 8 years ago by johnbillion (previous) (diff)

#24 in reply to: ↑ 23 @stevenkword
8 years ago

Replying to johnbillion:

@stevenkword Want to take another stab at this?

Absolutely! Thanks for the bump!

@stevenkword
8 years ago

Updates tests to include coverage for valid feeds

#25 @stevenkword
8 years ago

30210.7.patch back to you for review @johnbillion

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


8 years ago

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


8 years ago

#28 @johnbillion
8 years 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 years ago

No more global $wp_query

#29 @stevenkword
8 years ago

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

#30 @johnbillion
8 years 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
8 years 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
8 years 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
8 years 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
8 years 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.


8 years ago

#36 @peterwilsoncc
8 years 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
8 years ago

  • Milestone changed from 4.8 to Future Release

#38 follow-up: @johnbillion
4 years ago

  • Owner johnbillion deleted
  • Status changed from reopened to assigned

#39 in reply to: ↑ 38 @stevenkword
4 years ago

Replying to johnbillion: Were you triaging, or taking an interest in this ticket? Out of all the tickets open in this component, this is my passion ticket. Could a commiter please outline a path for adoption? I feel like this is a case of "great" getting in the way of "good" which is trying really hard to replace "bad".

#40 @johnbillion
4 years ago

I removed myself as the owner (for some reason Trac re-adds the assigned status when you do that to reopened tickets).

I'd like to get this in but I need to prioritise my efforts :-)

Do you want to refresh the patch?

#41 @pbearne
9 months ago

  • Owner set to pbearne

in #11585 we cache 404 return so that should fix that

Note: See TracTickets for help on using tickets.