Problem/Motivation

Twig_debug output is very hot right now and a big reason is that many functions have been converted to templates.

Still the output can be very useful already. Also this can be useful to ease the Drupal 7 > Drupal 8 path by having available this little gem feature already in Drupal 7.

Proposed resolution

Port over the twig debug output and make it available for D7 using theme_debug variable.

Remaining tasks

None.

User interface changes

None.

API changes

- API addition: theme_hook_original is set as in D8
- API addition of _theme_render_template_debug function - internal

New variable:

theme_debug => TRUE / FALSE, defaults to FALSE

CommentFileSizeAuthor
#34 2307505-34-followup.patch1.53 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 41,096 pass(es). View
#18 interdiff.txt523 bytesCottser
#18 2307505-18.patch8.25 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 40,929 pass(es). View
#17 interdiff.txt2.09 KBCottser
#17 2307505-17.patch8.24 KBCottser
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#14 interdiff.txt367 bytesCottser
#14 2307505-14.patch8.27 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 40,927 pass(es). View
#12 interdiff.txt6.37 KBCottser
#12 2307505-12.patch7.91 KBCottser
FAILED: [[SimpleTest]]: [MySQL] 40,925 pass(es), 2 fail(s), and 2 exception(s). View
#7 interdiff.txt4.18 KBCottser
#7 2307505-7.patch7.93 KBCottser
FAILED: [[SimpleTest]]: [MySQL] 40,925 pass(es), 2 fail(s), and 2 exception(s). View
#7 2307505-7-testonly.patch3.53 KBCottser
FAILED: [[SimpleTest]]: [MySQL] 40,920 pass(es), 7 fail(s), and 2 exception(s). View
core--theme-debug-output.diff4.23 KBFabianx
PASSED: [[SimpleTest]]: [MySQL] 41,240 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Fabianx’s picture

Issue summary: View changes
David_Rothstein’s picture

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);
}
Fabianx’s picture

Assigned: Unassigned » Fabianx
Status: Needs review » Needs work

That seems like a great suggestion!

Cottser’s picture

Issue tags: +Needs tests

This would be amazing.

Cottser’s picture

Assigned: Fabianx » Cottser

Going to work on this in the near future :)

Fabianx’s picture

Thanks, Cottser :)

Cottser’s picture

Assigned: Cottser » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.53 KB
FAILED: [[SimpleTest]]: [MySQL] 40,920 pass(es), 7 fail(s), and 2 exception(s). View
7.93 KB
FAILED: [[SimpleTest]]: [MySQL] 40,925 pass(es), 2 fail(s), and 2 exception(s). View
4.18 KB

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.

The last submitted patch, 7: 2307505-7-testonly.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 7: 2307505-7.patch, failed testing.

Fabianx’s picture

Assigned: Unassigned » Cottser
  1. +++ b/includes/theme.inc
    @@ -1521,6 +1529,73 @@ function theme_render_template($template_file, $variables) {
     /**
    + * Renders a template for any engine.
    + *
    

    I now it was my idea, but if we are gonna run with this for any engine, we should probably also pass the extension in and its trivial to do so.

  2. +++ b/includes/theme.inc
    @@ -1521,6 +1529,73 @@ function theme_render_template($template_file, $variables) {
    +  );
    +  if (variable_get('theme_debug_output', FALSE)) {
    +    $output['debug_prefix'] .= "\n\n<!-- THEME DEBUG -->";
    

    I think we can remove this if code now :)

The debug markup test still fails, no idea why though.

Cottser’s picture

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.

Cottser’s picture

Status: Needs work » Needs review
FileSize
7.91 KB
FAILED: [[SimpleTest]]: [MySQL] 40,925 pass(es), 2 fail(s), and 2 exception(s). View
6.37 KB

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

Status: Needs review » Needs work

The last submitted patch, 12: 2307505-12.patch, failed testing.

Cottser’s picture

Assigned: Cottser » Unassigned
Status: Needs work » Needs review
Issue tags: +Twig
FileSize
8.27 KB
PASSED: [[SimpleTest]]: [MySQL] 40,927 pass(es). View
367 bytes

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

Fabianx’s picture

Very little nit:

- Mark Carver asked for the variable to be only called "theme_debug" to more closely match "twig_debug" in D8.

Once that is done - if we agree this should be done - this is RTBC :).

Great work!

Cottser’s picture

Title: Port twig_debug_output to Drupal 7 » Port twig_debug output to Drupal 7
Assigned: Unassigned » Cottser

Sure, I agree!

Cottser’s picture

Assigned: Cottser » Unassigned
Issue summary: View changes
FileSize
8.24 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
2.09 KB

Here's that change.

Cottser’s picture

FileSize
8.25 KB
PASSED: [[SimpleTest]]: [MySQL] 40,929 pass(es). View
523 bytes

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

Cancelling the last test.

The last submitted patch, 17: 2307505-17.patch, failed testing.

Fabianx’s picture

Assigned: Unassigned » David_Rothstein
Status: Needs review » Reviewed & tested by the community

RTBC, great work!

joelpittet’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 2307505-18.patch, failed testing.

Fabianx queued 18: 2307505-18.patch for re-testing.

Cottser’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 2307505-18.patch, failed testing.

Fabianx queued 18: 2307505-18.patch for re-testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 2307505-18.patch, failed testing.

Fabianx queued 18: 2307505-18.patch for re-testing.

Cottser’s picture

Status: Needs work » Reviewed & tested by the community
David_Rothstein’s picture

Title: Port twig_debug output to Drupal 7 » (documentation/followup?) Port twig_debug output to Drupal 7
Status: Reviewed & tested by the community » Active
Issue tags: +7.33 release notes, +7.33 release announcement

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.');
   }
 
 }

  • David_Rothstein committed 4443a30 on 7.x
    Issue #2307505 by Cottser, Fabianx: Port twig_debug output to Drupal 7.
    
David_Rothstein’s picture

Assigned: David_Rothstein » Unassigned
David_Rothstein’s picture

Status: Active » Needs review
FileSize
1.53 KB
PASSED: [[SimpleTest]]: [MySQL] 41,096 pass(es). View

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.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, I totally agree, e.g. for page.tpl.php, we use the create theme suggestions functions (whatever that is called), which IIRC would then create the -- based on the page arguments. That should be properly escaped, but this is safer for sure and because its in a debug path, lets just do it.

Thanks so much for accepting the patch!

David_Rothstein’s picture

Title: (documentation/followup?) Port twig_debug output to Drupal 7 » Port twig_debug output to Drupal 7
Status: Reviewed & tested by the community » Fixed

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.

  • David_Rothstein committed 86a1ebe on 7.x
    Issue #2307505 by Cottser, David_Rothstein, Fabianx: Followup to ensure...
redndahead’s picture

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.

Cottser’s picture

@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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.