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

Throw exception on TemplateCacheWarmer failure #41922

Closed
umulmrum opened this issue Jul 1, 2021 · 2 comments · Fixed by #41944
Closed

Throw exception on TemplateCacheWarmer failure #41922

umulmrum opened this issue Jul 1, 2021 · 2 comments · Fixed by #41944

Comments

@umulmrum
Copy link
Contributor

umulmrum commented Jul 1, 2021

Description
The TemplateCacheWarmer has been ignoring errors since its first release in 2015. But I wonder what the rationale behind this decision is, as a) I'd like errors to pop up as early as possible (i.e. in a CI pipeline where the cache is warmed), and b) other cache warmers do throw exceptions, so ignoring errors doesn't seem to be a general rule in warmers.

Currently all templates with errors are skipped, so errors only appear during runtime (this might also trigger errors when deploying into a readonly filesystem but I didn't test it).

Unfortunately the comment in the empty catch block doesn't tell us why errors are ignored.

tl;dr: I ask to consider the TemplateCacheWarmer to fail on error.

@stof
Copy link
Member

stof commented Jul 1, 2021

Well, if you want to see template syntax errors in CI, run the lint:twig command in your CI.

Also, for templates, making it fail when a template cannot be parsed without errors would require adding a feature to exclude templates from the cache warmer. For instance, the templates shipped in DoctrineBundle for the profiler panels are relying on Twig functions provided by WebProfilerBundle. In a prod environment, you will not have WebProfilerBundle and so not those functions. This would make TemplateCacheWarmer fail when warming up the DoctrineBundle templates (that will never be used in prod as they are used only when you render the profiler, but they are still in the bundle).

@umulmrum
Copy link
Contributor Author

umulmrum commented Jul 2, 2021

Ah, I wasn't aware of your second argument, thanks for the pointer. But that also means that lint:twig can only be run in dev and test environments (where the WebProfilerBundle is active by default), and it might happen that arbitrary templates also depend on dev-only functions and explode in prod, right? Solutions to this might be more complex for developers so I'm not sure what the "perfect" way could look like. At the very least the comment in the catch block could be improved, I'll submit a PR for this later.

@fabpot fabpot closed this as completed Jul 3, 2021
fabpot added a commit that referenced this issue Jul 3, 2021
…Kruppa)

This PR was merged into the 5.4 branch.

Discussion
----------

[TwigBundle] Improve comment on error silencing

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #41922
| License       | MIT
| Doc PR        |

As discussed in the ticket, I think the comment needs to be more expressive.

Commits
-------

51124d5 [TwigBundle] Improve comment on error silencing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants