Changing follow vars, you will not see it take effect immediately because the settings was cached. We need to tell themers to clear the caches after the setting changes.

$settings['twig_debug'] = TRUE;
$settings['twig_auto_reload'] = TRUE;
$settings['twig_cache'] = FALSE;
Files: 
CommentFileSizeAuthor
#34 interdiff.txt1.18 KBOuti
#34 ImproveTwigVarsDocsInSettingsDotPHP-34.patch2.39 KBOuti
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,728 pass(es). View
#30 twig_debug-cache_clear-2056879-30-do-not-test.patch1.18 KBCottser
#28 interdiff.txt556 bytesOuti
#28 ImproveTwigVarsDocsInSettingsDotPHP-6.patch1.23 KBOuti
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,766 pass(es). View
#25 interdiff.txt532 bytesOuti
#25 ImproveTwigVarsDocsInSettingsDotPHP-5.patch1.21 KBOuti
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#17 ImproveTwigVarsDocsInSettingsDotPHP-4.patch904 bytesmartin107
PASSED: [[SimpleTest]]: [MySQL] 58,011 pass(es). View
#11 ImproveTwigVarsDocsInSettingsDotPHP-3.patch1.19 KBmartin107
PASSED: [[SimpleTest]]: [MySQL] 57,650 pass(es). View
#11 interdiff.txt845 bytesmartin107
#8 ImproveTwigVarsDocsInSettingsDotPHP-2.patch1.18 KBmartin107
PASSED: [[SimpleTest]]: [MySQL] 57,656 pass(es). View
#8 interdiff.txt607 bytesmartin107
#4 ImproveTwigVarsDocsInSettingsDotPHP.patch1.18 KBmartin107
PASSED: [[SimpleTest]]: [MySQL] 57,633 pass(es). View

Comments

jhodgdon’s picture

Status: Active » Postponed (maintainer needs more info)

It is certainly true for more than just Twig settings... Do we make similar "clear the cache" statements for other settings?

droplet’s picture

Status: Postponed (maintainer needs more info) » Active

AFAIK, We do not need to clear cache for other settings.

jhodgdon’s picture

Issue tags: +Novice

OK then, just needs a patch.

martin107’s picture

Assigned: Unassigned » martin107
FileSize
1.18 KB
PASSED: [[SimpleTest]]: [MySQL] 57,633 pass(es). View

Sample Patch as requested

martin107’s picture

Status: Active » Needs review
martin107’s picture

Assigned: martin107 » Unassigned
jhodgdon’s picture

Status: Needs review » Needs work

Thanks! Can we get a bit of a grammar update?

+ * Note changes to this setting will only take effect once the cache is cleared.

Either follow Note with a : or the word "that". Thanks!

martin107’s picture

FileSize
607 bytes
1.18 KB
PASSED: [[SimpleTest]]: [MySQL] 57,656 pass(es). View

Added colon.

martin107’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Thanks! Unfortunately, that makes the lines longer than 80 characters, so they need to be wrapped.

martin107’s picture

FileSize
845 bytes
1.19 KB
PASSED: [[SimpleTest]]: [MySQL] 57,650 pass(es). View

Corrected wrapping and trailing whitespace issue.

martin107’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Issue tags: +Needs manual testing

The documentation looks good to me.

I am still not certain that it is accurate, so I am not setting to RTBC until someone can verify that:
- If you change any of these settings and do not clear the cache, they do not take effect.
- If you then clear the cache, the setting does take effect.
This will probably require manual testing and each setting should be tested individually.

ioskevich’s picture

Assigned: Unassigned » ioskevich
ioskevich’s picture

Issue tags: +CodeSprintCIS

Adding CodeSprintCIS tag.

ioskevich’s picture

Assigned: ioskevich » Unassigned
Status: Needs review » Needs work

Precondition:

Clean Drupal 8 install. Logged in as UID 1, Drupal page cache disabled.
Tested markup changes on home page, modifications were done to /core/themes/bartik/templates/page.html.twig

1. $settings['twig_debug']

$settings['twig_auto_reload'] and $settings['twig_cache'] set to defaults (commented out).

Changing from FALSE to TRUE and vice-versa triggers output of TWIG debug (HTML comments injected into resulting page markup) immediately on page reload, no Drupal cache flush required.

Summary: cache flush IS NOT required to make this setting to work.

2. $settings['twig_auto_reload']

Tested with $settings['twig_debug'] set to FALSE, $settings['twig_cache'] = TRUE;

