Twig settings implementation: Show theme hook / template suggestions in HTML comments

Here's the patch.

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.

Display debugging information in comments surrounding Twig template output

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.

Looks really good now => RTBC, unless we need another test?

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.

CNW for tests.

Here's a fairly basic test, I'm not sure if it's worth testing every little bit of the debug markup.

Messed up the patch naming, should be 1906506-9.

#9: 1906506-7.patch queued for re-testing.

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'  -->

+      $current_template = explode('/', $template_file);
+      $current_template = array_pop($current_template);

This feels like $current_template = basename($template_file);.

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' -->

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' -->

Interdiff for #17, I accidentally re-uploaded the patch from #16. Sorry, testbot!

CNW for more test coverage, I'll work on that tonight.

drupal_find_theme_templates() uses strtr(), not str_replace().

+++ 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;

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.

Fixing a comment in the test, sorry for all the noise.

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!

awesome!
thanks!!!

Fantastic!

Should we add this to the Twig change notice? I presume, yes.

#26 yup

tag

better title

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.

I think it can just be added to the original Twig change-notice as it really just extends the functionality of Twig.

My feeling is that would make for a very long change notice.

Okay, then a separate one for both latest changes together :-D.

I'll try and have one of our core mentoring participants draft up the change notice tomorrow.

Writing change notice.

Change notice: http://drupal.org/node/1922666

tags
thanks!

Adding commit message

Thanks @dags, that's great! I just made a couple very minor tweaks to the change notice.

This seems to have been partially rolled back for some reason... http://drupalcode.org/project/drupal.git/commitdiff/1ce3eeb

It's probably because of #1925822: Stop locking me out of my own sites/default directory if Drupal is already insecure :)

So do we want a patch to restore the parts that were rolled back?

#42 looks good

This looks good to me, but assigning to Jennifer for a look-over since it's docs-related.

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

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

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.

Committed to 8.x. Thanks!