Symfony HttpCache listener for custom ttl header #214
Conversation
|
👍 Agreed on having it in the library: everything which is not strictly (full-stack) Symfony should go here. |
|
cool. can you please review the implementation - would prefer to fix implementation before writing documentation and more tests ;-) |
| { | ||
| private $ttlHeader; | ||
|
|
||
| const SMAXAGE_BACKUP = 'FOS-Smaxage-Backup'; |
There was a problem hiding this comment.
Should this also be configurable? Not important I guess.
There was a problem hiding this comment.
|
+1 |
|
|
||
| const SMAXAGE_BACKUP = 'FOS-Smaxage-Backup'; | ||
|
|
||
| public function __construct($ttlHeader = 'X-Reverse-Proxy-TTL') |
There was a problem hiding this comment.
Please add docblock explaining the $ttlHeader param.
|
Let’s pull this into 2.0. So needs a rebase. 😉 |
|
Please add docs and a test. |
|
ok, added documentation and a test. also some random cleanup in the doc. notably i started to write the calls to our subs into the corresponding doc section, rather than the extremly flaky includes. |
| @@ -109,10 +111,22 @@ Constant Getter Default | |||
| ``VARNISH_PORT`` ``getCachingProxyPort()`` ``6181`` port Varnish listens on | |||
| ``VARNISH_MGMT_PORT`` ``getVarnishMgmtPort()`` ``6182`` Varnish management port | |||
| ``VARNISH_CACHE_DIR`` ``getCacheDir()`` ``sys_get_temp_dir()`` + ``/foshttpcache-varnish`` directory to use for cache | |||
| ``VARNISH_VERSION`` ``getVarnishVersion()`` ``4`` installed varnish application version | |||
There was a problem hiding this comment.
this was rather puzzling me with my varnish 3 locally. i think it does make sense to use an environment over a constant here.
There was a problem hiding this comment.
Should we then remove the constant and getter for VARNISH_VERSION? I prefer to keep them, as it’s consistent this way with the other config params. I agree that you’ll typically set the Varnish version through an env var. By the way, testing against multiple Varnish versions is most useful for library/framework code; and less relevant for users testing an app against their caching setup.
There was a problem hiding this comment.
i am not removing anything except wrong documentation. i added documentation for the environment variable and keep the getter that is using the env variable. we use the env variable both in VarnishTestCase and tests/bootstrap.php
we could alternatively look at both or have tests/bootstrap.php copy the env variable into a constant. but that all feels a bit clumsy and just as confusing.
| issue invalidation requests. Let’s call the ACL `invalidators`. The ACL below | ||
| will be used throughout the Varnish examples on this page. | ||
| To invalidate cached objects in Varnish, begin by adding an `ACL` (for Varnish | ||
| 3 see `ACL for Varnish 3`_) to your Varnish configuration. This ACL determines |
There was a problem hiding this comment.
We should have a Varnish 3 tab below with the ACL config.
There was a problem hiding this comment.
there is no difference actually. i just wanted to link to the proper version of the documentation. not sure how we should handle that. duplicating the doc config with no difference seems a bit strange. should i just say that this did not change?
There was a problem hiding this comment.
this question is still open. @ddeboer want me to do anything more here?
There was a problem hiding this comment.
okay, updated. its actually no longer just an example...
|
updated |
| const struct gethdr_s hdr = { HDR_BERESP, "\024X-Reverse-Proxy-TTL:" }; | ||
| ttl = VRT_GetHdr(ctx, &hdr); | ||
| VRT_l_beresp_ttl(ctx, atoi(ttl)); | ||
| }C |
There was a problem hiding this comment.
@ddeboer do you see what is wrong here? i have the include above, and the code as you did in the playground. but tests are still not happy. any ideas? can i not include from an included file?
There was a problem hiding this comment.
You’re missing char *ttl; before line 30 where ttl is used: error: ‘ttl’ undeclared (first use in this function).
There was a problem hiding this comment.
oh well. seems i got confused by reading the diff and then just not thinking. thanks!
|
if this is green, i want to squash before we merge. final review anybody? |
|
Looks good to me. Go ahead and squash. 😄 |
|
squashed and some last cleanup. please check and merge if you are happy. |
Symfony HttpCache listener for custom ttl header
|
Thanks! |
fix #151
i guess its ok if this only goes to version 2, its not urgent but for completeness.
TODO
i opt for including this here rather than the bundle, for consistency of where we have http cache listeners. we could also move the vcl and documentation for the varnish custom ttl here - that would also make it easy to test that vcl as we already have the whole setup here.