If you have time@mpdude, I would love to get your review on this one.

I'd really like to help here! It's a bit difficult, though, as the description does not directly make clear what we're after.

After reading the linked issues my understanding is that we want to enable theResponseCacheStrategy to deal withprivate responses and come up with a result better thanno-cache, must-revalidate – that is, provide an expiration time.

Before discussing the implementation, can we agree on what the merge policy needs to be?

I have reviewed the existing tests and think that the merge rules could be as follows.

0. If we have no embedded response, leave the master response alone.
1. If one of the responses isprivate, the result must beprivate. Otherwise, all responses arepublic.
2. Validation-type headers (Last-Modified,ETag) need to be removed. Validation cannot reasonably be used on merged responses.
3. If the result ispublic, compute the finals-maxage by looking for the smallest value among all responses, considerings-maxage,max-age andExpires in this order. If only a single response
   does not provide any of these, the result must useno-cache, must-revalidate.
4. Try to compute a finalmax-age by looking for the smallest value among all responses, consideringmax-age andExpires in this order. If only a single response does not provide any of these two, we cannot computemax-age.
5. If the result isprivate and we don't have themax-age, we need to resort tono-cache, must-revalidate.
6. CalculateAge in a way that the resulting response becomes stale as soon as the lowest TTL of any response expires. [Do we have to considers-maxage ormax-age here?]

Does that make sense?

0. agreed

1. If not all of the responses are marked aspublic, the final should not havepublic imho. Not having apublic directive is not the same as not having any directive. In the same sense, having oneprivate response (allow browser caching) does not mean all responses can be cached by the browser. Only if all other are eitherpublic orprivate a browser should be allowed to cache the result.FYI: this might not be implemented in my PR.

2. agreed

3. Why do we merge multiple header values that are not meant to be the same?

   • s-maxage is for shared caches only
   • max-age is for private caches as well as for shared caches if nos-maxage is defined
   • Expires headers are similar tomax-age, but mostly useful for HTTP/1.0 protocol.Expires can existing in combination withmax-age to control or prohibit caching for HTTP/1.0 servers, especially if they do not support features likemust-revalidate.

4. same as 3.

5. Aprivate directive withoutmax-age is perfectly fine. Each cache definition is valid without other. It's up to the cache server to decide on a cache duration if none is given. This assumesall responses are marked as private but all or some of them do not have amax-age.

6. The age has nothing to do with expiration time.Age is determined by the difference betweenDate header (when the response has been added to cache) andnow. After merging multiple responses, age must represent the oldest response's age imho.

I still don't understand why Symfony generally adds ano-cache, must-revalidation. Quoting from#26245

Ano-cache directive essentially tells all caches (and that includes the Browser, which is a private cache) to no cache the response.

I think you're misreading the RFC you quoted?no-cache means "don't serve this from a cache without revalidation",private means "store in private caches only".no-store is the way of stating "don't keep this in caches".

You're somewhat right with my incorrect interpretation. I wasn't very sure about revalidation, so I looked it up:https://stackoverflow.com/questions/18148884/difference-between-no-cache-and-must-revalidate#19938619
So essentially,no-cache, must-revalidate allows a cache to store it but never to use it without revalidation. And we are removing both possible revalidation options (Etag and Last-Modified, see 2.), which essentially results in a useless waste of storage? Also,no-cache impliesmust-revalidate, so the second directive is not useful either?

Any progress here? The current implementation breaks client cache headers when working with ESI fragments :(

friendly ping@aschempp

I have updated the PR to fix some mentioned coding style and implemented point 1 inmy comment. I know there's more, but I think we should discuss the general idea before I finish the implementation.

However, most of the notes in my thoughts in the last comment still stand, so I guess we should also ping@mpdude 😁

Before we discuss the fine details of this though, we should agree or even write down our thoughts on the supposeddefault behavior. Symfonyalways adds cache control headers which I consider strange. Especially the second if-condition means I can have anExpires header but theResponseHeaderBag will add a cache-control directive that "breaks" this for any HTTP/1.1 cache.

According the the RFC, not having aCache-Control header is perfectly fine, but that is not possible with Symfony. Is this behavior considered correct@mpdude /@nicolas-grekas ? And if we want to be conservative (prevent caching if no caching headers are given), I think we should still correctly handle HTTP/1.0 headers?

Is there anything I can do to push this forward?

@aschempp would you mind rebasing please?

Rebase is done. The change fromaschempp@3d27b59#diff-e5a339b48cec0fa0a4d0c5c4c705f568R75 has been dropped, but I preserved the unit test and they are still passing.

I just looked at the disabled unit tests and will follow up with a few changes shortly.

Sorry for taking a while. I tried to implement merging ofLast-Modified andEtag but realized this won't work reliably. They now render a response uncachable without additional expiry headers.

One thing I'm not sure about is the default Response behavior. If there are notCache-Control headers, Symfony always addsprivate, must-revalidate.https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/ResponseHeaderBag.php#L309-L331

First of all,private means a browser is allowed to cache it, which is not the same asno Cache-Control provided, ie. probably not cacheable. Secondly,must-revalidate does not make sense because there is never any validation information (Last-Modified andEtag are removed), a validation will never be possible.

I have removed two assertions in the HttpCacheTest to fix tests. I'm very well aware that removing assertions to fix a test is not really a practical solution. However, as explained in#26245 (comment), I think the assertions (and Symfony behavior) were wrong in the first place …

Any follow-up questions for this PR I need to address?

(could you please rebase on latest 3.4?)

For me, this PR is too confusing to give meaningful feedback.

Can we somehow simplify this or break it into smaller pieces we could discuss independently?

@mpdude did you look at the diff or at the new version of the file directly? What can I do to simplify the changes? I think they are all related, there are no "individual features" that could be split into smaller pieces.

@nicolas-grekas I will rebase as soon as any questions are resolved. Previously all changes in the old file were irrelevant because it is basically a complete rewrite of the class.

Is there anything I can do to get this merged at some point?

@aschempp Can you rebase this pull request? We cannot merge a pull request with a merge commit. Thank you.

What's the status here?
Please rebase to account for short arrays also.

This PR is still ready to me. Be aware that it currently points to 3.4 as a bugfix, so I'm not sure about short arrays?

I have finished everything requested at the SymfonyCon, but we haven't made any more progress since. I'm not sure who's in charge of a final decision on what needs to be completed and actually merge this?

Short arrays have been applied to 3.4 also, that's why a rebase is needed :)

Rebased now and updated to short arrays using the php-cs-fixer

Thanks! What about unanswered comments from David?

Thanks! What about unanswered comments from David?

You mean this one?#26532 (comment)
I'm not sure how to handle this, or how a different comment would be an improvement…

the open comments are all about improving the phpdoc or doc comments. basically for the places where i misunderstood the code before, because i think the explanations given in the discussion here and in person in lisbon would merit to be in the code file for future reference. some of the interactions are quite intricate (i don't see a way to improve that) and therefore need to be well documented so we still remember why things are as they are in a couple of years.

Any suggestions maybe? :)

Thank you@aschempp.

Great job@aschempp and all others involved!