Opened 10 years ago
Last modified 9 months ago
#30210 assigned defect (bug)
WordPress generates unnecessary RSS feeds
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 4.0 |
Component: | Feeds | Keywords: | has-patch has-unit-tests |
Focuses: | Cc: |
Description
Attachments (9)
Change History (50)
#2
@
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.
#5
@
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... ?
#6
@
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
@
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!
#9
@
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
@
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.
#11
follow-up:
↓ 13
@
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.
#13
in reply to:
↑ 11
@
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?
#14
@
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
@
9 years ago
- Milestone changed from Future Release to 4.5
- Owner changed from stevenkword to johnbillion
- Status changed from assigned to reviewing
This ticket was mentioned in Slack in #core by stevenkword. View the logs.
9 years ago
#18
@
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
@
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.
#21
@
9 years ago
I'm taking another look at this in hopes of finding an appropriate solution for validating expected feeds.
#23
follow-up:
↓ 24
@
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?
#24
in reply to:
↑ 23
@
8 years ago
Replying to johnbillion:
@stevenkword Want to take another stab at this?
Absolutely! Thanks for the bump!
#25
@
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
#32
@
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
@
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.
This ticket was mentioned in Slack in #core by sergey. View the logs.
8 years ago
#36
@
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.
#38
follow-up:
↓ 39
@
4 years ago
- Owner johnbillion deleted
- Status changed from reopened to assigned
#39
in reply to:
↑ 38
@
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".
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.