2.1 $settings['twig_auto_reload'] set to FALSE

  • 2.1.1 Make changes to page.html.twig -> no changes to resulting page markup
  • 2.1.2 CCA -> Latest template changes appear in resulting markup
  • 2.1.3 Make changes to page.html.twig -> no changes to resulting page markup

2.2 $settings['twig_auto_reload'] set to TRUE

  • 2.1.1 Make changes to page.html.twig -> no changes to resulting page markup
  • 2.1.2 CCA -> Latest template changes appear in resulting markup
  • 2.1.3 Make changes to page.html.twig -> template changes appear in resulting markup

Summary: cache flush IS required to make this setting changes to apply.

3. $settings['twig_cache']

$settings['twig_debug'], $settings['twig_auto_reload'] set to default values (commented out).
Monitoring twig cache directory: /sites/default/files/php

3.1 $settings['twig_cache'] set to TRUE

  • 3.1.1 /sites/default/files/php/twig exists
  • 3.1.2 CCA -> /sites/default/files/php/twig exists

3.2 $settings['twig_cache'] set to FALSE

  • 3.2.1 /sites/default/files/php/twig exists
  • 3.2.2 CCA -> /sites/default/files/php/twig disappears
  • 3.2.3 Page reload -> /sites/default/files/php/twig still doesn't exist

Summary: cache flush IS required to make this setting changes to apply.

martin107’s picture

Status: Needs work » Needs review
FileSize
904 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,011 pass(es). View

Thanks Ioskevich, here is a patch that reflects your advice.

droplet’s picture

Status: Needs review » Needs work

1. $settings['twig_debug']

This one included the DUMP function, can you re-test again? Thanks.

ioskevich’s picture

Assigned: Unassigned » ioskevich
ioskevich’s picture

Assigned: ioskevich » Unassigned
Issue tags: -Novice, -Needs manual testing

Changing value of $settings['twig_debug']:

Debug markup is added/removed WITHOUT clearing cache.

To use dump() function, cache clear IS REQUIRED.

Might make sense to change help text for $settings['twig_debug'] in settings.php to reflect this?

ioskevich’s picture

Issue tags: +Novice, +Needs manual testing

Lost original tags somehow - re-adding.

jhodgdon’s picture

Issue tags: -Needs manual testing

I think we can get rid of the 'needs manual testing' tag at this point, because (THANK YOU!) ioskevitch tested it. :)

Cottser’s picture

Issue summary: View changes
Issue tags: +Twig

Just came across this so tagging as 'Twig' to hopefully get some eyes on it :)

Cottser’s picture

Issue tags: +documentation

I know the documentation tag is redundant but I'm just adding it so that this issue shows up on http://drupaltwig.org/issues/documentation which I just set up, hope that doesn't mess anyone up.

Outi’s picture

FileSize
1.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
532 bytes

I tested using the dump() function with the $settings['twig_debug'] set to TRUE, and the cache clear is indeed required as ioskevich pointed out in the comment #20. I changed the twig_debug help text to note this.

Outi’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 25: ImproveTwigVarsDocsInSettingsDotPHP-5.patch, failed testing.

Outi’s picture

Status: Needs work » Needs review
FileSize
1.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,766 pass(es). View
556 bytes

The last patch wasn't the right one, this one includes the changes I wanted to make.

Cottser’s picture

Thanks @Outi! It gets a bit more complicated because twig_auto_reload depends on twig_debug in a way: http://twig.sensiolabs.org/doc/api.html#environment-options

I would suggest we say that dump() and automatic reloading need a cache clear to take effect, and if we want to be really explicit we can say debug markup will show up immediately. Maybe we can split up this list into two sections and state that the second part (dump and auto_reload) need a cache clear to take effect?

 * When debugging is enabled:
 * - The markup of each Twig template is surrounded by HTML comments that
 *   contain theming information, such as template file name suggestions.
 * - Note that this debugging markup will cause automated tests that directly
 *   check rendered HTML to fail. When running automated tests, 'twig_debug'
 *   should be set to FALSE.
 * - The dump() function can be used in Twig templates to output information
 *   about template variables.
 * - Twig templates are automatically recompiled whenever the source code
 *   changes (see twig_auto_reload below).

Also for what it's worth it should be possible to make the debug markup only show up after cache clear. My first impression is that seems a bit undesirable but it would make things consistent and easier to document :)

Cottser’s picture

