Improve twig_* vars DOCs in settings.php and make Twig debug markup require a cache clear

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

OK then, just needs a patch.

Sample Patch as requested

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!

Added colon.

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

Corrected wrapping and trailing whitespace issue.

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.

Adding CodeSprintCIS tag.

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.

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

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?

Lost original tags somehow - re-adding.

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

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

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.

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.

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

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 :)

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.

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

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')));
  }

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.

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.

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

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!

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

Committed and pushed to 8.x. Thanks!