Port twig_debug output to Drupal 7

Something like this seems safe to go in to Drupal 7.

Would it be better to do it more like this though (to avoid an extra function call for production sites)?

if (variable_get('theme_debug_output', FALSE)) {
  $output = _theme_render_template_debug($render_function, $template_file, $variables);
}
else {
  $output = $render_function($template_file, $variables);
}

This would be amazing.

Going to work on this in the near future :)

This implements #2 and backports the tests. I couldn't figure out how to get drupal_render() to use test_theme so there are a couple drupalGet()s thrown in.

Oops on #2, and I agree on #1. I can make those changes a bit later today. Thanks @Fabianx!

The tests definitely pass locally for me, via UI and run-tests.sh. I started chucking some patches up on #2338069: Ignore: patch testing issue for 2307505 to see if anything would stick. My guess at this point is that testbot is not seeing the test_theme template in the registry.

This addresses #10 and takes out a drupal_theme_rebuild() from the test.

So... I forgot to git add the new testing template I added. This should be green now :)

Sure, I agree!

Here's that change.

And while we're at it, here's the return type for _theme_render_template_debug :)

Cancelling the last test.

Demo'd this patch in a lightning talk in VanDUG! And have been using it in my sites.

Committed to 7.x - thanks!

I added a note about this to CHANGELOG.txt, but it might be good to document somewhere else too (maybe a change notification)?

Question:

+    $output['debug_info'] .= "\n<!-- FILE NAME SUGGESTIONS:\n   " . implode("\n   ", $suggestions) . "\n-->";

Would it be a good idea to check_plain() this (maybe some of the other output too, but especially this one)? I'm not too worried about it is since this is intended for developers anyway (not for live sites) but I am thinking that theme suggestion names come from all over the place; might be good to be safe.

I made two small fixes on commit, by the way:

--- a/modules/simpletest/tests/theme.test
+++ b/modules/simpletest/tests/theme.test
@@ -536,7 +536,7 @@ class ThemeDebugMarkupTestCase extends DrupalWebTestCase {
     // Create a node and test different features of the debug markup.
     $node = $this->drupalCreateNode();
     $this->drupalGet('node/' . $node->nid);
-    $this->assertRaw('<!-- THEME DEBUG -->', 'Twig debug markup found in theme output when debug is enabled.');
+    $this->assertRaw('<!-- THEME DEBUG -->', 'Theme debug markup found in theme output when debug is enabled.');
     $this->assertRaw("CALL: theme('node')", 'Theme call information found.');
     $this->assertRaw('x node--1' . $extension . PHP_EOL . '   * node--page' . $extension . PHP_EOL . '   * node' . $extension, 'Suggested template files found in order and node ID specific template shown as cur
     $template_filename = $templates['node__1']['path'] . '/' . $templates['node__1']['template'] . $extension;
@@ -561,7 +561,7 @@ class ThemeDebugMarkupTestCase extends DrupalWebTestCase {
     variable_set('theme_debug', FALSE);
 
     $this->drupalGet('node/' . $node->nid);
-    $this->assertNoRaw('<!-- THEME DEBUG -->', 'Twig debug markup not found in theme output when debug is disabled.');
+    $this->assertNoRaw('<!-- THEME DEBUG -->', 'Theme debug markup not found in theme output when debug is disabled.');
   }
 
 }

Thinking about this a little more I think we do need the check_plain(). It's intended for HTML output, so regardless of security considerations or not I think it needs that. Would be safer to just do it.

Any thoughts on the attached patch?

It's possible Drupal 8 needs something similar, but would be useful to get this in for the upcoming D7 release since it's going out to sites soon.

Committed to 7.x - thanks!

Thank you for working on the patch :)

As for documentation, I added a bit more info to the CHANGELOG.txt entry for this (which will also go into the release notes). If anyone wants to create a formal change record for this also, feel free... but it's not strictly necessary or anything.

Is there a way for other modules to alter the $output variable to add on their own items? I could see other modules possibly having data that could be important to show.

@redndahead - Not as is, it's meant to just output relevant information coming from the theme system only.

Really happy to see this in, and I agree about the check_plain. Here's an issue for D8 to consider something similar: #2369781: Ensure twig_debug output has needed sanitization