WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 5 months ago

Last modified 4 months ago

#39157 closed defect (bug) (fixed)

Feed returns 404 when there are no posts

Reported by: pavelevap Owned by: johnbillion
Milestone: 4.7.3 Priority: high
Severity: normal Version: 4.7
Component: Feeds Keywords: has-patch has-unit-tests fixed-major
Focuses: Cc:

Description

Probably result of #30210.

In WP 4.6.1 website without posts returned empty feed.

In WP 4.7 there is only 404 response and message: ERROR: This is not a valid feed.

And another side effect: When using add_feed() to adding own function then it worked in WP 4.6.1 by default because $wp_query contains by default 10 latest posts and even if there were no posts, callback function was processed. But now there is check for $wp_query->have_posts() (see [38929]) and so after update to WP 4.7 all these functions stopped working :-( Probably some plugins will be affected by this behaviour...

Attachments (6)

39157.patch (3.5 KB) - added by stevenkword 8 months ago.
Adds WP_Query::is_home_feed()
39157.2.patch (3.5 KB) - added by stevenkword 8 months ago.
Updates doc
39157.3.patch (4.6 KB) - added by stevenkword 8 months ago.
Adds WP_Query::is_custom_feed()
39157.4.patch (7.7 KB) - added by stevenkword 7 months ago.
Adds unit tests
39157.diff (8.7 KB) - added by peterwilsoncc 7 months ago.
39157.2.diff (2.5 KB) - added by peterwilsoncc 7 months ago.

Download all attachments as: .zip

Change History (51)

#1 @stevenkword
8 months ago

Good catch! This might a little tricky to solve since the WP_Query::is_home() conditional is not set to true when viewing this feed. See #20899

Relates to #11053

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

@stevenkword
8 months ago

Adds WP_Query::is_home_feed()

@stevenkword
8 months ago

Updates doc

#2 @johnbillion
8 months ago

  • Keywords has-patch needs-testing needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.7.1

@stevenkword
8 months ago

Adds WP_Query::is_custom_feed()

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


8 months ago

#4 @Fencer04
8 months ago

I had this issue on one of my production sites where I added a custom feed via the functions.php file. I added the patch 3 and it resolved my issue.

#5 @swissspidy
8 months ago

This has been reported in the forums yesterday: https://wordpress.org/support/topic/error-this-feed-is-ivalid-after-update-wp-4-7/

The feed is generated using the Pods framework it seems, but the URL looks like it's on the home page.

#6 @alfredo49
8 months ago

i aplied 39157.3.patch , and my problem with custom post type feed is solved!!!

Last edited 8 months ago by alfredo49 (previous) (diff)

#7 @hlashbrooke
7 months ago

Just to add my voice in here - this issue is affecting all users with custom feeds. It is very far-reaching and 39157.3.patch fixes the issue. I'm not able to do the unit tests for this, but general testing shows it to work. Any plugin that relies on custom feeds (such as podcasting plugins and even sitemap generators it seems) is affected by this, so this patch needs to be included 4.7.1.

#8 @stevenkword
7 months ago

  • Priority changed from normal to high

#9 follow-up: @dd32
7 months ago

A self-contained plugin (or unit tests if possible) to reproduce the various angles of the issue would be appreciated here.

#10 in reply to: ↑ 9 @stevenkword
7 months ago

Replying to dd32:

A self-contained plugin (or unit tests if possible) to reproduce the various angles of the issue would be appreciated here.

I'm working on unit tests for testing the presence of a custom feed, which shouldn't be a problem. However, testing the negative case where there are no posts is a bit more work since the current tests for feeds populate those during setUp(). I'd like to break up these test files, but that will also make this ticket much harder to review. So with that in mind, I'm going to take a stab and doing this in two parts.

@stevenkword
7 months ago

Adds unit tests

#11 @stevenkword
7 months ago

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

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


7 months ago

#13 @alfredo49
7 months ago

This issue will be resolved in WP4.7.1?

#14 @marlloftus
7 months ago

Any update on how long before a fix will be made available?

#15 @stevenkword
7 months ago

@alfredo49 @marlloftus This is slated for 4.7.1 (the next release) pending a review from a Core Committer. However, if you are desperate for a fix, you can apply patch 39157.4.patch using this guide: https://make.wordpress.org/core/handbook/tutorials/working-with-patches/#applying-a-patch

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


7 months ago

#17 @rachelbaker
7 months ago

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

#18 follow-up: @marlloftus
7 months ago

Thanks for the update.

is there a rough time frame for the 4.7.1 release?

#19 in reply to: ↑ 18 @stevenkword
7 months ago

Replying to marlloftus:

Thanks for the update.

is there a rough time frame for the 4.7.1 release?

4.7.1 lands on Jan 10, but there is still a chance this patch may not be included in the final release. It is still included at this time with review happening in the next day or so. If all goes to plan we should know in the next few days. I'll keep this ticket up to date.

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

@peterwilsoncc
7 months ago

#20 follow-up: @peterwilsoncc
7 months ago

Looking at 39157.4.patch, I am not sure why the extra conditionals, is_home_feed() and is_custom_feed() are being introduced in this patch. I think new conditionals are best left to #20899.

There are alternatives available for checking the whether a feed should be served. That said, happy to be disagreed with.

Kindly ignore 39157.diff, I decided it was unrelated after uploading it.

#21 follow-up: @peterwilsoncc
7 months ago

  • Milestone changed from 4.7.1 to 4.7.2

Going to bump to the next point release. Too little time to patch and retest prior to the RC.

#22 in reply to: ↑ 21 @stevenkword
7 months ago

Replying to peterwilsoncc:

Going to bump to the next point release. Too little time to patch and retest prior to the RC.

I would like to make sure we are aware that the scope of the problem extends into custom feeds -- making custom feeds that do not render posts unusable (don't render at all as of 4.7) -- there have been complaints by plugin authors. The title of this ticket does not completely describe the scope of the issue.

Additionally, new conditionals were added to solve a long standing problem where is_home() and is_feed() are never simultaneously true. Rather than change the behavior expected by plugin and theme authors, a new set of conditionals appears to be the only way forward to distinguish home/custom feeds from just "any" feed. See this comment: https://core.trac.wordpress.org/ticket/20899#comment:19

Is there any way to keep this in the 4.7.1 milestone? What can I do to help?

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

#23 in reply to: ↑ 20 @stevenkword
7 months ago

Replying to peterwilsoncc:

Looking at 39157.4.patch, I am not sure why the extra conditionals, is_home_feed() and is_custom_feed() are being introduced in this patch. I think new conditionals are best left to #20899.

In 4.7 we killed all feeds that should not be rendered and missed the case for 'home feed on any site with zero posts' and for 'custom feeds' that don't rely on WP_Query. This is an attempt to bring those feeds back and fix the regression. Normally, I wouldn't be introducing new methods to WP_Query in a point release, but in this case, I think it's the right path forward and has the side effect of solving other problems moving forward.

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

#24 @dd32
7 months ago

If there's another committer who understands the implications of the above change, and can have it in before the deadline for 4.7.1, they can commit the fix.

I agree with @peterwilsoncc here and would have also punted this if I wasn't confident of it.

Additionally, new conditionals were added to solve a long standing problem

A targeted regression fix which doesn't introduce new things or fix existing bugs would have made this far easier to commit. In this case, that would've been to skip all the getter methods here and just check the properties directly.

This is the kind of thing that in the past I'd just reverted the original problem causing changeset and see a second run being taken at it. Reverting [38929] from 4.7.x is still an option.

#25 @swissspidy
7 months ago

Given the scope of the problem, it's best to fix it rather sooner than later. Either by reverting [38929] for 4.7.1 and fixing things in 4.8, or by making this a targeted regression fix as suggested by @dd32.

A patch without the getter methods seems doable, but it would make testing a bit more difficult. Also, since there seems to be a shift between minor and major releases, we're more likely to introduce new functions in point releases in the future.

I don't feel strongly about it either way, but a revert might be the safest bet if we can't get enough testing for this in time for 4.7.1.

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


7 months ago

#27 follow-up: @peterwilsoncc
7 months ago

Partial revert of [38929] in 39157.2.diff:

  • removes changes to src
  • does not revert the tests for valid feeds.

#28 in reply to: ↑ 27 @stevenkword
7 months ago

Replying to peterwilsoncc:

Partial revert of [38929] in 39157.2.diff:

  • removes changes to src
  • does not revert the tests for valid feeds.

This is a good revert! Leaving the tests in place, while preventing the regression seems reasonable to me. I would like to engage further in the discussion about the new conditionals, which I do believe are valuable, but I'm glad to see the issue addressed in 4.7.1. Thank you for your attention, @peterwilsoncc. You can expect a ping from me in the near future ;)

#29 @SergeyBiryukov
7 months ago

#39510 was marked as a duplicate.

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


6 months ago

#31 follow-up: @Fencer04
6 months ago

Was this included in the 4.7.1 update? I saw talk of a revert that should fix it but the site that I applied patch number 3 to is broken in the same way after the update to 4.7.1.

#32 @alfredo49
6 months ago

No, the path isnt included in WP 4.7.1, in my case , i updated my sites to WP 4.7.1 and custom post types feed are broken again (blank screen ERROR: THIS FEED IS INVALID), i had to reinstall WP 4.6

#33 @Fencer04
6 months ago

@alfredo49 I applied patch 3 again and it fixed the issue in my case.

#34 @alfredo49
6 months ago

@Fencer04 yep, also in my case, i hope the fix was included in WP4.7.2

#35 in reply to: ↑ 31 @peterwilsoncc
6 months ago

Replying to Fencer04:

Was this included in the 4.7.1 update? I saw talk of a revert that should fix it but the site that I applied patch number 3 to is broken in the same way after the update to 4.7.1.

It was not, there wasn't adequate time to test the new patch prior to the release.

In 4.7.2 I intent to partially revert [38929]. The tests for valid feeds will be retained everything else will be reverted, see 39157.2.diff. This is provided it tests well.

#36 @johnbillion
6 months ago

  • Owner changed from peterwilsoncc to johnbillion
  • Status changed from assigned to reviewing

#37 follow-up: @Fencer04
6 months ago

I don't mean to complain but this getting pushed back keeps breaking the mission critical part of one of my sites. Without the custom RSS feed I created iTunes, Stitcher, etc... can't find my new episodes.

I just wanted to put in that info hoping that a quick resolution can be done so that others don't have the same issue. In my opinion this should be treated as an issue breaking mission critical processes.

Do we know if any of these patches work with 4.7.2? I can check on it locally later but if someone knows it would be helpful.

Last edited 6 months ago by Fencer04 (previous) (diff)

#38 @swissspidy
6 months ago

4.7.2 turned out to be a security-only release with no other bug fixes, so this ticket was moved to 4.7.3 together with the others slated for the next maintenance release. I know it's unfortunate, but we're aware of the bugs' severity and every contributor here tries their best to properly fix it as soon as possible.

AFAIK the latest patches all still apply on 4.7.2.

#39 @peterwilsoncc
6 months ago

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

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.

#40 @peterwilsoncc
6 months ago

  • Keywords fixed-major added; needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backporting to 4.7 branch.

@johnbillion leaving this assigned to you to do the honours.

#41 in reply to: ↑ 37 @stevenkword
6 months ago

Replying to Fencer04:

Do we know if any of these patches work with 4.7.2? I can check on it locally later but if someone knows it would be helpful.

39157.4.patch still applies cleanly against 4.7.2 and should still resolve your problem until future solutions has been addressed. You should not have to do this more than once, and the code introduced in 4.7.0 will be partially reverted in 4.7.3. Therefore when you make that update, the problem should go away.

In the meantime, applying that patch should fix things.

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


6 months ago

#43 @psykro
5 months ago

If you are still struggling with this and need a temporary solution until this is merged into core, you can hook into the template_redirect action hook and override the default template_loader.php functionality with your own, to bypass the feed check. It's not a clean solution, but would work in a pinch.

#44 @dd32
5 months ago

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

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 volcommky. View the logs.


4 months ago

Note: See TracTickets for help on using tickets.