| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | theme system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Twig, Twig engine |
Issue Summary
Problem/Motivation
The Twig engine provides options for configuring debugging, automatic reloading (recompiling) of templates, and caching compiled templates in the filesystem. The core implementation of Twig supports these options but they are currently hardcoded and cannot be overridden by developers. These are the defaults in core:
debug: TRUE
auto_reload: FALSE
cache: TRUE
Enabling debug should also implicitly enable auto_reload unless auto_reload is set to FALSE. Having auto_reload default to NULL is Twig's default behaviour and would make more sense to folks who are already familiar with Twig.
Proposed resolution
Originally a UI solution was proposed, but since these settings are used by developers and will likely be different per environment, settings.php configuration was determined to be a better solution.
The patch adds three new settings (twig_debug, twig_auto_reload, twig_cache) to settings.php to control these Twig environment options and uses the Settings API to access this information when building the Twig environment.
The proposed patch also makes auto_reload match Twig's implementation and documentation, so that enabling debug also enables auto_reload unless a value is specified for auto_reload.
Remaining tasks
Review the latest patch.
User interface changes
None
API changes
None
Original report by japerry
in TwigFactory.php , autoreload is hardcoded to TRUE. We should be listening to the performance settings page instead.
Comments
#1
Per conversation with CHX and FabienX -- we've made a new twig page under performance that allows the setting of three variables:
'twig.debug.enabled'
'twig.debug_verbose.enabled'
'twig.autorefresh.enabled'
It also sets these variables in the TwigFactory in initializing twig.
#2
#3
Per IRC: In twig.engine please do output the
<!--only if debug_verbose is set. Thanks!#4
add the verbose flag check before rendering the twig templates.
Incorporates changes from the first patch, use this one instead.
#5
Just to clarify, I believe filter.module on d.o stripped out info from comment #3, it should say "do output the
<!--only if debug_verbose is set"#6
Please commit.
#7
*FIXED* here:
http://drupalcode.org/sandbox/pixelmord/1750250.git/commit/6e1c158d8304e...
#8
Tagging for move to core with "Twig engine"
#9
Automatically closed -- issue fixed for 2 weeks with no activity.
#10
Moving this from Twig sandbox to core queue. Attached patch adds admin settings for Twig cache, debug, and autorefresh options.
#11
The last submitted patch, drupal-make-twig-settings-configurable--1843034-10.patch, failed testing.
#12
+++ b/core/lib/Drupal/Core/CoreBundle.php@@ -339,7 +339,7 @@ protected function registerTwig(ContainerBuilder $container) {
+ 'cache' => $config->get('twig.debug.enabled') ? TRUE : FALSE,
Fatal error: Call to a member function get() on a non-object
Ah, $config is not available yet...
Hmm... Ideas?
#13
I'm guessing this is related to #1873442: Refactored core Twig integration
#14
Let's try this one
#15
Nevermind, sorry.
#16
Is it really necessary that these settings are available in the UI? If that requirement can be removed, these would be good candidates either for inclusion in $conf[] or in the new settings() API - #1833516: Add a new top-level global for settings.php - move things out of $conf
#17
Yeah, giving this some thought it seems that these settings pertain to developers, who have access to the codebase, and will vary per-environment. Providing an UI is an anti-pattern.
So can we refactor this for a non-UI solution?
#18
This is untested and the comments need help, but conceptually this is how it should look.
#19
Also, while I saw where it gets set, it appears the the verbose setting doesn't actually get checked or used anywhere?
#20
The last submitted patch, 1843034-twig-settings-17.patch, failed testing.
#21
This new patch restores the update_free_access variable which I had accidentally removed in the last patch. The last test run was (hopefully) canceled.
#22
The last submitted patch, 1843034-twig-settings-20.patch, failed testing.
#23
@heyrocker: you're right, the verbose setting doesn't affect anything currently, but it's used to print the list of theme hook suggestions in the debug comments. Will integrate into patch here after finishing in sandbox -- hopefully in the next hour or so.
#24
This is the extent of which I'd currently like to use Twig settings. I've removed 'debug_verbose' and instead always output all debugging information. Also, I've changed the setting names to reflect the names of the primary Twig options they affect. Thanks for your help, heyrocker. Let's see if it'll install.
#25
#26
+++ b/sites/default/default.settings.phpundefined@@ -284,6 +284,24 @@
+$settings['twig_debug'] = FALSE;
Actually, since all calls to these use FALSE as a default, you can just comment them out. That is what most of the other things in $settings do.
It is probably worth adding tests for these too.
#27
#28
Going to try writing some tests for this.
#29
+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined@@ -342,7 +342,7 @@ protected function registerTwig(ContainerBuilder $container) {
- 'cache' => TRUE,
+ 'cache' => settings()->get('twig_debug', FALSE),
I do not totally agree with that change (though I can without problems live with it) and the reason is performance.
Twig will hopefully be used for everything themed on the site and the compilation cache is an important asset here.
If you recompiled for each row, for each block, foreach breadcrumb, etc. this could be many many compilation runs and it could be slow.
Maybe however twig_debug should imply twig_auto_reload instead. The result is the same, but only for those templates that changed. Though then a note should be added that after enabling disabling twig_debug that the cache should be cleared.
I can see that someone wants to use twig_auto_reload however and not enable the debugging features. (in case of frequent deployments for example on a staging site). So keeping both options is a good choice IMHO.
Unrelated to this: We probably should create a patch to Devel 8.x to add the dpm, etc. functions as another TwigDevelExtension.
Besides that the patch looks good to me.
Thanks!
#30
Crosspost :)
#31
To summarize the points by Fabianx in #29: Twig 'cache' setting will be set by $settings['twig_cache']; 'debug' by $settings['twig_debug']; and 'auto_reload' by $settings['twig_auto_reload'];
Also, FWIW: in this patch (or the one in #24), in addition to making Twig settings configurable, implements a check for the 'debug' setting to output debugging information around twig templates.
#32
Made some progress on a test, the main issue I'm having is that the settings I'm overriding in my test methods are not being picked up by the Twig engine.
I did manual testing by changing my settings.php and that went fine, I did find that I needed to clear the cache so that the setting changes in settings.php took effect.
The settings are definitely being changed, I can check settings()->get() and see they are being updated. I've tried $this->resetAll() and even drupal_flush_all_caches() after $this->settingsSet() but that doesn't make any difference, nor does putting $this->settingsSet() into setUp(). I'm not sure if part of the issue is because Twig is loaded so early in the bootstrap, or maybe this is a DIC thing that I just don't understand yet.
Interdiff doesn't show the test being added, just the changes to other files from #24.
#33
+++ b/sites/default/default.settings.phpundefined@@ -299,7 +299,17 @@
+ * Disabling the Twig cache will store the compiled Twig templates in RAM
+ * instead of the file system.
Ha ha, that is a nice way to put it, but more correct would be IMHO that the templates are recompiled from source each time they are used.
However given that there is static class caching, you could say that it is "stored" for the duration of one request.
#34
+++ b/sites/default/default.settings.php@@ -284,6 +284,34 @@
/**
+ * Twig debugging:
+ *
+ * Displays debugging information in comments surrounding Twig template output.
+ *
+ * Not recommended in production environments.
+ */
+# $settings['twig_debug'] = TRUE;
Should we note the defaults?
i.e.
* Not recommended in production environments (Default: FALSE);
+++ b/core/themes/engines/twig/twig.engine@@ -31,22 +31,49 @@ function twig_init($template) {
+ * Renders twig templates.
I'd edit to say:
"Render twig template."
+++ b/core/themes/engines/twig/twig.engine@@ -31,22 +31,49 @@ function twig_init($template) {
+ * The output generated by the template, in addition to any debug information.
I'd edit to say:
"The output generated by the template, plus any debug information."
+++ b/core/themes/engines/twig/twig.engine@@ -31,22 +31,49 @@ function twig_init($template) {
- return drupal_container()->get('twig')->loadTemplate($template_file)->render($variables);
+ $output = array(
+ 'debug_prefix' => '',
+ 'debug_info' => '',
+ 'rendered_markup' => drupal_container()->get('twig')->loadTemplate($template_file)->render($variables),
+ 'debug_suffix' => '',
+ );
+ if (settings()->get('twig_debug', FALSE)) {
+ $output['debug_prefix'] .= "\n\n<!-- THEME DEBUG -->";
+ $output['debug_prefix'] .= "\n<!-- CALL: theme('{$variables['theme_hook_original']}') -->";
+ if (!empty($variables['theme_hook_suggestions'])) {
+ $extension = twig_extension();
+ $current_template = explode('/', $template_file);
+ $current_template = array_pop($current_template);
+ $suggestions = $variables['theme_hook_suggestions'];
+ $suggestions[] = $variables['theme_hook_original'];
+ foreach ($suggestions as $key => &$suggestion) {
+ $template = $suggestion . $extension;
+ $prefix = ($template == $current_template) ? 'x' : '*';
+ $suggestion = $prefix . ' ' . str_replace('_', '-', $template);
+ }
+ $output['debug_info'] .= "\n<!-- FILE NAME SUGGESTIONS:\n " . implode("\n ", $suggestions) . "\n -->";
+ }
+ $output['debug_info'] .= "\n<!-- BEGIN OUTPUT from '{$template_file}' -->\n";
+ $output['debug_suffix'] .= "\n<!-- END OUTPUT from '{$template_file}' -->\n\n";
+ }
Do we like our debug output?
+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigSettingsTest.php@@ -0,0 +1,56 @@
+ /**
+ * Ensures Twig engine debugging can be overridden.
+ */
+ function testTwigDebugOverride() {
+ $this->settingsSet('twig_debug', TRUE);
+ variable_set('theme_default', 'test_theme_twig');
+
+ $this->drupalGet('theme-test/template-test');
+ $this->assertRaw('<!-- THEME DEBUG -->', 'Twig debug can be overridden.');
+ }
+
Maybe we can extend this test and test that our FILE NAME SUGGESTIONS match our theme_hook_suggestions. On that note, I noticed some duplicates in theme_hook_suggestions. May want to check out http://drupal.org/project/issues/drupal?text=theme_hook_suggestions
Lastly, I think the settings should be in this order (logical to me): Cache, Auto-Reload, Debug
#35
actually, they will always match theme_hook_suggestions -- guess i just wanted to point to the issues around theme_hook_suggestions, as they're relevant here.
#36
If anyone has any ideas for how to test this I'm all ears. I'll be taking another look at the tests tonight though. After sleeping on it I think the issue is that the Twig environment is initialized in bootstrap and by that time it's too late to change any settings except by settings.php. Need to confirm that though. I think other tests that use $this->settingsSet() are dealing with run-time settings that can be changed on the fly.
Rerolled to hopefully address most of #33 and #34.
Some comments on #34:
I went with "Renders a Twig template.", per http://drupal.org/node/1354#functions.
Looks good to me, but discussing that would be better handled in another issue IMO.
I can see putting auto-reload before debug, but it sounds like disabling the cache should almost never be done because it would be a big performance hit. If we put cache first, people might just disable cache and not even realize they can just turn on auto-reload. Leaving the order as is for now, let's discuss this a bit more.
#37
Got a bit further on the test, the auto_reload test is passing but I'm still having trouble with the debug output test. It passes if I actually enable twig_debug in settings.php so the assertion is correct. Testbot can have a peek at this one.
#38
Almost forgot, huge thanks to @sun for telling me about
$this->rebuildContainer()in #drupal-contribute.#39
The last submitted patch, 1843034-37.patch, failed testing.
#40
This is not fully testable to the extent I was hoping, for example I don't think we can test that the debug markup gets inserted or that cache files don't get generated when caching is off.
Spoke to @tim.plunkett and @chx on IRC tonight and the main issue is this: you can override settings in the current request via
$this->settingsSet()and$this->rebuildContainer(), but as soon as you do adrupalGet(), that's a new request and any settings overrides are gone since they are only stored in memory.So here's a test only and combined patch with tests for all three settings to the extent that I've found them to be testable. The test only patch should show 2 failures.
I tested all three settings in settings.php and they worked as expected, would like someone else to perform manual testing.
#41
I'm going to add one more assertion each to the debug and auto_reload test methods, I don't like that they rely on the default values - the debug test method will currently pass because debug is currently set to TRUE in core.
#42
#40: 1843034-40.patch queued for re-testing.
#43
Updated per #41, looking for 3 failures in the test only patch, one for each setting.
#44
After talking to @Fabianx,
twig_auto_reloadshould default to NULL so thatauto_reloadis enabled whendebugis enabled. This makes our implementation match Twig's: http://twig.sensiolabs.org/doc/api.html#environment-optionsUpdated the default, added some more test coverage, and updated/tweaked default.settings.php to reflect this change.
#45
Restoring disappearing tags.
#46
#44: We should probably also add this information:
"debug: When set to true, the generated templates have a __toString() method that you can use to display the generated nodes (default to false)."
from Twig to settings.php in addition to our own debug markup.
#47
We discussed #46 in IRC and decided to update default.settings.php with a link to a (to be completed) handbook page with more information about debugging.
#48
Nitpick:
+++ b/sites/default/default.settings.php@@ -284,6 +284,41 @@
/**
+ * Twig debugging:
+ *
+ * Display debugging information in comments surrounding Twig template output
+ * and automatically recompile templates whenever the source code changes.
+ *
+ * @see http://drupal.org/node/1906392
+ *
+ * Not recommended in production environments (Default: FALSE).
+ */
+# $settings['twig_debug'] = TRUE;
+
+/**
+ * Twig auto-reload:
@see at the end of the comment block?
#49
I could go either way on the @see, the standards don't dictate where it needs to be. I think it makes sense where it is.
#50
Cottser: Sounds good. I like that the production environment warnings are in the same place (last line) in all the docblocks.
As we discussed on irc Cottser, let's pull the theme hook / template suggestions debug output feature into #1906506: Twig settings implementation: Show theme hook / template suggestions in HTML comments. This way we keep implementation out of this config/settings issue.
#51
Also, re manual testing: #40, we don't need to worry about [manual] testing the *implementation* of the settings here, only that the settings are set and overriden properly. It's the job of the Twig extension to handle those settings properly.
#52
Just updated the issue summary, here is a revised patch without the additional debug output.
#53
Per #51, and since we're no longer worrying about the debug output in this issue, I think the test coverage is plenty.
#54
Don't mind me, just fixing the name of the theme_test template file in a comment.
#55
+++ b/sites/default/default.settings.phpundefined@@ -284,6 +284,41 @@
+ * Display debugging information in comments surrounding Twig template output
+ * and automatically recompile templates whenever the source code changes.
This patch is close to RTBC, but this needs to be fixed first:
This comment now is wrong.
Lets use / copy the one from twig and fix the other issue to patch the debug output in as follow-up:
"debug: When set to true, the generated templates have a __toString() method that you can use to display the generated nodes (default to false)."
http://twig.sensiolabs.org/doc/api.html#environment-options
#56
+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigSettingsTest.phpundefined@@ -89,7 +89,7 @@ function testTwigCacheOverride() {
- // theme_test_template_test.html.twig.
+ // theme_test.template_test.html.twig.
I don't understand this change ... Why change only the comment?
#57
#55: great catch. I've updated the comment to remove the part about additional debug output, I'd rather not put anything about
__toString()and non-Drupal "nodes" in the docblock though. I added a note about the 'dump' function, I think explaining debugging in depth is better explained in the handbook and I just drafted a new handbook page on debugging variables: http://drupal.org/node/1906780#56: the template filename is theme_test.template_test.html.twig, the array index in $templates is theme_test_template_test.
#58
Looks good to me, was tested manually, docs are correct now => RTBC
#59
There was a chmod change in the patch which I think I avoided. Committed/pushed to 8.x.
#60
@catch: Thanks for noticing my accidental chmod, and thanks for the commit!
#61
I just noticed the added test was missed in the commit:
http://drupalcode.org/project/drupal.git/commit/c05b5bf
#62
Back to RTBC based on #58 to get the test committed :)
#63
Slight tweak of the test to use the twig_extension() function.
#64
The patch in #63 incorrectly contains #1906506-21: Twig settings implementation: Show theme hook / template suggestions in HTML comments. Here is the correct patch.
#65
More tests are seldom a bad thing, they pass as well => RTBC
#66
#63 looks good
+1 RTBC
#67
The patch in #64 is the patch to look at, even though I didn't rename the patch file.
#68
Change notice: http://drupal.org/node/1922666
#69
#68 looks good for me
#70
Test in #64 still needs to be committed :)
#71
woops
cross post
this #64 still needs commit
#72
Wow, that was off my radar, sorry!
Committed and pushed to 8.x. Yay, tests. :D
#73
TwigSettingsTestis failing in HEAD, both on the bot and locally.Drupal\system\Tests\Theme\TwigSettingsTest (12 pass(es), 1 fail(s), and 0 exception(s))- [fail] [Other] "Cached Twig template found." in TwigSettingsTest.php on line 99 of Drupal\system\Tests\Theme\TwigSettingsTest->testTwigCacheOverride().
Need to retest month-old patches before we commit them I think. :)
To roll this back:
git revert 3cf107#74
Oops. :) I'll get this in about a half hour when I get home.
#75
Ok, should be fixed up now. We'll need a (hopefully quick) re-roll of this one.
#76
I'm on it.
#77
Rerolled for #1829224: Convert the 'theme_default' variable to CMI.
#78
Oh, it was a simple thing after all. :)
RTBC if it passes.
#79
ufo cleaned
#80
#77 +1 RTBC
#81
#77: 1843034-77-test.patch queued for re-testing.
#82
#77: 1843034-77-test.patch queued for re-testing.
#83
#84
Committed d569e20 and pushed to 8.x. Thanks!
#85
Automatically closed -- issue fixed for 2 weeks with no activity.
#86
Reverting title for posterity.