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

feat: allow to configure header name for reverse_proxy_ttl specific value #638

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

94noni
Copy link
Contributor

@94noni 94noni commented Jan 8, 2025

from FriendsOfSymfony/FOSHttpCache#591

Fastly allows to cache via this https://www.fastly.com/documentation/reference/glossary/#term-surrogate-control header Surrogate-Control
(see also https://docs.fastly.com/en/guides/controlling-caching#setting-different-ttls-for-fastly-cache-and-web-browsers)

a specific origin response header, and this bundle allows to define a default value

this PR is allows to configure the ttl header name in the bundle avoid to create a fastly vcl or compute code for example

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, this makes sense to have this configurable 👍

can you please

  • add a note about this new option to the changelog
  • add some tests that ensure changing the header to a non-default name works? in the CacheControlListenerTest unit test and the ConfigurationTest unit test (the later is currently failing because the default value is showing up and not yet expected by the test)
  • update headers.rst documentation: add a mention and additional example (i'd keep the existing example and add another one to show how to customize the header) to the reverse_proxy_ttl section

src/DependencyInjection/FOSHttpCacheExtension.php Outdated Show resolved Hide resolved
src/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved
src/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved
@94noni 94noni force-pushed the ttl-header-cache-control branch 4 times, most recently from 7a5c1f0 to 35a73cc Compare January 8, 2025 16:29
@94noni
Copy link
Contributor Author

94noni commented Jan 8, 2025

@dbu thanks a lot for your fast review here 👍🏻
comments addressed, please review again and tell me if its all good :)

@dbu
Copy link
Contributor

dbu commented Jan 8, 2025

thanks for the quick reaction. can you document in the reference documentation instead of the features overview? and adjust the ?? fallback please - see comments above?

@94noni 94noni force-pushed the ttl-header-cache-control branch from 35a73cc to 8c3ee8b Compare January 8, 2025 16:48
Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@dbu dbu merged commit 7995f55 into FriendsOfSymfony:3.x Jan 8, 2025
12 checks passed
@dbu
Copy link
Contributor

dbu commented Jan 8, 2025

and released: https://github.com/FriendsOfSymfony/FOSHttpCacheBundle/releases/tag/3.1.0

@94noni
Copy link
Contributor Author

94noni commented Jan 8, 2025

wonderful thanks @dbu for your time here !

@94noni 94noni deleted the ttl-header-cache-control branch January 8, 2025 17:01
@richardhj
Copy link
Contributor

In strict mode, this is the error you'll be receiving:

 [KO]
Script importmap:require returned with error code 1
!!  
!!  In FOSHttpCacheExtension.php line 51:
!!                                                  
!!    [ErrorException]                              
!!    Warning: Undefined array key "cache_control"  
!!                                                  
!!  
!!  Exception trace:
!!    at vendor/friendsofsymfony/http-cache-bundle/src/DependencyInjection/FOSHttpCacheExtension.php:51

$ttlHeader = $config['cache_control']['ttl_header'];

@richardhj richardhj mentioned this pull request Jan 9, 2025
@alexander-schranz
Copy link
Contributor

This seems also introduce other issues:

TypeError: FOS\HttpCacheBundle\EventListener\CacheControlListener::__construct(): Argument #2 ($ttlHeader) must be of type string, null given, called in /home/runner/work/sulu/sulu/src/Sulu/Bundle/WebsiteBundle/Tests/Application/var/cache/website/test/ContainerY4qBT6C/Sulu_Bundle_WebsiteBundle_Tests_Application_KernelTestDebugContainer.php on line 530

PHP Warning: Undefined array key "cache_control" in /home/runner/work/sulu/sulu/vendor/friendsofsymfony/http-cache-bundle/src/DependencyInjection/FOSHttpCacheExtension.php on line 51

PHP Warning: Trying to access array offset on value of type null in /home/runner/work/sulu/sulu/vendor/friendsofsymfony/http-cache-bundle/src/DependencyInjection/FOSHttpCacheExtension.php on line 51

@dbu
Copy link
Contributor

dbu commented Jan 9, 2025

@alexander-schranz
Copy link
Contributor

Thank you all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants