Problem/Motivation

Developers routinely enable settings for Twig development by editing development.services.yml. However this is not convenient as it gets overwritten from core scaffold files with each Drupal composer update. We should provide a better way to do it.

There are 3 relevant Twig settings.

  1. auto_reload: always useful in development
  2. debug: often useful, but causes major bugs on some sites, see #2914733: [META] twig debug can break parts of site
  3. cache: should always be enabled even in development. This setting is already enabled by default so no changes are needed.

Proposed resolution

  1. Add auto_reload to development.services.yml because it's always useful in development.
  2. Create a new file twig-debug.services.yml specifically for the Twig debug setting.
  3. Reference this new services file in sites/example.settings.local.php, below the line of code where development.services.yml is referenced. Leave the reference commented out with a note explaining the potential dangers and implications of enabling theme debugging.
    /**
     * Enable Twig debugging services.
     *
     * Enabling Twig debugging is not recommended in production environments and
     * can break parts of Drupal. See https://drupal.org/i/2914733 for details.
     */
    $settings['container_yamls'][] = DRUPAL_ROOT . '/sites/twig-debug.services.yml';
    

    For the website themer, this code is more or less analogous to adding $config['theme_debug'] = TRUE; option in Drupal 7 settings.php file. Optionally, a CR could be added to describe this difference between D7 / D8.

Issue fork drupal-2839709

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

gpap created an issue. See original summary.

gpap’s picture

Issue summary: View changes
gpap’s picture

Thew’s picture

I agree with you. I need to add it manually every time I get the fresh installation.

Maybe add the twig parameters to development.services.yml and comment it out.
If themers want it, he can remove the comment, like Disable the render cache in example.settings.local.php.

# Local development services.
#
# To activate this feature, follow the instructions at the top of the
# 'example.settings.local.php' file, which sits next to this file.
parameters:
  http.response.debug_cacheability_headers: true
  # twig.config:
  #   debug: true
  #   auto_reload: true
  #   cache: false
services:
  cache.backend.null:
    class: Drupal\Core\Cache\NullBackendFactory

However, if my thought is right, you can only set debug: true. The auto_reload and cache will automatically follow the debug parameter. See Debugging compiled Twig templates

aadil.addweb’s picture

Status: Active » Needs review
StatusFileSize
new492 bytes

Please apply disable_twig_cache_by-2839709-5.patch.

star-szr’s picture

Status: Needs review » Needs work

Thanks for the suggestion. I'm not sure if this was discussed before, there may have been a reason to not include it. Enabling twig debug can have side effects, because of the HTML comments added. For example, #2656728: Twig split in debug mode. Enabling auto_reload for development should be safe though.

Disabling twig cache in my experience has no benefits, only downsides:

  1. Harder to debug compiled templates because they aren't saved to disk.
  2. Templates need to be compiled each time no matter what, slowing things down.

See https://github.com/hechoendrupal/drupal-console/pull/1310 from the Drupal Console project where something similar came up.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

I'm on board with the this except cache like @Cottser mentioned in #6. And with debug on, auto_reload is on by default so doesn't need to be added, though that's not too much a concern.

Would you mind removing those from the patch @binoli.addweb?

drupalmonkey’s picture

StatusFileSize
new450 bytes

Updated the patch as per #9.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Anonymous’s picture

#9 looks ok, thanks! But #6 also makes sense.

What if we add these lines with comment? This will help not to find the examples of disabling twig cache, just uncomment lines.
And this will help to prevent an incorrect insertion of one more parameters:. Because after #2827047: Add http.response.debug_cacheability_headers: true to development.services.yml it is not works, but still as recommended in many tutorials.

I am also in favor of saving the auto_reload, since it will be easier to set 'false' if desired.

Anonymous’s picture

Status: Needs work » Needs review
jwilson3’s picture

Title: Disable TWIG cache by default in development.services.yml » Enable Twig debug by default in development.services.yml
StatusFileSize
new426 bytes
new1.65 KB
new496 bytes
new1.7 KB

Updating title to reflect the last point on comment #4.

