forum.module - Convert PHPTemplate templates to Twig

Note we're hoping much of the custom forum output will be powered by views eventually

Attached is a patch

Thanks @disasm! See #1898448-9: search.module - Convert PHPTemplate templates to Twig for a fix for the pager exceptions.

A new version of the patch from #4 is attached with $variables['pager'] add to the preprocess functions per #1898448-9: search.module - Convert PHPTemplate templates to Twig and a couple of whitespace issues fixed. Passes all forum tests in my sandbox.

Tagging.

I reviewed this patch over yesterday and today, working on a new patch a bit later today.

Still working on this, new patch will be up tomorrow.

Add conversion summary table

Add testing steps

Leaving the manual testing tag off, I've gone over the markup several times and can't see anything that has changed other than whitespace. I've added manual testing steps to the summary though.

Here's what I changed:

• Updated documentation throughout.
• Converted theme() calls to render arrays and removed drupal_render() calls per http://drupal.org/node/1920746#render where possible. The only exception is forum_submitted which needs to be a string for now to be passed around various functions.
• Fixed column markup in forum-topic-list.html.twig.
• Added missing class to forum-icon.html.twig.
• Fixed overeager indent in forum-topic-list.html.twig by using the same logic found in #1939102: Convert theme_indentation() to Twig.

This just removes some data types from Twig template documentation per http://drupal.org/node/1823416#docblock.

Updating docs.

Updated docs (forum is not a variable, the loop variable can be called anything), and updated the recursion loop hack to use @steveoliver's obviously better method from #1898432-58: node.module - Convert PHPTemplate templates to Twig :)

Quick tweak to use 'collection' instead of 'list' in the Twig template docs.

Minor tweaks and docs updates. I added a couple todos pointing to #1970960: Twig implementation does not print int(0) so we can remove the type casting once that is resolved.

+++ b/core/modules/forum/templates/forum-form.html.twigundefined
@@ -0,0 +1,20 @@
+ * By default this does not alter the appearance of a form at all, but is
+ * provided as a convenience for themers.

I don't think these docs in forum-form.html.twig are helping anyone, removing :)

This patch will need to be revised after #1975462: Allow and test for NULL and integer 0 values in Twig templates. lands to remove the type casting.

Tweak test steps

Needs a reroll for #1818560: Convert taxonomy entities to the new Entity Field API and #1975462: Allow and test for NULL and integer 0 values in Twig templates., I'll take care of it in the next couple days.

First the reroll for #1818560: Convert taxonomy entities to the new Entity Field API to make the patch apply again.

• Removed a consolidation @todo (see #1757550-38: [Meta] Convert core theme functions to Twig templates for explanation)
• Removed int casting now that #1975462: Allow and test for NULL and integer 0 values in Twig templates. is in.

Unassigning, this needs reviews please!

Thanks @shanethehat :)

@steveoliver, review please!

Tagging for profiling.

Looks good to me

This is almost ready. Super small nitpicks:

+++ b/core/modules/forum/forum.module
@@ -978,9 +978,11 @@ function forum_preprocess_block(&$variables) {
- * Preprocesses variables for forums.tpl.php.
+ * Prepares variables for forum templates.
  *

1. for forums templates (plural).

+++ b/core/modules/forum/forum.module
@@ -1031,24 +1042,24 @@ function template_preprocess_forums(&$variables) {
- * Preprocesses variables for forum-list.tpl.php.
+ * Prepares variables for forum list templates.
+ *
+ *
+ * Default template: forum-list.html.twig.

2. One extra linebreak needs deleted.

+++ b/core/modules/forum/forum.module
index 0000000..25d7fe6
--- /dev/null
+++ b/core/modules/forum/templates/forum-icon.html.twig
@@ -0,0 +1,22 @@
+{#
+/**
+ * @file
+ * Default theme implementation to display an appropriate icon for a forum post.
+ *
+ * Available variables:
+ * - new_posts: Indicates whether or not the topic contains new posts.
+ * - icon_class: The icon to display. May be one of 'hot', 'hot-new', 'new',
+ *   'default', 'closed', or 'sticky'.
+ * - icon_title: Text alternative for the forum icon.
+ * - first_new: Indicates whether this is the first topic with new posts.
+ *
+ * @see template_preprocess()
+ * @see template_preprocess_forum_icon()
+ *
+ * @ingroup themeable
+ */
+#}
+<div class="icon topic-status-{{ icon_class }}" title="{{ icon_title }}">
+  {% if first_new %}<a id="new"></a>{% endif %}
+  <span class="element-invisible">{{ icon_title }}</span>

3. "Default theme implementation to display a status icon for a forum post."
4. Move those classes to template_preprocess_form_icon()
5. good place to use whitespace dashes:

{% if first_new -%}
  <a id="new"></a>
{%- endif %}

6. Wait, what -- @id=new?
7. Hmmm.. div @title and the invisible span? Aroo?
8. There are actually @id's all over the place in this patch.

Oh well, I guess 6, 7, and 8 can get cleaned up later.

dibbs.

1-5 done.

Well, I was going to wait for that to pass tests, but since it's being slow, I've updated the attributes on the forum icon template here, and cleaned up the docblock a little.

#30:

<stdin>:173: trailing whitespace.
  
<stdin>:216: trailing whitespace.
 *   $variables['attributes']['class'] from preprocess functions. 
warning: 2 lines add whitespace errors.

Just one nitpick and a possible minor performance improvement. Cottser was saying mentioning there was a small improvement when we removed the empty variable declarations from comment module. So we may want to do that here too. Specially since we fixed #1975462: Allow and test for NULL and integer 0 values in Twig templates.

+++ b/core/modules/forum/forum.module
@@ -994,23 +996,32 @@ function forum_preprocess_block(&$variables) {
     else {
-      $variables['forums'] = '';
+      $variables['forums'] = array();

Don't think we need this and may help to remove for performance.

+++ b/core/modules/forum/forum.module
@@ -994,23 +996,32 @@ function forum_preprocess_block(&$variables) {
     }
     else {
-      $variables['topics'] = '';
+      $variables['topics'] = array();

Same here

+++ b/core/modules/forum/forum.module
@@ -1031,24 +1042,23 @@ function template_preprocess_forums(&$variables) {
   }
   else {
-    $variables['forums'] = '';
-    $variables['topics'] = '';
+    $variables['forums'] = array();
+    $variables['topics'] = array();

And here.

+++ b/core/modules/forum/templates/forum-icon.html.twig
@@ -0,0 +1,26 @@
+<div {{ attributes }}>

Please remove the space in front of {{ attributes }}

I was wrong about initializing the variables, we need to at least initialize them unless we add if's to the template.

My mistake, sorry! See Drupal\Core\Template\TwigTemplate::getContextReference for why.

This reverts most of #38 other than the attributes tweak in the template.

Ah I see, here's the fix to #40. There was an accidental $$ and the way the classes were built out for the icon was not as it was originally.

Let's put the right patch up for that, shall we?

@steveoliver, can you have another gander at this one?

Since this addresses what I noticed in 1-5 of #28, I'd RTBC this after acceptable perf tests. Will get through the rest of my queue and come back and profile this if nobody gets to it before me.

profiling.

Set up for profiling:
- change to stark theme
- enable forum module
- add 3 new forum topics
- add 3 comments to each forum topic
- build a view of one node in "full" view mode, block display
- put the block in the "content" region

Pages profiled:
- Forums page (forum) - forums, forum-list, forum-submitted
- Forum page (forum/1) - forum-icon, forum-topic-list

I'm having some crazy weirdness intermittantly with these tests, I'm not sure what is going on and could use some help debugging these results. Leaving as Needs benchmarking for now.

Forums Page (forums)

=== 8.x..8.x compared (519aa37b5bebd..519aa3e018419):
ct  : 47,087|47,087|0|0.0%
wt  : 665,370|663,578|-1,792|-0.3%
cpu : 656,086|654,643|-1,443|-0.2%
mu  : 42,380,288|42,380,288|0|0.0%
pmu : 42,527,336|42,527,336|0|0.0%

Profiler output

=== 8.x..twig-forum compared (519aa37b5bebd..519aa45581fb3):
ct  : 47,087|47,342|255|0.5%
wt  : 665,370|672,896|7,526|1.1%
cpu : 656,086|661,419|5,333|0.8%
mu  : 42,380,288|42,673,160|292,872|0.7%
pmu : 42,527,336|42,796,120|268,784|0.6%

Profiler output

General DIscussion page (forum/1)

=== 8.x..8.x compared (519abf80bb905..519ac01f8d84e):
ct  : 53,166|53,166|0|0.0%
wt  : 725,430|725,683|253|0.0%
cpu : 715,191|715,643|452|0.1%
mu  : 43,985,408|43,985,408|0|0.0%
pmu : 45,306,352|45,306,352|0|0.0%

Profiler output

=== 8.x..twig-forum compared (519abf80bb905..519ac0bcaa064):
ct  : 53,166|55,010|1,844|3.5%
wt  : 725,430|733,731|8,301|1.1%
cpu : 715,191|724,258|9,067|1.3%
mu  : 43,985,408|43,807,624|-177,784|-0.4%
pmu : 45,306,352|43,964,064|-1,342,288|-3.0%

Profiler output

I just got the same results three times in a row for the second template....

Forums Page (forum)

=== 8.x..8.x compared (519b0e62696b0..519b12bb10362):

ct  : 47,905|47,770|-135|-0.3%
wt  : 677,118|675,055|-2,063|-0.3%
cpu : 668,431|666,306|-2,125|-0.3%
mu  : 42,554,448|42,463,368|-91,080|-0.2%
pmu : 42,713,496|42,622,800|-90,696|-0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519b0e62696b0&...

=== 8.x..twig-forum compared (519b0e62696b0..519b130ea93e9):

ct  : 47,905|48,026|121|0.3%
wt  : 677,118|682,746|5,628|0.8%
cpu : 668,431|670,743|2,312|0.3%
mu  : 42,554,448|42,756,072|201,624|0.5%
pmu : 42,713,496|42,889,096|175,600|0.4%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519b0e62696b0&...

General Discussion page (forum/1)

TEST ONE

=== 8.x..8.x compared (519b1557bbc04..519b171e1fb74):

ct  : 54,294|54,159|-135|-0.2%
wt  : 730,158|727,503|-2,655|-0.4%
cpu : 720,692|718,314|-2,378|-0.3%
mu  : 43,656,496|43,641,512|-14,984|-0.0%
pmu : 43,823,792|43,808,448|-15,344|-0.0%

Profiler output

=== 8.x..twig-forum compared (519b1557bbc04..519b17683f305):

ct  : 54,294|51,530|-2,764|-5.1%
wt  : 730,158|714,073|-16,085|-2.2%
cpu : 720,692|702,398|-18,294|-2.5%
mu  : 43,656,496|43,912,720|256,224|0.6%
pmu : 43,823,792|44,047,808|224,016|0.5%

Profiler output

TEST TWO

=== 8.x..8.x compared (519b1557bbc04..519b183413a55):

ct  : 54,294|54,159|-135|-0.2%
wt  : 730,158|726,964|-3,194|-0.4%
cpu : 720,692|717,294|-3,398|-0.5%
mu  : 43,656,496|43,641,512|-14,984|-0.0%
pmu : 43,823,792|43,808,448|-15,344|-0.0%

Profiler output

=== 8.x..twig-forum compared (519b1557bbc04..519b1891a9aa8):

ct  : 54,294|51,530|-2,764|-5.1%
wt  : 730,158|715,836|-14,322|-2.0%
cpu : 720,692|703,581|-17,111|-2.4%
mu  : 43,656,496|43,912,720|256,224|0.6%
pmu : 43,823,792|44,047,808|224,016|0.5%

Profiler output

TEST THREE

=== 8.x..8.x compared (519b1557bbc04..519b1a785aaa6):

ct  : 54,294|54,164|-130|-0.2%
wt  : 730,158|728,164|-1,994|-0.3%
cpu : 720,692|718,510|-2,182|-0.3%
mu  : 43,656,496|43,641,512|-14,984|-0.0%
pmu : 43,823,792|43,808,448|-15,344|-0.0%

">Profiler output

=== 8.x..twig-forum compared (519b1557bbc04..519b1ad213669):

ct  : 54,294|51,530|-2,764|-5.1%
wt  : 730,158|718,119|-12,039|-1.6%
cpu : 720,692|705,612|-15,080|-2.1%
mu  : 43,656,496|43,912,720|256,224|0.6%
pmu : 43,823,792|44,047,808|224,016|0.5%

Profiler output

This looks great. Hey, a preprocess cleanup for performance FTW!

+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch

Committed 21da6fb and pushed to 8.x. Thanks!