Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#33338 closed Cleanup/optimization (fixed)

Document that @never_cache and add_never_cache_headers() set the Expires header.

Reported by: Andy Chosak Owned by: Marcelo Galigniana
Component: Core (Cache system) Version: 3.2
Severity: Normal Keywords: cache
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

The documentation for the @never_cache decorator says that it sets the Cache-Control header but neglects to mention that it also sets Expires to the current time (if not already set).

Looking at the git blame, the use of Expires for @never_cache seems to predate the more modern Cache-Control. Is Expires still needed/desired as part of @never_cache?

It also feels a little bit unexpected that if @never_cache is going to set Expires as part of its behavior that it would leave alone a pre-existing Expires header that was set to some far future value.

It would be good to add tests for this behavior to the existing tests, both that Expires gets set if not already set and that, if already set, it doesn't get modified, if that is the desired behavior.

Change History (10)

comment:1 by Mariusz Felisiak, 2 years ago

Easy pickings: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

Agreed, we should clarify that the Expires header is added if it isn't already set in add_never_cache_headers() and @never_cache docs (see related patch_response_headers() docs). Extra tests are also welcome (as always).

comment:2 by Mariusz Felisiak, 2 years ago

Summary: Formalize @never_cache setting of Expires headerDocument that @never_cache and add_never_cache_headers() set the Expires header.

comment:3 by Marcelo Galigniana, 2 years ago

Owner: changed from nobody to Marcelo Galigniana
Status: newassigned

comment:4 by Marcelo Galigniana, 2 years ago

Has patch: set

comment:5 by Mariusz Felisiak, 2 years ago

Patch needs improvement: set

comment:7 by Mariusz Felisiak, 2 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In e61abab:

Refs #33338 -- Added never_cache() tests for Expires header.

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In 669dcefc:

Fixed #33338 -- Doc'd that never_cache() decorator set Expires header.

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In cc5bbd44:

[4.0.x] Fixed #33338 -- Doc'd that never_cache() decorator set Expires header.

Backport of 669dcefc04837c35fc2ec5ce906d84397005965d from main

Note: See TracTickets for help on using tickets.
Back to Top