WordPress.org

Make WordPress Core

Opened 10 months ago

Closed 7 months ago

#24622 closed defect (bug) (fixed)

If-Modified-Since is wrong by comment feed

Reported by: sweetie089 Owned by: 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:

Description

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:

Result:

  • "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.

sorry.

Attachments (2)

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

Download all attachments as: .zip

Change History (10)

comment:1 SergeyBiryukov10 months ago

Confirmed the bug. $this->query_vars['withcomments'] is only set for comment feeds if permalinks are enabled.

Version 0, edited 10 months ago by SergeyBiryukov (next)

comment:2 sweetie08910 months 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.

comment:3 SergeyBiryukov10 months ago

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

comment:4 SergeyBiryukov10 months 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 10 months ago by SergeyBiryukov (previous) (diff)

comment:5 wonderboymusic9 months ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

comment:6 nacin7 months 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.

comment:7 nacin7 months 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.

comment:8 nacin7 months 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.