It looks like you unchecked the "Allow edits from maintainer" box. That is fine, but please note that if you have multiple commits, you'll need to squash your commits into one before this can be merged. Or, you can check the "Allow edits from maintainers" box and the maintainer can squash for you.

Cheers!

Carsonbot

@nicolas-grekas Could I get a review on this?

I'm not sure I get the full consequences of this. Maybe@aschempp could help review?

For completeness: I provided a minimal reproducer in the PR mentioned above (aleho/symfony-httpcache)

Have a look atEsi.php to reproduce the issue.

Thanks for the ping@nicolas-grekas. The PR sounds valid, but if I understand it correctly it can only ever appear if#42355 gets merged, is that correct@aleho? Also, I'm unsure if HttpCache correctly handles a 304 from an ESI fragment, possibly extending the lifetime of the cache etc, did you maybe check for that?

@aschempp

Yes, that's right, the problem isn't triggered normally, becauseResponseCacheStrategy doesn't "support" 304 at the moment and won't send the headers, so clients won't request with an "If-Modified-Since" header.

But, and this is what this PR is about, you can easily trigger a server 500 by sending the header in a request.
I demonstrated this in therepo I linked to.

Unmodified Symfony with ESI enabled:

curl -kI https://symfony.localhost -H 'If-Modified-Since: Tue, 15 Feb 2022 9:00:00 GMT'HTTP/2 500 access-control-allow-origin: https://symfony.localhostalt-svc: h3=":443"; ma=2592000,h3-29=":443"; ma=2592000cache-control: no-cache, privatecontent-type: text/html; charset=UTF-8date: Tue, 15 Feb 2022 11:18:28 GMTstatus: 500 Internal Server Errorx-debug-exception: Error%20when%20rendering%20%22https%3A%2F%2Fsymfony.localhost%2F_fragment%3F_hash%3DfF%252Bd4YUs%252BLEmtFBw8s987QLP7mBirm5Ra2EBed1yzbk%253D%26_path%3D_format%253Dhtml%2526_locale%253Den%2526_controller%253DApp%25255CController%25255CDefaultController%25253A%25253Aheader%22%20%28Status%20code%20is%20304%29.x-debug-exception-file: %2Fapplication%2Fvendor%2Fsymfony%2Fhttp-kernel%2FHttpCache%2FAbstractSurrogate.php:99

Again, this happens with an unmodified Symfony installation and ESI enabled. In my opinion this is a bug, there should be no 500 when sending a request with an "If-Modified-Since" header.

In my demo I didn't even return a 304 inany of the controller actions / fragments, I only set a last modified date.
This is enough forHttpCache to correctly checkisNotModified with the (client) request and correctly return a 304 (as the fragment action set a last-modified date), but ESI assuming 304 is an invalid status will throw an exception.

@aschempp@nicolas-grekas Do you have any new feedback on this?

All Symfony versions I tested are still affected.

IIUC, it needs#42355, which is going to be part of 6.2 (as a new feature). So, I'd suggest to wait for the other PR to merged and then, this one can be added on top for 6.2 as well.

@fabpot This PR is actually independent of the other one.

The 500 can be observed in any Symfony version if the browser sends anIf-Modified-Since header (even though it shouldn't, Symfony should also not produce a 500 on the other hand, I think).

Thank you@aleho.