#148 — duplicate accept-encoding in vary header

by Manuel Amador (Rudd-O) — last modified Jul 12, 2009 07:33 AM
State Tested and confirmed closed
Version: 1.2
Area Functionality
Issue type Patch
Severity Medium
Submitted by Manuel Amador (Rudd-O)
Submitted on Jan 23, 2009
Responsible Ricardo Newbery
Target release: 1.2.1
Return to tracker
This is not a bug in CacheFu but a bug in the ZPublisher. You are adding an accept-encoding header if the configuration in CacheFu has it, so the header should be:

Vary: accept-language, accept-encoding

Instead, for some reason, the header is:

Vary: accept-language, accept-encoding
  [TAB] accept-encoding

Accept-encoding is there TWICE.

Hereby I attach a patch for ZPublisher, I dunno if newer Zope versions have this problem.

Tested it with Plone 3.1.7 and the latest compatible Zope.
Attached:
cmf-accept-encoding.patch — differences between files, 758 bytes
Added by Manuel Amador (Rudd-O) on Jan 23, 2009 11:47 AM
the patch basically what it does is, if it detects that accept-encoding is already in the vary header, then just sends the original vary header. in principle, however, the addition of what ZPublisher does is WRONG, because Vary is a comma-separated list, not a newline-separated one.
Added by Manuel Amador (Rudd-O) on Jan 23, 2009 11:55 AM
btw the patch I gave fixes the issue, and should be sent upstream (I haev no idea where to send it). BUT, in the light that ZPublisher already adds the header if the content is compressed, there shall be no need to demand the user to set that up in the Vary header, don't you think? However, even if this is the case, the \n bug should really be fixed in ZPublisher to add comma-separated values instead (not that I don't understand why the appendHeader function operates the way it does, I do, but hey, fix).
Added by Ricardo Newbery on Mar 06, 2009 06:10 PM
Issue state: Unconfirmed → Resolved
Responsible manager: (UNASSIGNED) → newbery
Target release: None → 1.2.1
Please feel free to submit your patch to the Zope tracker (https://bugs.launchpad.net/zope/)

In the meantime, I agree we probably don't need to add this header in CacheSetup as ZPublisher takes care of it for us. But to avoid user confusion, we'll still *pretend* that we do... http://dev.plone.org/collective/changeset/81782 ;-)

Added by Manuel Amador (Rudd-O) on Jul 12, 2009 07:33 AM
Issue state: Resolved → Tested and confirmed closed
It appears that Zope 2.10.8 includes my patch already! I dunno how they found out, but it's my patch to the *letter*.

Excellent!

No responses can be added.