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

Disable auto cache control of the Symfony SessionListener #1033

Merged
merged 1 commit into from Dec 2, 2019

Conversation

@Toflar
Copy link
Member

Toflar commented Nov 29, 2019

Fixes #1028.

In fact, we don't need the default behaviour of Symfony because we qualify for "advanced usage" and our listener already correctly handles making responses private if needed.
I thought we need session stacking but that's not required because the SessionListener of SF correctly closes the session during kernel.finish_request which will cause our listener to correctly determine that the session was not started yet during the next kernel.response.

@Toflar Toflar self-assigned this Nov 29, 2019
@Toflar Toflar added the defect label Nov 29, 2019
@Toflar Toflar requested a review from aschempp Nov 29, 2019
@Toflar Toflar added this to the 4.9 milestone Nov 29, 2019
@leofeyer leofeyer merged commit d7facc7 into master Dec 2, 2019
9 checks passed
9 checks passed
Coverage
Details
Coding Style
Details
PHP 7.2
Details
PHP 7.3
Details
PHP 7.4
Details
Prefer Lowest
Details
Bundles
Details
Windows
Details
codecov/project 89.07% (+<.01%) compared to 1a0bfef
Details
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Dec 2, 2019

Thank you @Toflar.

@leofeyer leofeyer deleted the fix-caching-sf-4-4 branch Dec 2, 2019
@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Dec 2, 2019

Should we do this for everything? Or should we only do that for Contao requests? Also, I realize that this listener is in the contao-core bundle, so this is also applied to any regular Symfony application, not sure we can assume anything about caching and cookie whitelists there?

@Toflar

This comment has been minimized.

Copy link
Member Author

Toflar commented Dec 2, 2019

I think it's not wrong in the core-bundle but it should probably check for isFrontendMasterRequest() or what did you mean?

leofeyer added a commit that referenced this pull request Dec 3, 2019
…iable (see #1034)

Description
-----------

While debugging #1033 I noticed that we cannot configure the trace level of `HttpCache`. It absolutely makes no sense to bind that to `$this->isDebug` because in our case, this is always the case when we are in `dev` mode which again disables the HttpCache completely.

So here we go, by setting `TRACE_LEVEL=full` the `Contao-Cache` header gives you a lot more insight into the fragment requests.

Commits
-------

64fe068 Allow to configure the HttpCache trace level using an environment variable
leofeyer added a commit to contao/manager-bundle that referenced this pull request Dec 3, 2019
…iable (see #1034)

Description
-----------

While debugging contao/contao#1033 I noticed that we cannot configure the trace level of `HttpCache`. It absolutely makes no sense to bind that to `$this->isDebug` because in our case, this is always the case when we are in `dev` mode which again disables the HttpCache completely.

So here we go, by setting `TRACE_LEVEL=full` the `Contao-Cache` header gives you a lot more insight into the fragment requests.

Commits
-------

64fe0685 Allow to configure the HttpCache trace level using an environment variable
@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Dec 9, 2019

I think it's not wrong in the core-bundle but it should probably check for isFrontendMasterRequest() or what did you mean?

yes thats what I meant

@Toflar

This comment has been minimized.

Copy link
Member Author

Toflar commented Dec 9, 2019

Add an issue then so we don't forget about it.

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Dec 10, 2019

See #1079

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.