Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#24622 closed defect (bug) (fixed)

If-Modified-Since is wrong by comment feed

Reported by: sweetie089's profile sweetie089 Owned by: nacin's profile nacin
Milestone: 3.7 Priority: normal
Severity: normal Version: 1.5.1
Component: Feeds Keywords: has-patch 3.7-early needs-unit-tests
Focuses: Cc:


Processing of "If-Modified-Since" is wrong by comment feed.
If time newer than the last contribution time is specified, "304 Not Modified" will certainly return.

procedure for reproducing:


  • "304 Not Modified" returns.

Created patch uses the last comment time for a branch condition, when "comments-rss", and "comments-rss2" and "comments-atom" are included in "feed" of the URL parameter.

Although creation of the test code was also tried, it gave up for the following reason.

  • The exit function is performed within a function.
  • There is no how to acquire the contents set by header.


Attachments (2)

If-Modified-Since_Bug.patch (563 bytes) - added by sweetie089 11 years ago.
If-Modified-Since_Bug_rev2.patch (580 bytes) - added by sweetie089 11 years ago.

Download all attachments as: .zip

Change History (10)

#1 @SergeyBiryukov
11 years ago

Confirmed the bug.

We check for $this->query_vars['withcomments'] (tags/3.5.1/wp-includes/class-wp.php#L338), but it's only set for comment feeds if permalinks are enabled.

Last edited 11 years ago by SergeyBiryukov (previous) (diff)

#2 @sweetie089
11 years ago

Thank you for the check.

Indeed, it is normal if the permalink is effective.
Let's imitate a permalink and set "withcomments" by "parse_request".

Since the remade patch is attached, please use the favorite one.

#3 @SergeyBiryukov
11 years ago

Found a similar block in WP_Query::parse_query(): tags/3.5.1/wp-includes/query.php#L1597.

#4 @SergeyBiryukov
11 years ago

  • Keywords 3.7-early added
  • Milestone changed from Awaiting Review to Future Release
  • Version changed from trunk to 1.5.1

Introduced in [2384]. Related: [2627], [4483], [4934].

Last edited 11 years ago by SergeyBiryukov (previous) (diff)

#5 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

#6 @nacin
11 years ago

So it seems like that block from WP_Query actually needs to be moved entirely into the WP class. Is there even a reason to have that in WP_Query, other than for direct use of WP_Query for preparing a comments feed *without* using withcomments? (Fine with keeping it, just want to make sure we look at it from all angles.)

Because of the block in WP_Query, we should use the same logic in the WP class. If comments-*, strip it and set withcomments.

Is this easily unit testable? That would be nice.

#7 @nacin
11 years ago

  • Keywords needs-unit-tests added

Looking at this again, I actually think the logic in and location of the first patch was closer to ideal than the second patch.

WP_Query specifically sets withcomments to 1 if the feed query variable contains comments-, even if withoutcomments is set. It overrides withoutcomments, which is only consulted when withcomments is not specified and we are dealing with a singular item.

So, the logic needs to be moved up a bit, to ensure the behavior is the same.

I don't think there is any unit tests here; that would be good to have in the future.

#8 @nacin
11 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 25683:

Ensure wp::send_headers() detects a comments feed when permalinks are disabled and thus the withcomments QV is omitted. This fixes Last-Modified.

props sweetie089.
fixes #24622.

Note: See TracTickets for help on using tickets.