This is the basic idea of how the last paragraph of my last comment would work - need to determine if this would still be testable but I'd like some feedback as to what folks think about this. To me it seems good that the debug markup would be consistent with dump() and auto reload, otherwise you can get into a state where things seem "half working".

Steps to test:

  1. Apply the patch: https://drupal.org/patch/apply
  2. Play around with twig_debug in settings.php, you should have to clear cache to show/hide the debug markup.
Outi’s picture

Cottser, I tested your patch and it might indeed be a good idea to require the cache clear everywhere. In that case I'll change the help text.

By the way, it seems to me that this point at the $settings['twig_debug'] help text is duplicating what follows in the help text of $settings['twig_auto_reload']:

 * - Twig templates are automatically recompiled whenever the source code
 *   changes (see twig_auto_reload below).

Help text of $settings['twig_auto_reload']:

 * Automatically recompile Twig templates whenever the source code changes. If
 * you don't provide a value for twig_auto_reload, it will be determined based
 * on the value of twig_debug.

And should the default value mentioned on the help text of $settings['twig_auto_reload'] be FALSE (as elsewhere) and not NULL ?

* Not recommended in production environments (Default: NULL).

Cottser’s picture

Title: Improve twig_* vars DOCs in settings.php » Improve twig_* vars DOCs in settings.php and make Twig debug markup require a cache clear
Component: documentation » theme system

Thanks @Outi! I think if we combine the patches from #25 and #30 then we've covered our bases pretty well here (but I'm happy to be proven wrong!). Moving to the theme system component (we still have the 'documentation' tag) to account for the functional change.

The duplication in twig_debug help text is intentional because there is a common misunderstanding that twig_auto_reload is needed to allow for automatic template recompilation. The default of NULL is true because if twig_auto_reload is left NULL it inherits the value of twig_debug.

The short version is: as a themer all you really need is twig_debug = TRUE 99% of the time.

You can see where the Twig service is set up in \Drupal\Core\CoreServiceProvider::registerTwig():

  /**
   * Registers Twig services.
   *
   * This method is public and static so that it can be reused in the installer.
   */
  public static function registerTwig(ContainerBuilder $container) {
    $container->register('twig.loader.filesystem', 'Twig_Loader_Filesystem')
      ->addArgument(DRUPAL_ROOT);
    $container->setAlias('twig.loader', 'twig.loader.filesystem');

    $container->register('twig', 'Drupal\Core\Template\TwigEnvironment')
      ->addArgument(new Reference('twig.loader'))
      ->addArgument(array(
        // This is saved / loaded via drupal_php_storage().
        // All files can be refreshed by clearing caches.
        // @todo ensure garbage collection of expired files.
        // When in the installer, twig_cache must be FALSE until we know the
        // files folder is writable.
        'cache' => drupal_installation_attempted() ? FALSE : Settings::get('twig_cache', TRUE),
        // @todo Remove in followup issue
        // @see http://drupal.org/node/1712444.
        'autoescape' => FALSE,
        'debug' => Settings::get('twig_debug', FALSE),
        'auto_reload' => Settings::get('twig_auto_reload', NULL),
      ))
      ->addArgument(new Reference('module_handler'))
      ->addArgument(new Reference('theme_handler'))
      ->addMethodCall('addExtension', array(new Definition('Drupal\Core\Template\TwigExtension')))
      // @todo Figure out what to do about debugging functions.
      // @see http://drupal.org/node/1804998
      ->addMethodCall('addExtension', array(new Definition('Twig_Extension_Debug')));
  }
Cottser’s picture

Oh, and also expect tests to fail with the change made in twig.engine if you post a combined patch, those will need some reworking.

Outi’s picture

FileSize
2.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,728 pass(es). View
1.18 KB

Here is a patch that combines the patches of the comment #25 and the comment #30. The interdiff file is between the patch of the comment #25 and this new patch.

Cottser’s picture

I'm slightly surprised that tests pass but great :D Thanks @Outi!

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, nice work. Makes sense and if anyone wants to have things take immediate effect, they would need to disable the container caching by using e.g. a read-only filesystem class for the PHPStorage (not that anyone would want to).

Also reduces dependencies and keeps things with in the service.

Full win!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Cool, seems like this will save some poor themers some face-desking.

Committed and pushed to 8.x. Thanks!

  • Commit 0526492 on 8.x by webchick:
    Issue #2056879 by martin107, Outi, ioskevich, Cottser: Improve twig_*...

Status: Fixed » Closed (fixed)

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