I'm in favor of putting these values into the development.services.yml file as well as having commented out versions of twig_auto_reload and twig_cache. But I also think that this is sufficiently confusing that we should include some of the documentation from Debugging Complied Twig Templates inline for immediate reference and warning (see patch 13b).

Here are two patches, one with and one without documentation, for your consideration.

Also please be aware that making this change, will make our documentation more complex for front end developers. The reason is that it will be more complex than simply adding a couple lines to services.yml file (per current documentation).

1) Copy example.settings.local.php to settings.local.php
2) Uncomment the line in settings.local.php that enables development.services.yml
3) Confirm that parameters.twig.debug is set to true in development.services.yml and that the line is not commented out.

Furthermore, we'll possibly need a change notice. Any front end developer that has been putting these settings in services.yml as per the current documentation page suggests, but then also has development.services.yml enabled (either unwittingly, or from instructions from a backend developer) will not know that the values in services.yml will now be overridden by those in development.services.yml, creating confusion. (Anecdotally, this just happened to a front-end developer on our team last week after a backend dev hardcoded debug: false in development (!!!) the debug: true in services.yml was being ignored completely, causing some moments of confusion).

Here are three documentation pages that mention this that I know of, there may be more:

https://www.drupal.org/docs/8/theming/twig/debugging-twig-templates
https://www.drupal.org/docs/8/theming/twig/debugging-compiled-twig-templ...
https://www.drupal.org/node/2598914

adamps’s picture

Status: Needs review » Needs work

I have created a META issue to list all of the known problems of twig debug: #2914733: [META] twig debug can break parts of site.

  • I propose that we link to this from development.services.yml.
  • Given so many potential problems I wonder if we really ought to turn twig debug on by default. Perhaps we should instead turn auto_reload on by default and have the line for twig_debug present but commented out?

A secondly benefit of the direction this issue is taking us is that we can subsequently use development.services.yml to solve some of these issues. For example to solve #2796453: RSS output not valid when using Twig debug, there could be a new response_filter.rss.twig_debug and class RssResponseTwigDebugFilter implements EventSubscriberInterface that fixes the feed by moving the comments after the xml tag.

adamps’s picture

On reflection I feel that we should avoid commented out lines in development.services.yml. That file is shipped in Drupal core and gets overridden every upgrade, so users should not be editing it. Instead I suggest:

  • Add twig auto_reload to development.services.yml
  • New file twig-debug.services.yml sets twig debug and in future could have extra lines to workaround bugs that introduces
  • example.settings.local.php contains a commented out line to include twig-debug.services.yml
  • As already documented, users would copy example.settings.local.php to settings.local.php. They can then uncomment the line - which is safe because they are not editing a delivered file.
willthemoor’s picture

Wondering if a better longer(ish) term move might be to (automagically) allow for development.services.local.yml (or is it local.development.services.yml)? That is, Drupal would use it if available instead of needing to go to a settings file an uncomment.

Then a user can create it and it could be .gitignored. As it is now, using this on teams is a little squirrelly.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

norman.lol’s picture

Priority: Minor » Normal

Raising priority due to inactivity. Every time I start a fresh Drupal I wonder why this isn't there. It's the development.services.yml. And this should be the default during development.

agileadam’s picture

You have another vote (me) for putting this in development.services.yml out-of-the-box.

baluertl’s picture

Plus one vote on enabling this option by default out-of-the-box.

adamps’s picture

Note to commenters #21, #22, #23 - please read #14, #15. Enabling twig debug introduces multiple bugs several of them Major, see #2914733: [META] twig debug can break parts of site. If we commit this change, it won't be possible to test those features on a dev site, and people could spend hours trying to work out why.

Therefore I think the best we can do is to make it easier to enable twig debug with a persistent setting that doesn't get overridden every time you update core.

jwilson3’s picture

Status: Needs work » Needs review
StatusFileSize
new1.25 KB

Here is comment #15 in patch format.

1) Create a file at sites/theme-development.services.yml I decided against calling it twig-debug.services.yml, because as mentioned, the file could potentially do more than just Twig debugging in the future. (This naming choice would be analagous to having called development.services.yml something like "cache-disable.services.yml".)

2) Reference the new services file in example.settings.local.php, next to the line of code where development.services.yml is referenced, but leave it commented out, with a note explaining the potential dangers and implications of enabling theme debugging.

Note: I've decided against enabling auto_reload in development.services.yml because I'm yet to be convinced this is actually necessary. My reading of the documentation of these parameters in sites/default/default.services.yml leads me to understand that when left null (which is default), the auto_reload value will be determined automatically based on the value of twig_debug. This seems like a logical behavior.

jwilson3’s picture

Title: Enable Twig debug by default in development.services.yml » Provide theme-development.services.yml to make twig_debug easier

Status: Needs review » Needs work

The last submitted patch, 25: 2839709-25-theme-development-services.patch, failed testing. View results

adamps’s picture

@jwilson3 thanks looks good to me - although I haven't tested it

  • I agree with your improved choice of the file name.
  • As per #15, I propose that we could add twig auto_reload to development.services.yml. That setting is safe and seems generally recommended for any dev site.
  • We should update the issue summary to match the new proposed resolution.

I removed some tags that look like "adding random keywords". If anyone disagrees, apologies, please feel free to put them back.

jwilson3’s picture

Issue summary: View changes
Status: Needs work » Needs review

As per #15, I propose that we could add twig auto_reload to development.services.yml. That setting is safe and seems generally recommended for any dev site.

I apologize @AdamPS. I don't fully understand why we need to explicitly set auto_reload to anything specific in development.services.yml whenthe default (recommended) value is NULL anyway? As per existing docs, auto_reload value knows how to be set correctly based on the value of twig_debug so shouldn't this parameter just be left out of development.services.yaml so as not to confuse people or pollute development services file with theme-related parameters that shouldn't really ever need to be changed? Can you maybe explain your stance on this again in another way, if you still feel strongly about it?

We should update the issue summary to match the new proposed resolution.

Done. I also mentioned that for the website themer, the code addition in example.settings.local.php is more or less analogous to adding $config['theme_debug'] = TRUE; option in Drupal 7 settings.php file. So possibly it makes sense that the CR would describe this difference between D7 / D8, as many other CRs follow a similar documentation pattern.

#25 failed because of this error:

Scaffold source and destination files must have the same contents.
assets/scaffold/files/example...al.php

I don't know how to resolve this error in the patch itself. My guess is that this is a newish bug with patches that modify the scaffold files that has appeared recently since the drupal/recommended-project composer template has been released for 8.8+ Maybe we have to file a bug report, to the drupal.org issue queue? I'm not sure. Will try to set back to needs review and cross fingers.

jwilson3’s picture

Status: Needs review » Needs work

The last submitted patch, 25: 2839709-25-theme-development-services.patch, failed testing. View results

adamps’s picture

As per #15, I propose that we could add twig auto_reload to development.services.yml. That setting is safe and seems generally recommended for any dev site.

Perhaps I was too hasty in supporting your change of filename. It seems to me crucial that we first select the right functional behavior / user experience then secondly choose the filenames (rather than let the filenames force the functional behavior).

I propose that we should have the absolute minimum number of services files. It's easy to understand the currently checked in code which is a single file, development.services.yml. This allows for two types of site: live and dev. The only reason that we are moving away from that is because of the twig_debug setting where there is no single value that is correct for all dev sites.

  1. When you are trying to understand which template file causes each part of the HTML then it's good to have the setting enabled.
  2. If you are developing a part of the website that would be broken by twig_debug then you need to have twig_debug disabled, even if you are doing theme development of that part of the website.

Therefore this setting needs to have its own file which allows people to choose to enable or disable as they prefer. Bearing in mind the bold-italic text in bullet 2 above then my current thinking is that we should call it twig-debug.services.yml.

On the other hand, the setting auto_reload should be enabled for all dev sites, because there is no disadvantage (other than performance which is irrelevant in dev). Hence it belongs in development.services.yml.

adamps’s picture

