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.

Files: 
CommentFileSizeAuthor
#77 1843034-77-test.patch4.35 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 53,830 pass(es). View
#77 interdiff.txt712 bytesCottser
#64 1843034-63-test.patch4.32 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 50,532 pass(es). View
#63 1843034-63-test.patch11.15 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 50,576 pass(es). View
#63 interdiff.txt1.5 KBCottser
#57 1843034-57.patch7.23 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 49,107 pass(es). View
#57 interdiff.txt641 bytesCottser
#54 1843034-54.patch7.19 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 49,081 pass(es). View
#54 interdiff.txt903 bytesCottser
#52 1843034-51.patch7.19 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 49,066 pass(es). View
#52 interdiff.txt2.47 KBCottser
#47 1843034-47.patch9.67 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 49,093 pass(es). View
#47 interdiff.txt525 bytesCottser
#44 1843034-44-testonly.patch4.28 KBCottser
FAILED: [[SimpleTest]]: [MySQL] 49,133 pass(es), 4 fail(s), and 0 exception(s). View
#44 1843034-44.patch9.62 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 49,051 pass(es). View
#44 interdiff.txt2.8 KBCottser
#43 1843034-43-testonly.patch3.88 KBCottser
FAILED: [[SimpleTest]]: [MySQL] 48,962 pass(es), 4 fail(s), and 2 exception(s). View
#43 1843034-43.patch9.03 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 49,057 pass(es). View
#43 interdiff.txt2.09 KBCottser
#40 1843034-40-testonly.patch3.41 KBCottser
FAILED: [[SimpleTest]]: [MySQL] 49,163 pass(es), 2 fail(s), and 0 exception(s). View
#40 1843034-40.patch8.56 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 49,079 pass(es). View
#40 interdiff.txt3.29 KBCottser
#37 1843034-37.patch7.19 KBCottser
FAILED: [[SimpleTest]]: [MySQL] 48,718 pass(es), 1 fail(s), and 0 exception(s). View
#37 interdiff.txt1.58 KBCottser
#36 interdiff.txt2.08 KBCottser
#36 1843034-36-do-not-test.patch6.8 KBCottser
#32 interdiff.txt1.53 KBCottser
#32 1843034-32-do-not-test.patch6.53 KBCottser
#32 1843034-32-testonly-do-not-test.patch1.64 KBCottser
#24 drupal-make-twig-settings-configurable--1843034-24.patch4.7 KBsteveoliver
PASSED: [[SimpleTest]]: [MySQL] 49,343 pass(es). View
#21 1843034-twig-settings-20.patch2.36 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] 49,262 pass(es), 3 fail(s), and 5 exception(s). View
#18 1843034-twig-settings-17.patch2.81 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#14 drupal-1843034-14.patch4.61 KBc4rl
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#10 drupal-make-twig-settings-configurable--1843034-10.patch4.57 KBsteveoliver
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#4 autorefresh-debug-twig-2.patch6.46 KBjaperry
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch autorefresh-debug-twig-2.patch. Unable to apply patch. See the log in the details link for more information. View
#1 autorefresh-debug-twig.patch4.79 KBjaperry
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch autorefresh-debug-twig.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

japerry’s picture

FileSize
4.79 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch autorefresh-debug-twig.patch. Unable to apply patch. See the log in the details link for more information. View

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.

japerry’s picture

Status: Active » Needs review
Fabianx’s picture

Status: Needs review » Needs work

Per IRC: In twig.engine please do output the <!-- only if debug_verbose is set. Thanks!

japerry’s picture

FileSize
6.46 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch autorefresh-debug-twig-2.patch. Unable to apply patch. See the log in the details link for more information. View

add the verbose flag check before rendering the twig templates.

Incorporates changes from the first patch, use this one instead.

c4rl’s picture

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"

Fabianx’s picture

Status: Needs work » Reviewed & tested by the community

Please commit.

Fabianx’s picture

Status: Reviewed & tested by the community » Fixed
Fabianx’s picture

Issue tags: +Twig engine

Tagging for move to core with "Twig engine"

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

steveoliver’s picture

Title: Make reload mode dependent on "production mode" setting » Make Twig settings configurable
Project: » Drupal core
Version: » 8.x-dev
Component: Twig engine (twig_engine branch) » theme system
Assigned: japerry » steveoliver
Status: Closed (fixed) » Needs review
Issue tags: +Twig
FileSize
4.57 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Moving this from Twig sandbox to core queue. Attached patch adds admin settings for Twig cache, debug, and autorefresh options.

Status: Needs review » Needs work

