Posted by steveoliver on February 3, 2013 at 4:24am
13 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | documentation |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Twig |
Issue Summary
Issue #1906506 by Fabianx, steveoliver, Cottser: Added Twig settings implementation: Show theme hook / template suggestions in HTML comments.A follow-up to #1843034: Make Twig settings configurable, when debug is enabled, let's add our own custom debugging output as HTML comments wrapped around the template. The debugging output helps themers determine which template is being rendered, which theme suggestions are available and which theme /template suggestion has been selected for each piece of theme output.
Something like this:
<!-- THEME DEBUG -->
<!-- CALL: theme('node') -->
<!-- FILE NAME SUGGESTIONS:
* node--article.html.twig
* node--2.html.twig
x node.html.twig
-->
<!-- BEGIN OUTPUT from 'core/modules/node/templates/node.html.twig' -->
<article id="node-1" class="node node-article promoted view-mode-full clearfix" role="article" about="/node/1" typeof="sioc:Item foaf:Document">
<p>(content)</p>
</article>
<!-- END OUTPUT from 'core/modules/node/templates/node.html.twig' -->
Comments
#1
Here's the patch.
#2
For reference, here's the phrase from the default.settings.php docblock in the patch at #1843034-47: Make Twig settings configurable, to be updated in this issue after #1843034: Make Twig settings configurable gets in.
#3
Attached patch cleans up comment in twig_theme() and adds note about debugging output in comment for 'twig_debug' in default.settings.php.
Note: Git attribution goes primarily to Fabianx, who implemented this earlier in the Twig sandbox (see #1820158: Print template names in HTML comments).
#4
Talked with @steveoliver in IRC, here's a slightly tweaked comment for default.settings.php that should be easier to skim and makes the handbook link a bit more prominent.
#5
+++ b/sites/default/default.settings.php@@ -286,11 +286,16 @@
- * @see http://drupal.org/node/1906392
+ * When debugging is enabled:
+ * - The markup of each Twig template is surrounded by HTML comments with
+ * information such as template file name suggestions.
+ * - The 'dump' function can be used in Twig templates to output information
The item about comments could read a little better.
That's about all I see.
Fabian/jen/carl: RTBC?
#6
Looks really good now => RTBC, unless we need another test?
#7
I like the new wording, rerolled to fix one thing:
+++ b/sites/default/default.settings.phpundefined@@ -286,11 +286,16 @@
+ * contain themeing information such as template file name suggestions.
themeing = theming.
After talking to @Fabianx I think we can and should write tests for this, possibly a unit test.
Interdiff is from #4.
#8
CNW for tests.
#9
Here's a fairly basic test, I'm not sure if it's worth testing every little bit of the debug markup.
#10
Messed up the patch naming, should be 1906506-9.
#11
The last submitted patch, 1906506-7.patch, failed testing.
#12
#9: 1906506-7.patch queued for re-testing.
#13
+++ b/core/themes/engines/twig/twig.engine@@ -31,22 +31,47 @@ function twig_init($template) {
+ $suggestions[] = $variables['theme_hook_original'];
I think this line needs to be deleted. theme_hook_original seems to be automatically added to theme_hook_suggestions.
#14
That line needs to stay so we can show which theme suggestion is in use.
With the line:
<!-- THEME DEBUG --><!-- CALL: theme('node') -->
<!-- FILE NAME SUGGESTIONS:
* node--my-thing.html.twig
* node--1.html.twig
x node.html.twig
-->
<!-- BEGIN OUTPUT from 'core/modules/node/templates/node.html.twig' -->
Without it:
<!-- THEME DEBUG --><!-- CALL: theme('node') -->
<!-- FILE NAME SUGGESTIONS:
* node--my-thing.html.twig
* node--1.html.twig
-->
<!-- BEGIN OUTPUT from 'core/modules/node/templates/node.html.twig' -->
#15
<?php+ $current_template = explode('/', $template_file);
+ $current_template = array_pop($current_template);
?>
This feels like
$current_template = basename($template_file);.#16
Regarding my comment in #14, it would be more accurate to say the line needs to be there so that the base template is added to the debug output. In the example I showed, the base template is also the template in use.
@Damien - thanks for that, fixed.
Also made a couple minor whitespace tweaks/fixes to the debug output.
Before:
<!-- THEME DEBUG --><!-- CALL: theme('node') -->
<!-- FILE NAME SUGGESTIONS:
* node--page.html.twig
* node--1.html.twig
x node.html.twig
-->
<!-- BEGIN OUTPUT from 'core/modules/node/templates/node.html.twig' -->
…
<!-- END OUTPUT from 'core/modules/node/templates/node.html.twig' -->
After:
<!-- THEME DEBUG --><!-- CALL: theme('node') -->
<!-- FILE NAME SUGGESTIONS:
* node--page.html.twig
* node--1.html.twig
x node.html.twig
-->
<!-- BEGIN OUTPUT from 'core/modules/node/templates/node.html.twig' -->
…
<!-- END OUTPUT from 'core/modules/node/templates/node.html.twig' -->
#17
Okay, I think this does need more tests.
Before:
<!-- THEME DEBUG --><!-- CALL: theme('node') -->
<!-- FILE NAME SUGGESTIONS:
* node--page.html.twig
* node--1.html.twig
* node.html.twig
-->
<!-- BEGIN OUTPUT from 'core/themes/stark/templates/node--1.html.twig' -->
After:
<!-- THEME DEBUG --><!-- CALL: theme('node') -->
<!-- FILE NAME SUGGESTIONS:
* node--page.html.twig
x node--1.html.twig
* node.html.twig
-->
<!-- BEGIN OUTPUT from 'core/themes/stark/templates/node--1.html.twig' -->
#18
Interdiff for #17, I accidentally re-uploaded the patch from #16. Sorry, testbot!
CNW for more test coverage, I'll work on that tonight.
#19
drupal_find_theme_templates() uses strtr(), not str_replace().
#20
+++ b/core/themes/engines/twig/twig.engineundefined@@ -31,22 +31,47 @@ function twig_init($template) {
+ $template = $suggestion . $extension;
+ $template = strtr($template, '_', '-');
Should probably be:
$template = strtr($suggestion, '_', '-') . $extension;#21
Addressed #20 and added quite a bit more test coverage.
New test-only patch and a patch combining the new test with the patch from #16 to show that it exposes the failure there.
#22
Fixing a comment in the test, sorry for all the noise.
#23
passing tests, verified output locally.
#24
Oh man! This feature is AWESOME! It's like devel themer lite in core. Likely to make the forthcoming Twig conversions much easier on people, as well.
Committed and pushed to 8.x. Thanks!
#25
awesome!
thanks!!!
#26
Fantastic!
Should we add this to the Twig change notice? I presume, yes.
#27
#26 yup
#28
tag
#29
better title
#30
What a nice surprise to wake up to :)
Can the change notice combine this issue and #1843034: Make Twig settings configurable? That would make the most sense to me.
#31
I think it can just be added to the original Twig change-notice as it really just extends the functionality of Twig.
#32
My feeling is that would make for a very long change notice.
#33
Okay, then a separate one for both latest changes together :-D.
#34
I'll try and have one of our core mentoring participants draft up the change notice tomorrow.
#35
Writing change notice.
#36
Change notice: http://drupal.org/node/1922666
#37
tags
thanks!
#38
Thanks @dags, that's great! I just made a couple very minor tweaks to the change notice.
#39
This seems to have been partially rolled back for some reason... http://drupalcode.org/project/drupal.git/commitdiff/1ce3eeb
#40
It's probably because of #1925822: Stop locking me out of my own sites/default directory :)
#41
So do we want a patch to restore the parts that were rolled back?
#42
Yeah.
#43
#42 looks good
#44
This looks good to me, but assigning to Jennifer for a look-over since it's docs-related.
#45
It's OK, I guess..
The first list item took me a while to parse, and has a minor which/that grammar problem (if you want the word to be "which" there, it needs to be preceded by a comma; otherwise, the word needs to be "that" -- in this case I think "that" is better?)... maybe it could use a comma before "such as" as well? That would probably help readability.
In the second list item, should the dump() function have () on it rather than '' around it? It seems like it should, but I don't know much about Twig (yet). :)
Probably the first line should not end in :, but it looks like the default.settings.php file is all over the map on that. Some of the "doc blocks" (they aren't really doc blocks anyway) don't have a first-line summary, some do and it ends in ".", some do and it ends in ":". So we probably ought to file a separate issue to clean this up. Anyway none of them should really be /** */ docblocks because they aren't documenting anything we can display in the API module anyway. But ... let's deal with that on a separate issue (can someone file one?).
#46
Do we have an official Drupal rule about that vs. which? I thought "which" could be used with or without a comma, and that's what the dictionary says too (http://www.ahdictionary.com/word/search.html?id=T5157300).
In any case, "that" works also so let's switch to that. And add a comma before "such as". And change 'dump' to dump()...
Should probably get this in and then leave other things to followups? (Especially because this was already committed with the original text and then rolled back apparently by accident.)
#47
I don't think that dictionary is a great reference for usage and commas.... Check Strunk & White, Chicago Manual of Style, etc. If "which" is used to introduce a clause like in the previous patch, it definitely needs a comma, and that is standard English usage, not a Drupal rule. [My "Index to English" usage reference book says to generally use "that" for restrictive clauses and "which" for non-restrictive unless there are other "that" words being used in the sentence. And it says to set off non-restrictive modifiers with commas and not to use commas with restrictive. Restrictive is when the information is essential to the meaning (you can't leave it out without changing the meaning). So in this case, it's probably restrictive and hence should use "that" and no comma; if you think it is non-restrictive you can use "which" and a comma.]
Meanwhile... the new patch is much better, thanks! +1 for RTBC now.
#48
Committed to 8.x. Thanks!
#49
Automatically closed -- issue fixed for 2 weeks with no activity.