-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[FrameworkBundle] Added caching to TemplateController #6083
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
[FrameworkBundle] Added caching to TemplateController #6083
Conversation
IMHO I think the caching should be allowed to be set optionally |
I wrote it this way, because I assume, that it will cover more use-cases, than the other way round, but you are right, that this will change the current behaviour. Would like to hear other opinions, because I don't think one uses this action for anything else than fully-static content (means: The current behaviour doesn't feel very useful to me). |
I totally agree, but I would then suggest keep the caching on by default, but have the option to turn it off if necessary |
Actually I think to have caching permanently enabled for static content would probably be the best scenario, but I like to think in terms of flexibility and specific user requirements. It would be great to get some opinions from others on this |
I thought about it and I come to the conclusion, that it is probably a not so good idea to enable caching by default, because ... well, it's not possible to disable it again. I guess something like this
may be not so uncommon as I suggested in the first commit. |
Changed bevahiour: - No caching by default - Implicit public caching, whe one of maxAge or sharedAge is set - Explicit private caching, when private is set and evaluates to true - Explicit public caching, when private is set and evaluates to false
Removed unused constants
@@ -25,11 +25,28 @@ class TemplateController extends ContainerAware | |||
* Renders a template. | |||
* | |||
* @param string $template The template name | |||
* @param int|null $maxAge Max age for client caching | |||
* @param int|null $sharedAge Max age for shared (proxy) caching | |||
* @param bool|null $private Whether or not caching should apply for client caches only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be Boolean
instead of bool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious: Boolean
, but not Integer
?
Can you make a PR for the docs? (symfony/symfony-docs). Thanks. |
Because the main purpose for the
TemplateController
seems to be to render static pages like "disclaimer" and such, it seems useful to allow caching.