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

WIP: Fix usage of symfony cache with shared max age 0 #4385

Conversation

alexander-schranz
Copy link
Member

@alexander-schranz alexander-schranz commented Jan 23, 2019

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
License MIT

What's in this PR?

Fix usage of symfony cache with shared max age 0.

Why?

Symfony will not cache if shared max age is set and 0 because of $response->getMaxAge() in $response->isCacheable() will return at first s-max-age.

Example Usage

sulu_http_cache:
    handlers:
        public:
            max_age: 100 
            shared_max_age: 0

@alexander-schranz alexander-schranz force-pushed the bugfix/symfony-cache-shared-max-age-0 branch 2 times, most recently from e2fda60 to 19550de Compare January 23, 2019 15:45
@alexander-schranz alexander-schranz added the Bug Error or unexpected behavior of already existing functionality label Jan 24, 2019
@alexander-schranz alexander-schranz added this to the Release 1.6 milestone Jan 24, 2019
@alexander-schranz alexander-schranz removed the request for review from wachterjohannes January 28, 2019 14:37
@danrot
Copy link
Contributor

danrot commented Feb 4, 2019

I would move that logic into the HttpCache component from Symfony, because I would like to have the s-maxage header set to whatever is set in the config for external browsers/proxies. Therefore the validate method of the HttpCache component could be overridden, which will store the response in case the default implementation didn't. That means we would have to introduce another check in the validate method.

@alexander-schranz alexander-schranz force-pushed the bugfix/symfony-cache-shared-max-age-0 branch from 19550de to 1f3a510 Compare February 4, 2019 21:15
@alexander-schranz alexander-schranz force-pushed the bugfix/symfony-cache-shared-max-age-0 branch from 1f3a510 to 1e7fdd1 Compare February 4, 2019 22:21
@alexander-schranz
Copy link
Member Author

@danrot did have a look at it but seems the validate method is not the only case where isCacheable is called or where logic is based on the getMaxAge function of the Response.

@alexander-schranz alexander-schranz changed the title Fix usage of symfony cache with shared max age 0 WIP: Fix usage of symfony cache with shared max age 0 Feb 5, 2019
@alexander-schranz
Copy link
Member Author

Maybe obsolute after the changes in: symfony/symfony#26532

@danrot
Copy link
Contributor

danrot commented Apr 4, 2019

What does "maybe" mean? 🙂 Can we close this PR?

@alexander-schranz alexander-schranz deleted the bugfix/symfony-cache-shared-max-age-0 branch April 4, 2019 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Error or unexpected behavior of already existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants