Needs work
Project:
Drupal core
Version:
main
Component:
theme system
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
28 Dec 2016 at 16:35 UTC
Updated:
23 Jan 2023 at 16:29 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
gpap commentedComment #3
gpap commentedComment #4
Thew commentedI 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.
However, if my thought is right, you can only set
debug: true. Theauto_reloadandcachewill automatically follow the debug parameter. See Debugging compiled Twig templatesComment #5
aadil.addweb commentedPlease apply disable_twig_cache_by-2839709-5.patch.
Comment #6
star-szrThanks 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:
See https://github.com/hechoendrupal/drupal-console/pull/1310 from the Drupal Console project where something similar came up.
Comment #8
joelpittetI'm on board with the this except
cachelike @Cottser mentioned in #6. And with debug on,auto_reloadis 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?
Comment #9
drupalmonkey commentedUpdated the patch as per #9.
Comment #11
Anonymous (not verified) commented#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.Comment #12
Anonymous (not verified) commentedComment #13
jwilson3Updating 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.debugis set totruein 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: falsein development (!!!) thedebug: truein 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
Comment #14
adamps commentedI have created a META issue to list all of the known problems of twig debug: #2914733: [META] twig debug can break parts of site.
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 EventSubscriberInterfacethat fixes the feed by moving the comments after the xml tag.Comment #15
adamps commentedOn 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:
Comment #16
willthemoor commentedWondering if a better longer(ish) term move might be to (automagically) allow for
development.services.local.yml(or is itlocal.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.
Comment #21
norman.lolRaising 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.
Comment #22
agileadamYou have another vote (me) for putting this in development.services.yml out-of-the-box.
Comment #23
baluertlPlus one vote on enabling this option by default out-of-the-box.
Comment #24
adamps commentedNote 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.
Comment #25
jwilson3Here 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.
Comment #26
jwilson3Comment #28
adamps commented@jwilson3 thanks looks good to me - although I haven't tested it
I removed some tags that look like "adding random keywords". If anyone disagrees, apologies, please feel free to put them back.
Comment #29
jwilson3I 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?
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:
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.
Comment #30
jwilson3Comment #32
adamps commentedPerhaps 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 thetwig_debugsetting where there is no single value that is correct for all dev sites.twig_debugthen you need to havetwig_debugdisabled, 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.Comment #33
adamps commentedI 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.
Comment #38
spokjeUsed
2839709-25-theme-development-services.patchas a base for the new MR against9.3.x.Hidden all the (now outdated) previous patch files.
Comment #40
spokjeComment #42
damienmckennaI 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.
Comment #43
jwilson3I'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.
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.
Comment #44
jwilson3Patch adds scaffold files.
Comment #45
jwilson3AFAICT, 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'] = TRUEin sites/files/default.settings.php. In this issue, the approach is very similar; you just uncomment one line in settings.local.php.Comment #47
jwilson3The test run failure seems unrelated to this issue.
Comment #48
star-szrI 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".
Comment #49
jwilson3Cache is now set to `true` to address #48. Thanks for the reference to catch the context from comment #6.
Comment #50
adamps commentedThanks @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.
Comment #51
jwilson3I'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.
Comment #52
adamps commentedThanks @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:
Comment #53
adamps commentedI'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.
Comment #54
jwilson3What 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.
Comment #55
adamps commentedAh I see what you mean.
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.
Comment #56
damienmckennaFYI 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.
Comment #57
jwilson3okay. I think the patchfile can be edited to remove the copy. Thanks Damien.
Comment #58
damienmckennaIdea: 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?
Comment #59
jwilson3No 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%Comment #60
jwilson3This 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?
Comment #61
jwilson3NR for #59
Comment #62
joachim commentedI'm confused why twig parameters are split across two files. Should this be explained in the files?
Comment #63
jwilson3#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.
Comment #65
adamps commented@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.
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.
Yes I agree the existing file names are confusing. However I feel that the names I propose are quite clear.
Comment #66
jwilson3@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:
Comment #67
jwilson3The 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
Comment #69
adamps commentedMany 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).
Comment #70
jwilson3Patch fixes the following error from previous test run on #67.
Comment #71
jwilson3Comment #72
jwilson3Comment #73
jwilson3Comment #74
joachim commentedBefore/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'.
Comment #75
jwilson3Good point. thanks.
How about:
Comment #76
adamps commentedNew patch looks good. Here is my suggestion for the comments in development.services.yml:
The change for auto_reload is because there is a separate setting for cache which remains enabled.
Comment #77
star-szrSorry 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_headersin 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_headersin 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?Comment #78
jwilson3See #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.
Comment #79
star-szrThanks @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.
Comment #80
adamps commentedI 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.
I agree.
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.
Comment #81
jwilson3So then something like this in development.services.yml ?
Agree. Can remove this in next patch when we finalize verbiage above.
Comment #82
jwilson3Comment #83
adamps commented@jwilson3 Looks good to me thanks
Comment #84
jwilson3Note there are some new discussions that would affect the direction set here.
Comment #85
jwilson3Comment #87
jwilson3Some 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.
Comment #88
jwilson3Comment #89
star-szrThanks 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):
To respond to @AdamPS in #80:
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.
Comment #90
jwilson3I 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:
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!
Comment #91
adamps commentedOK 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.
Comment #92
star-szrThat is my understanding at least.