The last submitted patch, drupal-make-twig-settings-configurable--1843034-10.patch, failed testing.

steveoliver’s picture

+++ 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?

steveoliver’s picture

I'm guessing this is related to #1873442: Refactored core Twig integration

c4rl’s picture

Status: Needs work » Needs review
FileSize
4.61 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Let's try this one

c4rl’s picture

Status: Needs review » Needs work

Nevermind, sorry.

heyrocker’s picture

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

c4rl’s picture

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?

heyrocker’s picture

Status: Needs work » Needs review
FileSize
2.81 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

This is untested and the comments need help, but conceptually this is how it should look.

heyrocker’s picture

Also, while I saw where it gets set, it appears the the verbose setting doesn't actually get checked or used anywhere?

Status: Needs review » Needs work

The last submitted patch, 1843034-twig-settings-17.patch, failed testing.

heyrocker’s picture

Status: Needs work » Needs review
FileSize
2.36 KB
FAILED: [[SimpleTest]]: [MySQL] 49,262 pass(es), 3 fail(s), and 5 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 1843034-twig-settings-20.patch, failed testing.

steveoliver’s picture

@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.

steveoliver’s picture

FileSize
4.7 KB
PASSED: [[SimpleTest]]: [MySQL] 49,343 pass(es). View

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.

steveoliver’s picture

Status: Needs work » Needs review
heyrocker’s picture

+++ 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.

heyrocker’s picture

Status: Needs review » Needs work
Cottser’s picture

Assigned: steveoliver » Cottser

Going to try writing some tests for this.

Fabianx’s picture

Assigned: Cottser » steveoliver
+++ 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!

Cottser’s picture

Assigned: steveoliver » Cottser

Crosspost :)

steveoliver’s picture

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.

Cottser’s picture

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.

Fabianx’s picture

+++ 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.

steveoliver’s picture

+++ 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

steveoliver’s picture

test that our FILE NAME SUGGESTIONS match our theme_hook_suggestions

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.

Cottser’s picture

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'd edit to say:

"Render twig template."

I went with "Renders a Twig template.", per http://drupal.org/node/1354#functions.

Do we like our debug output?

Looks good to me, but discussing that would be better handled in another issue IMO.

Lastly, I think the settings should be in this order (logical to me): Cache, Auto-Reload, Debug

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.

Cottser’s picture

Status: Needs work » Needs review
FileSize
1.58 KB
7.19 KB
FAILED: [[SimpleTest]]: [MySQL] 48,718 pass(es), 1 fail(s), and 0 exception(s). View

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.

Cottser’s picture

Almost forgot, huge thanks to @sun for telling me about $this->rebuildContainer() in #drupal-contribute.

Status: Needs review » Needs work

The last submitted patch, 1843034-37.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing
FileSize
3.29 KB
8.56 KB
PASSED: [[SimpleTest]]: [MySQL] 49,079 pass(es). View
3.41 KB
FAILED: [[SimpleTest]]: [MySQL] 49,163 pass(es), 2 fail(s), and 0 exception(s). View

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 a drupalGet(), 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.

Cottser’s picture

Status: Needs review » Needs work

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.

Cottser’s picture

Status: Needs work » Needs review

#40: 1843034-40.patch queued for re-testing.

Cottser’s picture

FileSize
2.09 KB
9.03 KB
PASSED: [[SimpleTest]]: [MySQL] 49,057 pass(es). View
3.88 KB
FAILED: [[SimpleTest]]: [MySQL] 48,962 pass(es), 4 fail(s), and 2 exception(s). View

Updated per #41, looking for 3 failures in the test only patch, one for each setting.

Cottser’s picture

Issue tags: -Twig, -Twig engine
FileSize
2.8 KB
9.62 KB
PASSED: [[SimpleTest]]: [MySQL] 49,051 pass(es). View
4.28 KB
FAILED: [[SimpleTest]]: [MySQL] 49,133 pass(es), 4 fail(s), and 0 exception(s). View

After talking to @Fabianx, twig_auto_reload should default to NULL so that auto_reload is enabled when debug is enabled. This makes our implementation match Twig's: http://twig.sensiolabs.org/doc/api.html#environment-options

Updated the default, added some more test coverage, and updated/tweaked default.settings.php to reflect this change.

Cottser’s picture

Issue tags: +Twig, +Twig engine

Restoring disappearing tags.

Fabianx’s picture

#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.

Cottser’s picture

FileSize
525 bytes
9.67 KB
PASSED: [[SimpleTest]]: [MySQL] 49,093 pass(es). View

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.

steveoliver’s picture

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?

Cottser’s picture

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.