I believe that the test failure is a real problem that requires the patch to be altered.

There are a few files that have a more complicated situation because they are part of the Drupal Core GIT repository, but don't live in the core directory on live Drupal site. These are now called scaffold files and they appear to end up being in GIT in two different places: sites/development.services.yml and core/assets/scaffold/files/development.services.yml. My guess is that this patch needs to change both files - and likely also two places for the newly created file.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Spokje made their first commit to this issue’s fork.

spokje’s picture

Used 2839709-25-theme-development-services.patch as a base for the new MR against 9.3.x.
Hidden all the (now outdated) previous patch files.

spokje’s picture

Status: Needs work » Needs review

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

damienmckenna’s picture

I opened #3272587 to accomplish the same effect but using a different approach. FWIW it doesn't work because the changes in development.services.yml don't override the default settings in services.yml, which seems like a core bug.

jwilson3’s picture

Status: Needs review » Needs work

On the other hand, the setting auto_reload should be enabled for all dev sites, because there is no disadvantage (other than performance which is irrelevant in dev). Hence it belongs in development.services.yml.

I'm not sure I agree. If you're not explicitly editing twig files, there is no reason to enable this setting. Both "debug" and "auto_reload" afaict, go together always. Furthermore, why cause non-themer devs unnecessary performance costs on their local by default? This is totally unnecessary if you're doing Decoupled Drupal. $conf['theme_debug'] is commented out in default.settings.php for Drupal 7.

scaffold files [...] appear to end up being in GIT in two different places: sites/development.services.yml and core/assets/scaffold/files/development.services.yml. My guess is that this patch needs to change both files - and likely also two places for the newly created file.

Patch in #25 has no effect on development.services.yml, but I'm setting to NW because a new scaffold file need to be created for theme-development.services.yml and the scaffold for sites/example.settings.local.php needs to be updated to match.

jwilson3’s picture

Status: Needs work » Needs review
StatusFileSize
new2.58 KB
new1.36 KB

Patch adds scaffold files.

jwilson3’s picture

AFAICT, Patch in #44 and issue #3272587: Enable theme debugging by default in development.services.yml are two very similar solutions to the same problem.

The key difference being that this issue presumes theme debugging should be off by default — just as Drupal 7 assumed — and should be independently togglable of other backend development services such as caching. Whereas #3272587 presumes theme debugging should be on by default for all developers. However, this causes problems when a developer wants to turn off theme debugging themselves locally, they'd either have to disable development.services.yml entirely, OR leave development.services.yml that is typically tracked by git in an overridden state. That may be resolved eventually by #3094699: Remove development.services.yml from Drupal's composer scaffolding, but if coupled with #3272587, it will again result in theme debugging being on by default for all devs.

This issue creates a new file at sites/theme-development.services.yml and a new setting in sites/example.settings.local.php, which isolates the frontend theme debugging from other backend development services. My argument here is that not all developers need or want Twig debug enabled on their local environment.

This issue also more closely mimics the existing Drupal 7 workflow whereby frontend developers were expected to uncommenting the # $conf['theme_debug'] = TRUE in sites/files/default.settings.php. In this issue, the approach is very similar; you just uncomment one line in settings.local.php.

Status: Needs review » Needs work

The last submitted patch, 44: 2839709-44-theme-development-services.patch, failed testing. View results

jwilson3’s picture

Status: Needs work » Needs review

The test run failure seems unrelated to this issue.

star-szr’s picture

I just want to mention that setting twig cache to false is generally unhelpful: https://www.drupal.org/docs/theming-drupal/twig-in-drupal/debugging-comp...

Edit: See also my old comment in this issue - #6.

I do agree that "not all developers need or want Twig debug enabled on their local environment".

jwilson3’s picture

StatusFileSize
new2.58 KB
new744 bytes

Cache is now set to `true` to address #48. Thanks for the reference to catch the context from comment #6.

adamps’s picture

Status: Needs review » Needs work

Thanks @jwilson3 for keeping this one moving. The latest patch seems like good progress, and most of my earlier comments have been addressed. I have two comments still:

