Task

Convert PHPTemplate templates to Twig templates.

Remaining

  • Patch needs review

Theme function name/template path Conversion status
core/modules/forum/templates/forum-icon.tpl.php converted
core/modules/forum/templates/forum-list.tpl.php converted, also has a #type table conversion issue: #1938906: Convert forum theme tables to table #type
core/modules/forum/templates/forum-submitted.tpl.php converted
core/modules/forum/templates/forum-topic-list.tpl.php converted, also has a #type table conversion issue: #1938906: Convert forum theme tables to table #type
core/modules/forum/templates/forums.tpl.php converted

Testing steps

  1. Enable the Forum module.
  2. Navigate to admin/structure/forum/add/forum, the output should be rendered by forum-form.html.twig.
  3. Navigate to admin/structure/forum/add/container, the output should be rendered by forum-form.html.twig.
  4. Navigate to /forum. The outer container should be rendered by forums.html.twig, the list of forums rendered by forum-list.html.twig, "Last post" (n/a) rendered by forum-submitted.html.twig.
  5. Click "Add new forum topic" and add a test forum topic to the "General discussion" forum.
  6. Navigate to /forum, the topics, posts, and last post columns should be updated.
  7. Navigate to /forum/1. The outer container should be rendered by forums.html.twig, the list of forum topics rendered by forum-topic-list.html.twig, the forum topic icon rendered by forum-icon.html.twig. The topic submission and last reply string should be rendered by forum-submitted.html.twig.
  8. Test further by adding forum containers. A good test set would be a forum on the top level (not in a container), a container containing one forum, and a container containing more than one forum. Add descriptions to some of the forums and containers.

#1938906: Convert forum theme tables to table #type
#1987400: forum.module - Convert theme_ functions to Twig
#1757550: [Meta] Convert core theme functions to Twig templates

CommentFileSizeAuthor
#43 1898418-43-twig-forum-tpl-php.patch25.72 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 55,450 pass(es). View
#43 interdiff.txt1.96 KBjoelpittet
#42 interdiff.txt1.96 KBjoelpittet
#42 1843740-59-views-exposed-form.patch9.49 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 55,536 pass(es). View
#40 1898418-40.patch25.85 KBCottser
FAILED: [[SimpleTest]]: [MySQL] 55,643 pass(es), 0 fail(s), and 150 exception(s). View
#40 interdiff.txt1.19 KBCottser
#38 drupal-1898418-38.patch25.79 KBc4rl
FAILED: [[SimpleTest]]: [MySQL] 55,691 pass(es), 1 fail(s), and 150 exception(s). View
#38 drupal-1898418-38.patch-interdiff.txt1.6 KBc4rl
#35 drupal-1898418-35.patch25.85 KBc4rl
FAILED: [[SimpleTest]]: [MySQL] 55,513 pass(es), 0 fail(s), and 150 exception(s). View
#35 drupal-1898418-35.patch-interdiff.txt3.34 KBc4rl
#31 twig-forum-template-only-1898418-31.patch76.39 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch twig-forum-template-only-1898418-31.patch. Unable to apply patch. See the log in the details link for more information. View
#31 interdiff.txt3.51 KBjenlampton
#30 interdiff.txt2.42 KBshanethehat
#30 twig-forum-template-only-1898418-29.patch24.92 KBshanethehat
PASSED: [[SimpleTest]]: [MySQL] 55,750 pass(es). View
#24 twig-forum-template-only-1898418-24.patch24.67 KBshanethehat
PASSED: [[SimpleTest]]: [MySQL] 55,573 pass(es). View
#20 1898418-20.patch26.35 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 55,372 pass(es). View
#20 interdiff.txt1.95 KBCottser
#19 interdiff.txt1.29 KBCottser
#19 1898418-19.patch27.26 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 55,438 pass(es). View
#16 1898418-16.patch27.17 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 54,449 pass(es). View
#16 interdiff.txt2.04 KBCottser
#15 1898418-15.patch27.25 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 54,473 pass(es). View
#15 interdiff.txt1.55 KBCottser
#14 1898418-14.patch27.23 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 54,473 pass(es). View
#14 interdiff.txt5.53 KBCottser
#12 1898418-11.patch27.4 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 54,319 pass(es). View
#12 interdiff.txt2.7 KBCottser
#11 1898418-10.patch27.45 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 54,299 pass(es). View
#11 interdiff.txt16.47 KBCottser
#7 drupal-forum_twig-1898418-7.patch20.17 KBfloydm
PASSED: [[SimpleTest]]: [MySQL] 52,600 pass(es). View
#4 drupal-forum_twig-1898418-4.patch19.28 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 52,211 pass(es), 0 fail(s), and 15 exception(s). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

c4rl’s picture

Issue tags: +Twig

Tagging

larowlan’s picture

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

joshuarussell’s picture

Assigned: Unassigned » joshuarussell
disasm’s picture

Status: Active » Needs review
FileSize
19.28 KB
FAILED: [[SimpleTest]]: [MySQL] 52,211 pass(es), 0 fail(s), and 15 exception(s). View

Attached is a patch

Status: Needs review » Needs work

The last submitted patch, drupal-forum_twig-1898418-4.patch, failed testing.

Cottser’s picture

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

floydm’s picture

Status: Needs work » Needs review
FileSize
20.17 KB
PASSED: [[SimpleTest]]: [MySQL] 52,600 pass(es). View

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.

Cottser’s picture

Issue tags: +Needs manual testing

Tagging.

Cottser’s picture

Assigned: joshuarussell » Cottser
Issue tags: -Needs manual testing

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

