[HttpKernel] Refine Vary header check to skip special handling of 'Accept-Language' when it's the only entry and '_vary_by_language' is `true` in `CacheAttributeListener` #61368

santysisi · 2025-08-09T00:46:03Z

Q	A
Branch?	6.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	Fix #61229
License	MIT

Summary

Refine Vary header check to skip special handling of 'Accept-Language' when it's the only entry and '_vary_by_language' is true in CacheAttributeListener

Reason

Previously, the CacheAttributeListener was executed after the ResponseListener. When the set_locale_from_accept_language option is enabled in framework.yaml, the ResponseListener sets a Vary header on the response. Because CacheAttributeListener checks if a Vary header already exists before modifying it, its logic was being bypassed if it ran after the ResponseListener.

References

santysisi · 2025-08-09T18:25:04Z

If we want to avoid changing the priority, we can add a condition to check whether the request has the _vary_by_language attribute set to true, and the response's Vary header contains only Accept-Language.

nicolas-grekas · 2025-08-19T14:47:59Z

check whether the request has the _vary_by_language

let's do this, so that this won't interfere with app's listeners

santysisi · 2025-08-20T02:28:34Z

status: needs work

There is a problem, i will try to resolve that problem tomorrow

santysisi · 2025-08-22T22:18:08Z

status: needs review

The problem was resolved by sending the 'Accept-Language' header at the end

Spomky · 2025-08-27T09:19:08Z

Many thanks @santysisi.
Looks good to me. I have just one question: how Vary: * is covered?

santysisi · 2025-08-27T22:00:09Z

Thanks for the review! 🙌

Regarding Vary: *, I don't think it's currently covered.

I tested it locally, and it seems that when the response includes Vary: *, it still gets cached. I'm not sure if that's a bug or expected behavior, but it happens regardless of this PR, so it doesn't appear to be introduced by these changes.

Spomky · 2025-08-28T07:55:11Z

I found in RFC9111 section4.1 the following sentence: A stored response with a Vary header field value containing a member "*" always fails to match..
So I understand Vary * should be treated as a relevant case and left untouched (no append).

May I suggest to add 2 test cases.

    yield 'vary * (no append) — vary_by_language=true' => [
        'responseVary'   => ['*'],
        'cacheVary'      => ['X-Foo'],
        'varyByLanguage' => true,
        'expectedVary'   => ['*'],
    ];

    yield 'vary * (no append) — vary_by_language=false' => [
        'responseVary'   => ['*'],
        'cacheVary'      => ['X-Foo'],
        'varyByLanguage' => false,
        'expectedVary'   => ['*'],
    ];

Edit: and hasRelevantVaryHeader to directly return true if * is present.

santysisi · 2025-08-28T22:13:34Z

Hey, thanks a lot for your suggestions! The change has been made

Edit: I'm not sure if it's necessary to add the modification to the hasRelevantVaryHeader method to directly return true when * is present. With the current condition, I believe it already returns true in that case (your suggested test seems to confirm that behavior, thanks again for the suggestion! ❤️ ). But if you still think the change is needed, I'm happy to update it! 😄

Spomky

Looks good. No additional comment.
Many thanks!

cdaguerre · 2025-09-04T13:33:02Z

This should also fix: #58663

nicolas-grekas

LGTM, just some nitpicking

src/Symfony/Component/HttpKernel/Tests/EventListener/CacheAttributeListenerTest.php

src/Symfony/Component/HttpKernel/EventListener/CacheAttributeListener.php

src/Symfony/Component/HttpKernel/Tests/EventListener/CacheAttributeListenerTest.php

santysisi · 2025-09-05T22:23:40Z

Hi! Thanks a lot for your suggestions. I’ve made the changes you recommended.
Please let me know if there’s anything else needed.
Thanks again! ❤️

nicolas-grekas

LGTM, I'm just wondering about these.

src/Symfony/Component/HttpKernel/EventListener/CacheAttributeListener.php

nicolas-grekas · 2025-09-08T14:02:03Z

src/Symfony/Component/HttpKernel/EventListener/CacheAttributeListener.php

        }

+        if (($vary = $response->getVary()) && \in_array('Accept-Language', $vary, true)) {
+            // Ensure 'Accept-Language' is included at the end of the Vary header


Why is this necessary BTW?

This is necessary because when you have multiple requests with different headers listed in the Vary response header, if Accept-Language is not included in the final response, the cache will not be used for subsequent requests. Each request will be treated as unique, and the previously cached response won't apply.

my question was about the ordering: why do we need to reorder?

Sorry, my previous comment was incorrect. What I meant to say is:

The issue occurs if Accept-Language is not included at the end of the Vary header in the response , not if Accept-Language is missing from the response itself.

I haven't tested this with many servers yet, but I noticed this behavior using the Symfony local server (started via symfony server:start). I'm not sure whether it's an issue with Symfony itself (I don't think so), or something related to the local web server.

I'll try to test with other servers over the weekend and will update here

Would you have some reproducer I could run with the Symfony local server?

Sure, I’ll give it a try as soon as possible 😄

in case you're up to having a look: ping @philpichet ;)