1) As per #15, I propose that we could add twig auto_reload to development.services.yml. This setting is different from twig debug - it's highly advantageous for development, and there's no real disadvantage other than a perf impact (probably fairly minor - just checking some file times?). We can help all developers without them needing to alter their settings.local.php.

2) I believe there is no need to set auto_reload or cache - those values are already the default. Therefore it's potentially better to leave them out for simplicity and to avoid interfering if someone wants to alter them.

jwilson3’s picture

Status: Needs work » Needs review
StatusFileSize
new4.16 KB
new1.76 KB

I've implemented the changes requested in #50, though I do have my doubts about the approach and fringe benefits of #50.1

IMO it is confusing to put Twig settings in two separate services.yml files. Because then developers will wonder where is the best place to put other Twig-related settings, or they won't notice and they'll think its fine to put their other Twig additions in development.services.yml

IMO it is uncomplicated for any dev working on Twig files to go enable theme-develoment services in their settings.local.php by uncommenting a single line, if they really need that twig_autoload feature. IMO grouping like-with-like would win out over any potential minor advantage to having twig_autoload turned on for all developers.

Let's see what others say.

adamps’s picture

Thanks @jwilson3

I can appreciate your perspective. I guess we each have a different emphasis on the relative importance of different factors, so it would be good to get other opinions.

There is good reason for the separation:

  • autoload should always be on in development, else you can waste time because you altered a template but it wasn't picked up and you don't understand why - I find this a major advantage
  • debug should only be on when you needed because it breaks quite a few things - and if you want the debug comments but they missing, it's obvious
adamps’s picture

they won't notice and they'll think its fine to put their other Twig additions in development.services.yml

I'm curious: is there some reason not to do that?

The setup I have is a file called twig-debug.services.yml containing only debug:true, and everything else in development.services.yml. The second one is always included for dev, the first turned on/off as needed. Of course it's not the only way but it seems to work well.

jwilson3’s picture

they won't notice and they'll think its fine to put their other Twig additions in development.services.yml

What I mean is that someone could add debug: true to development.services.yml seeing that there are other twig elements there, when there is another way to do that. I guess there is nothing we can do to prevent footguns like that.

adamps’s picture

Ah I see what you mean.

I guess there is nothing we can do to prevent footguns like that.

Well we could add a comment to development.services.yml??

I guess I do still prefer the filename twig-debug.services.yml rather than theme-development.services.yml - the file should contain only one the setting. However I'll leave it up to you.

damienmckenna’s picture

FYI the patch doesn't apply using composer, because composer-patches uses the "patch" command to apply patches to projects downloaded using a tarball and the "patch" command doesn't support the "copy" instruction in the patch files.

jwilson3’s picture

Status: Needs review » Needs work

okay. I think the patchfile can be edited to remove the copy. Thanks Damien.

damienmckenna’s picture

Idea: should the existing "development.services.yml" file be renamed to "development.caching.services.yml" and this new one renamed to 'development.theming.services.yml"? That would group the two files together in the directory listing and make it more clear what both files are configured for by default?

jwilson3’s picture

No interdiff because I literally just rebased the feature branch on latest 9.4.x, and regenerated the patch using a similarity index of 0% -M0%

git checkout 9.4.x
git pull --all --prune
git checkout 2839709
git rebase 9.4.x
git diff 9.4.x..2839709 -M0% > 2839709-58-theme-development-services.patch
jwilson3’s picture

Idea: should the existing "development.services.yml" file be renamed to "development.caching.services.yml" and this new one renamed to 'development.theming.services.yml"? That would group the two files together in the directory listing and make it more clear what both files are configured for by default?

This is a really good idea and I don't want to shut it down, but playing devils advocate, a rename of an existing services.yml would affect a lot more things, docs come to mind, and would break existing sites settings.local.php (which are disconnected from git) and could end up breaking local developers codebases because they still reference a now nonexistent develompent.services.yml file. Maybe there are other things too we haven't thought of. A more-versed core contributor/maintainer might be able to better vet an idea like this though.

maybe just renaming "theme-development.services.yml" to "development-theming.services.yml" would achieve the same thing?

jwilson3’s picture

Status: Needs work » Needs review

NR for #59

joachim’s picture

+++ b/core/assets/scaffold/files/theme-development.services.yml
@@ -0,0 +1,9 @@
+    debug: true

+++ b/sites/development.services.yml
@@ -4,6 +4,8 @@
+  twig.config:
+    auto_reload: true

I'm confused why twig parameters are split across two files. Should this be explained in the files?

jwilson3’s picture

#62 is exactly what I speculated would happen in #51, and why my vote still goes towards removing twig.auto_reload: true from the development.services.yml file, because even if it were to have some clarifying comment, telling people where to not put stuff via a comment is the quickest way to get totally ignored. IMO the code should lead by example.

While looking at this, I noticed the existing development.services.yml doesn't have any comments in it to document the other parameter settings. It also doesn't bother to point users to where the parameters are properly documented in default.services.yml. So my immediate sense is that if we add some comment in this file to document this out-of-place parameter, we'll need a follow-up to -- in general -- improve the comments in the file. This furthers my point to leave development.services.yml alone, so we can a) consolidate theme debug settings into one place and b) get this in before bikeshedding to death. The late addition of auto_reload: true could/should(?) be split off to a follow-up because the other part of this issue solves the OP and issue title.

That being said, if someone wants to churn out a comment to clarify why its okay to have twig.auto_reload in development services, but other frontend twig services should stay in theme-development.services.yml, I'll be happy to update the patch.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

adamps’s picture

Issue summary: View changes
Status: Needs review » Needs work

a) consolidate theme debug settings into one place and

@jwilson3 I deduce that your sites are not broken by enabling twig debug so you are lucky. As I have explain several times, some sites have major bugs when twig debug is enabled, please see #2914733: [META] twig debug can break parts of site. I see this is missing from the IS, so I have added it.

On these sites, we do most of our twig development with twig debug off (because it breaks the site) but auto-reload on (essential for efficient development). Then we enable twig debug briefly as we need it.

If an extra parameter is added to the file containing the twig debug setting then it breaks the whole point of this issue and the split into two files.

b) get this in before bikeshedding to death

Perhaps this issue is stuck because the current approach doesn't meet the requirements of a proportion of the community?? Please I urge you again to consider the alternative I have offered that as far as I know meets everybody's needs.

#62 is exactly what I speculated would happen in #51,

Yes I agree the existing file names are confusing. However I feel that the names I propose are quite clear.

jwilson3’s picture

@AdamPS. Thank you for laying out the complete concise argument. I wasn't able to put it all together because, as you deduced, I'm not immediately suffering from any of the issues on #2914733: [META] twig debug can break parts of site. But I can see how this could be problematic.

Remaining tasks include:

  1. a comment to clarify why it is okay to have twig.auto_reload in development services and why not to enable or add twig.config.debug: true in that file, per @joachim in #62 and resolving concerns from @jwilson3 in #54
  2. rename theme-development.services.yml to twig-debug.services.yml, per @AdamPS in #15, #32, #53, #55
  3. consider renaming twig-debug.services.yml to start with the letter "d" to resolve @DamienMcKenna's #58 and response #60.
  4. If this gets in, it will need docs update on https://www.drupal.org/docs/theming-drupal/twig-in-drupal/debugging-twig...
jwilson3’s picture

Status: Needs work » Needs review
StatusFileSize
new5.06 KB
new5.95 KB

The file twig-debug.services.yml being named after what amounts to a single setting feels like a wrong approach. However, on the other hand, I don't see an immediately better approach, so I went with it, and drastically updated the patch (see interdiff) to improve documentation around all this.

This patch fixes #66.1 and #66.2

Pending: Someone chiming in about #66.3 and eventually #66.4

Status: Needs review » Needs work

The last submitted patch, 67: 2839709-67-theme-development-services.patch, failed testing. View results

adamps’s picture

Many thanks @jwilson3 that patch seems like a direction that will work for all of us. I will review in more detail and add my thoughts on your questions in June (unfortunately no time next week).

