Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prioritize headers set by the Response class #9235

Open
wants to merge 4 commits into
base: 4.6
Choose a base branch
from

Conversation

michalsn
Copy link
Member

Description
This PR changes the way headers are set. Now headers set by the Response class will be prioritized and will replace those set by calling the header() function.

Why is this important? Without this change, we cannot override the headers set previously with the header() function.

This is relevant, especially when we work with a session. By default, session.cache_limiter is set to nocache, which is fine for the default setting and will automatically set headers:

Expires: Thu, 19 Nov 1981 08:52:00 GMT
Cache-Control: no-store, no-cache, must-revalidate
Pragma: no-cache

But we have no option to change these headers, even when we set different Cache-Control etc with the Response class. If we do so, we end up with two entries for Cache-Control, which will possibly lead to unexpected behavior. We also cannot remove the default headers set by session.cache_limiter, because they are set with the header() function directly.

Headers set with the Response class should be prioritized. This is potentially a BC break, but also a bugfix.

Ref: #9234

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@michalsn michalsn added bug Verified issues on the current code behavior or pull requests that will fix them breaking change Pull requests that may break existing functionalities 4.6 labels Oct 25, 2024
Copy link
Contributor

@neznaika0 neznaika0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. You've relieved the pain. I fixed this session_cache_limiter("); in app/Common.php


In previous versions, headers set by the ``Response`` class were added to existing
ones - giving no options to change them. That could lead to unexpected behavior when
the same headers were set with mutually exclusive directives.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on this "mutually exclusive headers"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some additional explanations below, in the changelog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.6 breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants