This issue has novice tasks. If you are an experienced core developer and have multiple commit mentions, please review novices' work on these tasks rather than doing them yourself. Feedback from experienced contributors is valued.

Problem/Motivation

twig_debug currently references an internal function, _theme(). It would probably be more helpful if the display was more like a render array or if we can come up with another way to present the theme hook(s) called since developers need to build render arrays instead of calling _theme().

Proposed resolution

Change twig_debug output to not refer to _theme(). Exact solution to be determined and up for discussion and suggestions.

Remaining tasks

Steps to reproduce

Uncomment the $settings['twig_debug'] = TRUE; line in settings.php and view source or use web inspector or equivalent and you should see something like this:

<!-- THEME DEBUG -->
<!-- CALL: _theme('html') -->
<!-- FILE NAME SUGGESTIONS:
   * html--front.html.twig
   * html--.html.twig
   x html.html.twig
-->
<!-- BEGIN OUTPUT from 'core/modules/system/templates/html.html.twig' -->

User interface changes

n/a

API changes

n/a

Files: 
CommentFileSizeAuthor
#25 don_t_print_theme-2201789-25.patch3.05 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,023 pass(es). View
#19 interdiff.txt2.82 KBjjcarrion
#19 2201789-remove_theme-twig-19.patch4.19 KBjjcarrion
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,831 pass(es). View
#16 twig-debug.png49.8 KBrteijeiro
#14 2201789-remove_theme-twig-14.patch4.19 KBstefanos.petrakis
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,728 pass(es). View
#14 interdiff.txt3.99 KBstefanos.petrakis
#12 interdiff-2201789-7-12.txt3.97 KBstefanos.petrakis
#12 2201789-remove_theme-twig-12.patch4.17 KBstefanos.petrakis
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,738 pass(es). View
#7 2201789-remove_theme-twig-7.patch2.89 KBwiifm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,549 pass(es). View

Comments

lussoluca’s picture

FileSize
749 bytes

Hi Cottser,
I suggest to simply removes all references to _theme function because print something like a render array would be confusing (too many variations and data).

Luca

lussoluca’s picture

Status: Active » Needs review
Cottser’s picture

Status: Needs review » Needs work

Thank you @lussoluca and sorry for the late response. Yes I agree we shouldn't print a whole render array. But I'm not sure we should refer to theme functions since those should be going away or at least being severly reduced by #1757550: [Meta] Convert core theme functions to Twig templates.

Geizt’s picture

FileSize
2.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,460 pass(es). View

Removal of "_theme" in twig debug output as well as removal of test of "_theme" from testTwigDebugMarkup().

Geizt’s picture

Status: Needs work » Needs review

Review patch added to comment #4.

joelpittet’s picture

Status: Needs review » Needs work

@Geizt thanks for posting a patch though the intent isn't to remove the lines entirely as they provide helpful information. Just avoid referencing the internal call to _theme().

It's a bit fuzzy on what to rename it to, but something along the lines of Rendered theme hook: 'node__foo__bar' maybe?

Note theme() doesn't exist anymore and _theme() is internal only. drupal_render($render_array) will render the variables into the template. The #theme property dictates which hook_theme to call the prepeprocess and render a twig template. Hope that helps some.

wiifm’s picture

Status: Needs work » Needs review
FileSize
2.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,549 pass(es). View

Attached is a patch that will hopefully achieve what @joelpittet is after. Lets see what the test bot says.

joelpittet’s picture

That works for me, @Cottser will that wording work for you as well?

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

Works for me, thanks everyone!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

2201789-remove_theme-twig-7.patch no longer applies.

error: patch failed: core/themes/engines/twig/twig.engine:55
error: core/themes/engines/twig/twig.engine: patch does not apply

stefanos.petrakis’s picture

Assigned: Unassigned » stefanos.petrakis
stefanos.petrakis’s picture

FileSize
4.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,738 pass(es). View
3.97 KB

Rerolled the patch from #7
+ Replaced the PHP_EOL character with a preg_match /s switch (essentially rewrote the string comparison replacing strpos with preg_match for multi-line matching)

rteijeiro’s picture

I can see some issues:

- Please, don't use tabs for indentation, just use spaces instead ;)
- You can just call the interdiff "interdiff.txt".
- Don't forget to update the issue status as "Needs review" in order to let the testbot to test it.

Great work! Thanks!!

stefanos.petrakis’s picture

Status: Needs work » Needs review
FileSize
3.99 KB
4.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,728 pass(es). View

Following rteijeiro comments (thanks):

+ Removed tab characters
+ Renamed interdiff-2201789-7-12.txt to interdiff.txt

joelpittet’s picture

Nice work guys, just an FYI, the interdiff.txt naming you did in #12 is not bad, and can be useful to tell at a glance which patch you were working off (if there is any confusion), but usually there isn't and save your self a few seconds and do what @rteijeiro said. Either way, it's up to you on that one, as long as there is one, I'm happy:)

The way you did it in #12 is how it's recommended in the docs so good work also reading them:) @see https://drupal.org/documentation/git/interdiff

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
49.8 KB

I always like your feedback @joelpittet. Thanks!!

Applied the patch and everything seems to work like a charm. It's a RTBC for me. Great work @stefanos.petrakis!!

markcarver’s picture

Status: Reviewed & tested by the community » Needs work

I know this is rather petty, but I think it's important that the label should really be capitalized like the line before and after RENDERED THEME HOOK:

jjcarrion’s picture

jjcarrion’s picture

Status: Needs work » Needs review
FileSize
4.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,831 pass(es). View
2.82 KB

I have make the patch and the interdiff capitalizing the "Rendered theme hook" text.

markcarver’s picture

Status: Needs review » Reviewed & tested by the community

Awesome :) Ty! Setting back to RTBC since that was so minor.

markcarver’s picture

Issue tags: -Needs reroll
markcarver’s picture

Assigned: jjcarrion » Unassigned

I should really start looking at the entire form before submitting lol

webchick’s picture

Status: Reviewed & tested by the community » Needs review

So I assume this is fine, since other people with far more theming experience in their pinky finger than I have in all of me worked on this and marked this RTBC, buuuuttt....

Before:

<!-- THEME DEBUG -->
<!-- CALL: _theme('html') -->
<!-- FILE NAME SUGGESTIONS:
   * html--front.html.twig
   * html--.html.twig
   x html.html.twig
-->
<!-- BEGIN OUTPUT from 'core/modules/system/templates/html.html.twig' -->

After:

<!-- THEME DEBUG -->
<!-- RENDERED THEME HOOK: 'html' -->
<!-- FILE NAME SUGGESTIONS:
   * html--front.html.twig
   * html--.html.twig
   x html.html.twig
-->
<!-- BEGIN OUTPUT from 'core/modules/system/templates/html.html.twig' -->

The "before" thing was a word "call" which is a common verb regarding what happens to PHP functions, and its value is something I can easily look up because "_theme()" is something I can search for on api.drupal.org and find docs, etc.

"Rendered theme hook" though feels extremely jargony to me, and it's not clear what to do with that if I want to find out more information. Is there any other term which would be both a) accurate b) not point people to deprecated nonsense, but c) would be clearer to front-end developers new to Drupal what on earth we're talking about here? Or is this really the best term? Like my "beginner mind" would call this something like "Base template" or something, I dunno.

Don't want to cause a huge bikeshed, but since I know Twiggy people are around and sprinting, thought maybe it was worth kicking this back for a quick round of brainstorming. If this is indeed the best term though, that's fine.

joelpittet’s picture

@webchick I agree with your concerns it is a bit jargony. Probably doesn't even need the word "Rendered" because that doesn't give any more information to what is going on.

_theme() still does the rendering of the template, though giving that information could be deterimental as it may indicate people to use that now private function.

drupal_render() is what does the load of the work, but again telling people to use that is also not a great idea because twig calls that automagically when an array is being "printed" to the screen. And calling that early makes a string out of the structured data, effectively flattening it.

So... we need a way of saying that the rendered/flattened string came from a renderable array and most likely a '#theme' => 'boink' triggered rendering the template 'boink.html.twig'.

Suggestions?

joelpittet’s picture

FileSize
3.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,023 pass(es). View

Re-rolled. Just the text change. Removed the word "Rendered" because it was superfluous information.

Jolidog’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me, THEME HOOK is clear on what is doing, RENDERED is not needed at all and adds to the confusion.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: don_t_print_theme-2201789-25.patch, failed testing.

jsbalsera’s picture

It passes the test locally, so I put it to retest

Status: Needs work » Needs review
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Cool, I think this works, and is certainly better than telling them to call something they shouldn't. When I google for "theme hook" I'm taken to https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func..., the Drupal 8 version of that is https://api.drupal.org/api/drupal/core%21modules%21system%21system.api.p... and talks about the concept. It's not as good as the overview at https://api.drupal.org/api/drupal/core%21modules%21system%21theme.api.ph... but we could perhaps add a pointer from hook_theme() to that API page in a separate follow-up to help people out.

Committed and pushed to 8.x. Thanks!

  • webchick committed 6e5fd14 on 8.0.x
    Issue #2201789 by joelpittet, jjcarrion, stefanos.petrakis, wiifm, Geizt...

Status: Fixed » Closed (fixed)

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