Hi! I hope you're doing well 😊
I tried to reproduce the error yesterday and today, but I haven’t been able to 🤔
I’m not sure if it was something specific to my computer or environment at the time, but I'm now using the same machine and the same reproducer and everything is working fine.

I’m really not sure what happened. Sorry for the confusion 🙏

@philpichet did some changes in #61518 on the topic, this might have fixed it!

Oh, got it! 😄 Thanks for the explanation, and big thanks to @philpichet for the fix ❤️

santysisi · 2025-09-08T22:19:08Z

Hi Nicolas, thanks a lot for your suggestions! ❤️
The changes have been made. Please let me know if there's anything else you'd like to adjust

…cept-Language' when it's the only entry and '_vary_by_language' is `true` in `CacheAttributeListener`

nicolas-grekas · 2025-09-15T14:23:46Z

Thank you @santysisi.

carsonbot added Status: Needs Review Bug HttpKernel labels Aug 9, 2025

This comment has been minimized.

Sign in to view

carsonbot added this to the 7.4 milestone Aug 9, 2025

santysisi changed the base branch from 7.4 to 6.4 August 9, 2025 00:47

santysisi force-pushed the fix/cache-listener-priority branch from 9eec426 to 04f4315 Compare August 9, 2025 00:48

santysisi force-pushed the fix/cache-listener-priority branch 2 times, most recently from 8240e2d to 8085b7e Compare August 20, 2025 02:18

santysisi changed the title ~~[HttpKernel] Adjust priority of CacheAttributeListener to ensure Vary header is not skipped~~ [HttpKernel] Refine Vary header check to ignore 'Accept-Language' when handled by request in CacheAttributeListener Aug 20, 2025

santysisi force-pushed the fix/cache-listener-priority branch from 8085b7e to 4a186a5 Compare August 20, 2025 02:20

santysisi force-pushed the fix/cache-listener-priority branch from 4a186a5 to 85c3d45 Compare August 20, 2025 02:27

carsonbot added Status: Needs Work and removed Status: Needs Review labels Aug 20, 2025

santysisi force-pushed the fix/cache-listener-priority branch from 85c3d45 to 34293e7 Compare August 22, 2025 22:03

carsonbot added Status: Needs Review and removed Status: Needs Work labels Aug 22, 2025

santysisi force-pushed the fix/cache-listener-priority branch from 34293e7 to ce29227 Compare August 28, 2025 22:21

Spomky approved these changes Aug 29, 2025

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Aug 29, 2025

nicolas-grekas approved these changes Sep 5, 2025

View reviewed changes

santysisi force-pushed the fix/cache-listener-priority branch from ce29227 to da81baf Compare September 5, 2025 13:26

nicolas-grekas reviewed Sep 8, 2025

View reviewed changes

santysisi force-pushed the fix/cache-listener-priority branch from da81baf to ff32a6e Compare September 8, 2025 21:18

nicolas-grekas modified the milestones: 7.4, 6.4 Sep 9, 2025

santysisi force-pushed the fix/cache-listener-priority branch from ff32a6e to 9537e1c Compare September 10, 2025 03:14

[HttpKernel] Refine Vary header check to skip special handling of 'Ac…

fa2bcbe

…cept-Language' when it's the only entry and '_vary_by_language' is `true` in `CacheAttributeListener`

nicolas-grekas force-pushed the fix/cache-listener-priority branch from 9537e1c to fa2bcbe Compare September 15, 2025 14:23

nicolas-grekas merged commit cae30bb into symfony:6.4 Sep 15, 2025
10 of 11 checks passed

This was referenced Sep 27, 2025

Release v6.4.26 #61864

Merged

Release v7.3.4 #61865

Merged

Uh oh!

[HttpKernel] Refine Vary header check to skip special handling of 'Accept-Language' when it's the only entry and '_vary_by_language' is true in CacheAttributeListener #61368

[HttpKernel] Refine Vary header check to skip special handling of 'Accept-Language' when it's the only entry and '_vary_by_language' is true in CacheAttributeListener #61368

Uh oh!

Conversation

santysisi commented Aug 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Reason

References

Uh oh!

This comment has been minimized.

santysisi commented Aug 9, 2025

Uh oh!

nicolas-grekas commented Aug 19, 2025

Uh oh!

santysisi commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

santysisi commented Aug 22, 2025

Uh oh!

Spomky commented Aug 27, 2025

Uh oh!

santysisi commented Aug 27, 2025

Uh oh!

Spomky commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

santysisi commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Spomky left a comment

Choose a reason for hiding this comment

Uh oh!

cdaguerre commented Sep 4, 2025

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

santysisi commented Sep 5, 2025

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

santysisi commented Sep 8, 2025

Uh oh!

nicolas-grekas commented Sep 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

[HttpKernel] Refine Vary header check to skip special handling of 'Accept-Language' when it's the only entry and '_vary_by_language' is `true` in `CacheAttributeListener` #61368

[HttpKernel] Refine Vary header check to skip special handling of 'Accept-Language' when it's the only entry and '_vary_by_language' is `true` in `CacheAttributeListener` #61368

santysisi commented Aug 9, 2025 •

edited

Loading

santysisi commented Aug 20, 2025 •

edited

Loading

Spomky commented Aug 28, 2025 •

edited

Loading

santysisi commented Aug 28, 2025 •

edited

Loading