steveoliver’s picture

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.

steveoliver’s picture

Issue tags: +Needs manual testing

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.

Cottser’s picture

FileSize
2.47 KB
7.19 KB
PASSED: [[SimpleTest]]: [MySQL] 49,066 pass(es). View

Just updated the issue summary, here is a revised patch without the additional debug output.

Cottser’s picture

Issue tags: -Needs manual testing

Per #51, and since we're no longer worrying about the debug output in this issue, I think the test coverage is plenty.

Cottser’s picture

Issue tags: -Needs manual testing
FileSize
903 bytes
7.19 KB
PASSED: [[SimpleTest]]: [MySQL] 49,081 pass(es). View

Don't mind me, just fixing the name of the theme_test template file in a comment.

Cottser’s picture

Issue summary: View changes

Adding issue summary

Cottser’s picture

Issue summary: View changes

Updating remaining tasks

Cottser’s picture

Issue summary: View changes

Updated issue summary.

Fabianx’s picture

+++ 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

Fabianx’s picture

+++ 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?

Cottser’s picture

FileSize
641 bytes
7.23 KB
PASSED: [[SimpleTest]]: [MySQL] 49,107 pass(es). View

#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.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, was tested manually, docs are correct now => RTBC

catch’s picture

Status: Reviewed & tested by the community » Fixed

There was a chmod change in the patch which I think I avoided. Committed/pushed to 8.x.

Cottser’s picture

@catch: Thanks for noticing my accidental chmod, and thanks for the commit!

Cottser’s picture

Status: Fixed » Active

I just noticed the added test was missed in the commit:
http://drupalcode.org/project/drupal.git/commit/c05b5bf

Cottser’s picture

Assigned: Cottser » Unassigned
Status: Active » Reviewed & tested by the community

Back to RTBC based on #58 to get the test committed :)

Cottser’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.5 KB
11.15 KB
PASSED: [[SimpleTest]]: [MySQL] 50,576 pass(es). View

Slight tweak of the test to use the twig_extension() function.

Cottser’s picture

FileSize
4.32 KB
PASSED: [[SimpleTest]]: [MySQL] 50,532 pass(es). View
Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

More tests are seldom a bad thing, they pass as well => RTBC

podarok’s picture

#63 looks good
+1 RTBC

Cottser’s picture

The patch in #64 is the patch to look at, even though I didn't rename the patch file.

dags’s picture

podarok’s picture

Status: Reviewed & tested by the community » Fixed

#68 looks good for me

Cottser’s picture

Status: Fixed » Reviewed & tested by the community

Test in #64 still needs to be committed :)

podarok’s picture

woops
cross post
this #64 still needs commit

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Wow, that was off my radar, sorry!

Committed and pushed to 8.x. Yay, tests. :D

xjm’s picture

Title: Make Twig settings configurable » [HEAD BROKEN] Make Twig settings configurable
Category: task » bug
Priority: Normal » Critical
Status: Fixed » Active

TwigSettingsTest is 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
webchick’s picture

Oops. :) I'll get this in about a half hour when I get home.

webchick’s picture

Title: [HEAD BROKEN] Make Twig settings configurable » Make Twig settings configurable
Category: bug » task
Priority: Critical » Normal
Status: Active » Needs work

Ok, should be fixed up now. We'll need a (hopefully quick) re-roll of this one.

Cottser’s picture

Assigned: Unassigned » Cottser

I'm on it.

Cottser’s picture

Assigned: Cottser » Unassigned
Status: Needs work » Needs review
FileSize
712 bytes
4.35 KB
PASSED: [[SimpleTest]]: [MySQL] 53,830 pass(es). View
xjm’s picture

Status: Needs review » Reviewed & tested by the community

Oh, it was a simple thing after all. :)

RTBC if it passes.

podarok’s picture

ufo cleaned

podarok’s picture

#77 +1 RTBC

xjm’s picture

Issue tags: -Twig, -Twig engine

#77: 1843034-77-test.patch queued for re-testing.

Fabianx’s picture

Issue tags: +Twig, +Twig engine

#77: 1843034-77-test.patch queued for re-testing.

Cottser’s picture

Title: Make Twig settings configurable » (Tests for) Make Twig settings configurable
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed d569e20 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Cottser’s picture

Title: (Tests for) Make Twig settings configurable » Make Twig settings configurable

Reverting title for posterity.

Cottser’s picture

Issue summary: View changes

Better wording.

Michelle’s picture

Adding a note in case anyone else ends up confused like me. The changes here to settings.php moved over to example.settings.local.php in #2226761: Change all default settings and config to fast/safe production values