Cottser’s picture

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

Cottser’s picture

Issue summary: View changes

Add conversion summary table

Cottser’s picture

Issue summary: View changes

Add testing steps

Cottser’s picture

Assigned: Cottser » Unassigned
FileSize
16.47 KB
27.45 KB
PASSED: [[SimpleTest]]: [MySQL] 54,299 pass(es). View

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.
Cottser’s picture

FileSize
2.7 KB
27.4 KB
PASSED: [[SimpleTest]]: [MySQL] 54,319 pass(es). View

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

Cottser’s picture

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

Updating docs.

Cottser’s picture

Assigned: Cottser » Unassigned
Status: Needs work » Needs review
FileSize
5.53 KB
27.23 KB
PASSED: [[SimpleTest]]: [MySQL] 54,473 pass(es). View

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

Cottser’s picture

FileSize
1.55 KB
27.25 KB
PASSED: [[SimpleTest]]: [MySQL] 54,473 pass(es). View

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

Cottser’s picture

FileSize
2.04 KB
27.17 KB
PASSED: [[SimpleTest]]: [MySQL] 54,449 pass(es). View

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

Cottser’s picture

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.

Cottser’s picture

Issue summary: View changes

Tweak test steps

Cottser’s picture

Assigned: Unassigned » Cottser
Status: Needs review » Needs work
Cottser’s picture

Status: Needs work » Needs review
FileSize
27.26 KB
PASSED: [[SimpleTest]]: [MySQL] 55,438 pass(es). View
1.29 KB

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

Cottser’s picture

FileSize
1.95 KB
26.35 KB
PASSED: [[SimpleTest]]: [MySQL] 55,372 pass(es). View
Cottser’s picture

Assigned: Cottser » Unassigned

Unassigning, this needs reviews please!

c4rl’s picture

Title: Convert forum module to Twig » forum.module - Convert PHPTemplate templates to Twig
Status: Needs review » Needs work

Per #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to templates rather than theme_ functions. See #1987400: forum.module - Convert theme_ functions to Twig for theme_ function conversion.

c4rl’s picture

Issue summary: View changes

Remove sandbox link

c4rl’s picture

Issue summary: View changes

Remove theme_ function listing

shanethehat’s picture

Assigned: Unassigned » shanethehat
shanethehat’s picture

Assigned: shanethehat » Unassigned
Status: Needs work » Needs review
FileSize
24.67 KB
PASSED: [[SimpleTest]]: [MySQL] 55,573 pass(es). View
Cottser’s picture

Assigned: Unassigned » steveoliver

Thanks @shanethehat :)

@steveoliver, review please!

Cottser’s picture

Issue tags: +Needs profiling

Tagging for profiling.

larowlan’s picture

Looks good to me

steveoliver’s picture

Status: Needs review » Needs work

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.

jenlampton’s picture

Assigned: steveoliver » jenlampton

dibbs.

shanethehat’s picture

Assigned: jenlampton » steveoliver
Status: Needs work » Needs review
FileSize
24.92 KB
PASSED: [[SimpleTest]]: [MySQL] 55,750 pass(es). View
2.42 KB

1-5 done.

jenlampton’s picture

FileSize
3.51 KB
76.39 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch twig-forum-template-only-1898418-31.patch. Unable to apply patch. See the log in the details link for more information. View

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.

Status: Needs review » Needs work

The last submitted patch, twig-forum-template-only-1898418-31.patch, failed testing.

Fabianx’s picture

#30:

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

Assigned: steveoliver » c4rl

Looks like #31 has a bunch of issues due to inclusion of the old JSON LD code, I'll atttempt to fix.

c4rl’s picture

Assigned: c4rl » Unassigned
Status: Needs work » Needs review
FileSize
3.34 KB
25.85 KB
FAILED: [[SimpleTest]]: [MySQL] 55,513 pass(es), 0 fail(s), and 150 exception(s). View

I think this accomplishes what was meant in #31.

Interdiff with respect to #30.

joelpittet’s picture

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

c4rl’s picture

Status: Needs review » Needs work
c4rl’s picture

Status: Needs work » Needs review
FileSize
1.6 KB
25.79 KB
FAILED: [[SimpleTest]]: [MySQL] 55,691 pass(es), 1 fail(s), and 150 exception(s). View

Joel's nits from #36

Status: Needs review » Needs work

The last submitted patch, drupal-1898418-38.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
25.85 KB
FAILED: [[SimpleTest]]: [MySQL] 55,643 pass(es), 0 fail(s), and 150 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 1898418-40.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
9.49 KB
PASSED: [[SimpleTest]]: [MySQL] 55,536 pass(es). View
1.96 KB

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.

joelpittet’s picture

FileSize
1.96 KB
25.72 KB
PASSED: [[SimpleTest]]: [MySQL] 55,450 pass(es). View

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

Cottser’s picture

Assigned: Unassigned » steveoliver

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

steveoliver’s picture

Assigned: steveoliver » Unassigned

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.

jenlampton’s picture

Assigned: Unassigned » jenlampton

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

jenlampton’s picture

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

jenlampton’s picture

Assigned: jenlampton » Unassigned

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

steveoliver’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Title: forum.module - Convert PHPTemplate templates to Twig » [READY] forum.module - Convert PHPTemplate templates to Twig
Status: Reviewed & tested by the community » Closed (duplicate)
alexpott’s picture

Title: [READY] forum.module - Convert PHPTemplate templates to Twig » forum.module - Convert PHPTemplate templates to Twig
Status: Closed (duplicate) » Fixed

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

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

Anonymous’s picture

Issue summary: View changes

Related issues