jwilson3’s picture

Status: Needs work » Needs review
StatusFileSize
new5.75 KB
new704 bytes

Patch fixes the following error from previous test run on #67.

Scaffold source and destination files must have the same contents.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
     # For more information about debugging Twig templates, see\n
     # https://www.drupal.org/node/1906392.\n
     #\n
-    # Enabling Twig debugging is not recommended in production environments.\n
+    # Enabling Twig debugging is not recommended in production environments and\n
+    # can break parts of Drupal. See https://drupal.org/i/2914733 for details.\n
     # @default false\n
     debug: false\n
     # Twig auto-reload:\n
jwilson3’s picture

Title: Provide theme-development.services.yml to make twig_debug easier » Provide twig-debug.services.yml to make Debugging Twig easier
jwilson3’s picture

Issue summary: View changes
jwilson3’s picture

Issue summary: View changes
joachim’s picture

Status: Needs review » Needs work
+++ b/core/assets/scaffold/files/development.services.yml
@@ -2,8 +2,17 @@
+    # by default. Before changing the value here, follow the instructions
+    # in twig-debug.services.yml instead.

Before/instead doesn't make sense.

'Before' means 'do the other thing, then this one after'.
'Instead' means 'do the other thing and not this one'.

jwilson3’s picture

Good point. thanks.

How about:

# Twig debugging is turned off here because there is a better way to enable it. 
# See twig-debug.services.yml for instructions.
adamps’s picture

New patch looks good. Here is my suggestion for the comments in development.services.yml:

  twig.config:
    # Twig debugging is intentionally disabled on development environments by
    # default because it can break parts of Drupal. For details see
    # https://drupal.org/i/2914733. To enable it, follow the instructions in
    # twig-debug.services.yml.
    debug: false
    # Automatically reload modified Twig files.
    auto_reload: true

The change for auto_reload is because there is a separate setting for cache which remains enabled.

star-szr’s picture

Sorry I may be missing something obvious but why do we need to explicitly set debug to false in development.services.yml? It defaults to false anyway.

Also, we have comments talking about twig debug breaking parts of Drupal in a lot of places but seems like we should have that in twig-debug.services.yml as well. On that note, it seems a bit odd that we override http.response.debug_cacheability_headers in a file called twig-debug.services.yml. I have personally run into situations where the cacheability headers break things so they are in a somewhat similar boat. Naming things is hard. I'm not even sure if frontend-debug.services.yml would be accurate so I'm not sure what to suggest other than removing that from the scope of this patch/issue.

I hope I'm not being too nitpicky here. Thanks for working on this!

Edit: I see that we still have http.response.debug_cacheability_headers in development.services.yml so it seems like we should just remove it from twig-debug.services.yml. Seems like an out of scope change, maybe from an old patch?

jwilson3’s picture

why do we need to explicitly set debug to false in development.services.yml? It defaults to false anyway.

See #62 and then #51 for context, and let us know your thoughts please.

I suppose because you're asking this, then the proposed text in #76 is not going to be enough to satisfy your valid question.

star-szr’s picture

Thanks @jwilson3! In that case, my recommendation would be to try splitting off the auto_reload change to another issue if it's causing too much contention here. While nice to have, I would hate to see that hold up this feature since it's not a dependency or anything.

adamps’s picture

Title: Provide twig-debug.services.yml to make Debugging Twig easier » Make Debugging Twig easier

Sorry I may be missing something obvious but why do we need to explicitly set debug to false in development.services.yml? It defaults to false anyway.

I tend to agree. We can have a comment saying why debug is not set by default. However we don't need to actually set it to false.

Edit: I see that we still have http.response.debug_cacheability_headers in development.services.yml so it seems like we should just remove it from twig-debug.services.yml. Seems like an out of scope change, maybe from an old patch?

I agree.

my recommendation would be to try splitting off the auto_reload change to another issue if it's causing too much contention here.

This has been suggested before but it creates major problems for a certain set of users - see #65. For some sites, we need a way to set auto_reload (always useful in development) without debug (often useful, but causes major bugs on some sites), as explained in the IS. You might not need this part of the issue, but some of us do😃. Please let's not cycle back over the same ground yet again and go back to a patch that fails to meet the needs of the entire community.

jwilson3’s picture

We can have a comment saying why debug is not set by default. However we don't need to actually set it to false.

So then something like this in development.services.yml ?

  twig.config:
    # Automatically reload modified Twig files. Twig debugging is off by default
    # and should not be set here because it can break Drupal. To enable it,
    # follow the instructions in twig-debug.services.yml.
    auto_reload: true
it seems like we should just remove http.response.debug_cacheability_headers from twig-debug.services.yml.

Agree. Can remove this in next patch when we finalize verbiage above.

jwilson3’s picture

Issue summary: View changes
adamps’s picture

@jwilson3 Looks good to me thanks

jwilson3’s picture

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

jwilson3’s picture

Some folks are trying to get Twig debug added to development.services.yml which we've already addressed here. Linking it as another possible duplicate. I've asked them to address the concerns we all have (@Cottser and others) about enabling this option by default. If you feel strongly about it please chime in on #3278887: Update development.services.yml to include twig debug by default in case I missed something.

jwilson3’s picture

star-szr’s picture

Thanks for the bump and issue maintenance work @jwilson3!

Reviewing the overall issue, my suggestion to move this forward is still to reduce scope. Echoing my comment from #79 (June 9 2022):

...my recommendation would be to try splitting off the auto_reload change to another issue if it's causing too much contention here. While nice to have, I would hate to see that hold up this feature since it's not a dependency or anything.

To respond to @AdamPS in #80:

This has been suggested before but it creates major problems for a certain set of users - see #65. For some sites, we need a way to set auto_reload (always useful in development) without debug (often useful, but causes major bugs on some sites), as explained in the IS. You might not need this part of the issue, but some of us do😃. Please let's not cycle back over the same ground yet again and go back to a patch that fails to meet the needs of the entire community.

I may be missing something but I don't see how reducing the scope "creates" any problems for any users. Making it easier to turn on Twig debug doesn't hurt anyone. One issue does not need to solve all the things. auto_reload defaulting to on can (and IMO should) be handled in another issue. By splitting things up, we still accomplish this issue's goal as stated in the title ("Make Debugging Twig easier").

Small, well-tested and reasoned iterative improvements are much easier and faster to get in than larger changes. Working iteratively still meets the needs of the entire community - the patch from #49 does nothing to prevent you from turning on auto_reload in development, while adding a valuable feature that makes it easier for folks to toggle Twig debugging.

A separate issue for defaulting auto_reload to true in development would allow that functionality to be discussed and evaluated for its own merits. I think it's a fine idea, but I also strongly think that it does not need to be part of the scope here.

jwilson3’s picture

Small, well-tested and reasoned iterative improvements are much easier and faster to get in than larger changes.

I agree. And assuming AdamPS would be amenable... it sounds then like next steps would be to go back to the simplicity of patch in #49, and potentially:

1. Change the file name on that patch to twig-debug.services.yml (for various reasons, discussed in previous comments)

2. Remove these two lines:

+    auto_reload: null
+    cache: true

which would mean those settings would inherit the existing default values, and further clarify and align the file name with its intended scope.

3. Debate about adding auto_reload: true to development.services.yml in a follow-up.

4. Re-review comments and patches since Comment #49 to incorporate any other relevant changes, eg like documentation improvements.

Let's get consensus and get this one in! Its a long time coming!

adamps’s picture

3. Debate about adding auto_reload: true to development.services.yml in a follow-up.

OK I agree this is an excellent idea providing that we don't do anything that would prevent the second issue from going ahead. In other words the patch in this issue would effectively be a subset of the latest larger combined patch??

NB There is also #3278493: Make it easier for theme builders to enable Twig debugging and disable render cache that is leaning towards two separate GUI options.

star-szr’s picture

In other words the patch in this issue would effectively be a subset of the latest larger combined patch??

That is my understanding at least.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.