Problem/Motivation

Remove forum_topic_list and forum_list template/theme functions to consolidate.

forum_topic_list is building a sortable table, but in doing so it's hardcoding a call to _theme_table_cell() which is being removed from the theme layer. @see #1939008: Convert theme_table() to Twig

Proposed resolution

Convert the twig template for forum-topic-list.html.twig to a #theme => table to remove the dependency on that private theme function.

User interface changes

N/A

API changes

N/A

Follow-up from #1939008: Convert theme_table() to Twig.
#1804614: [meta] Consolidate theme functions and properly use theme suggestions in core

Comments

larowlan’s picture

I think there's already an issue for this

larowlan’s picture

Check in forum module perhaps

larowlan’s picture

Oh, you split this from #1938906: Convert forum theme tables to table #type, carry on :)

joelpittet’s picture

Assigned: joelpittet » Unassigned
Status: Active » Needs review
Issue tags: -Needs patch
FileSize
6.12 KB
PASSED: [[SimpleTest]]: [MySQL] 58,701 pass(es). View

While it feels a bit dirty to put markup back into PHP:S This is probably for the 'greater good'...

This removed the dependency of _theme_table_cell call and tablesort from the forum module, also let's the #theme => table do the zebra and build the headers.

So if this can get in, the #theme=>table to twig can be simplified a bit and remove the forum related code and @todo from the patch.

joelpittet’s picture

Issue tags: +Novice, +Needs manual testing

Tagging.

rteijeiro’s picture

Status: Needs review » Needs work

It seems that the patch removed topic_id and header variables:

User warning: topic_id could not be found in _context in "core/themes/bartik/templates/page.html.twig" at line 167 in Drupal\Core\Template\TwigTemplate->getContextReference() (line 51 of core/lib/Drupal/Core/Template/TwigTemplate.php).

User warning: header could not be found in _context in "core/themes/bartik/templates/page.html.twig" at line 167 in Drupal\Core\Template\TwigTemplate->getContextReference() (line 51 of core/lib/Drupal/Core/Template/TwigTemplate.php).
joelpittet’s picture

@rteijeiro re #6 That looks like a caching issue. Delete /sites/default/files/php to remove the compiled twig files. I'm sure that it because there is no topic_id in the entire core after this patch is applied.

rteijeiro’s picture

Status: Needs work » Reviewed & tested by the community

@joelpittet: You are right. Tried with a clean installation and works like a charm ;)

It's a RTBC for me and patch still applies!!

joelpittet’s picture

Status: Reviewed & tested by the community » Needs review

Thank you for the RTBC:) I think we still need to confirm the markup is identical to before so I didn't accidently break. It would be great to see an HTML diff for forum_topic_list

Steps to compare:

  • Enable forum module
  • Add a couple forum topics to the General Discussion forum.
  • Turn on twig debug so you have the right template (in this case the template will be the same: forum-topic-list.html.twig)
  • Head over to /forum/1 and copy the HTML
  • Apply this patch and maybe add something to the twig markup to show the change is getting picked up.
  • copy the the HTML from /forum/1 and compare
rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Just compared markup before and after patch apply.

The only one difference is the "active" class in the replies cell:

<td class="replies active">

It's still a RTBC :P

joelpittet’s picture

Issue tags: -Novice, -Needs manual testing +Twig

Thank you @rteijeiro!
RTBC++

alexpott’s picture

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

Patch no longer applies.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
6.14 KB
PASSED: [[SimpleTest]]: [MySQL] 59,188 pass(es). View

Re-rolled

markcarver’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/forum/templates/forum-topic-list.html.twig
@@ -4,65 +4,13 @@
+{{ table }}

Why do we still have a template file for this then?

markcarver’s picture

FileSize
215.32 KB

To clarify:

If the "form_list" theme function is really just a "table" in disguise, then whatever is calling this theme function should just return '#theme' => 'table' in the render array. Which means this special theme function can be removed entirely. We need to stop doing:
theme_wtf.jpg

edit: Which in Twig the equivalent in this case would be: {{ table }}.

joelpittet’s picture

@Mark Carver probably don't need one... but not sure exactly how to let it still be themable. I saw steveoliver and a few others use something like table__forum_topic_list to provide a theme suggestion... but I think that only works for 'type' => 'table'? And 'type' => 'table' seems to be really slow right now and a pain to build out... @see #1938926-29: Convert simpletest theme tables to table #type

So any suggestions? My main concern is that this is a blocker for theme_table converted to twig as it has the hard-coded dependencies for the table sorting. So all this table needs is the ability to sort which theme_table has and it's faster.

joelpittet’s picture

Title: Remove call to _theme_table_cell() within template_preprocess_forum_topic_list » Conlidate forum.module and Remove call to _theme_table_cell() within template_preprocess_forum_topic_list
Assigned: Unassigned » joelpittet
Issue tags: +d8dtx, +theme system cleanup, +Theme Component Library, +Template consolidation

Moving this to our consolidation meta #1804614: [meta] Consolidate theme functions and properly use theme suggestions in core

Going to make helper functions and drop forum_list and forum_topic_list templates.

c4rl’s picture

@joelpittet #17 What do you mean by "helper functions?"

To me, it seems like most things here in forum_theme() are all sub-calls of #theme:forums, which means we can simply consolidate everything into a single callback. If this callback is complicated (and it could be) it may justify providing a template after all for the entire thing instead of having some incredibly verbose #theme:table. So, let's consider consolidating everything into 1 template as well as consolidating everything into something that #theme:table can handle.

Make sense? :)

markcarver’s picture

Assigned: joelpittet » markcarver

Taking over this one to provide an example of why one one line templates are bad and how we should utilize render arrays the way they're meant to be used.

joelpittet’s picture

Assigned: markcarver » Unassigned
markcarver’s picture

Assigned: Unassigned » markcarver
Cottser’s picture

Title: Conlidate forum.module and Remove call to _theme_table_cell() within template_preprocess_forum_topic_list » Consolidate forum.module and remove call to _theme_table_cell() within template_preprocess_forum_topic_list()

Just cleaning up the title slightly, don't mind me.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
4.14 KB
8.35 KB
FAILED: [[SimpleTest]]: [MySQL] 58,852 pass(es), 1 fail(s), and 0 exception(s). View

I have a feeling forum has no render tests... but anyways, here's a partial patch, @Mark Carver let me know if this goes in the direction you're looking for?

Status: Needs review » Needs work

The last submitted patch, 2092343-23_theme_table_cell.patch, failed testing.

c4rl’s picture

This isn't comprehensive enough. As I explained in #18 *all* theme callbacks with the exception of #theme:forums are useless:

  • #theme:forum_list and #theme:forum_topic_list are only called by #theme:forums so they should just be part of #theme:forums
  • Moreover #theme:forum_icon and #theme:forum_submitted are only called by #theme:forum_topic_list so they should just be part of #theme:forums as well
  • #theme:forum_form is absolutely useless.

Willing to hear arguments to the contrary. :)

c4rl’s picture

Assigned: markcarver » c4rl

Taking a shot at this.

c4rl’s picture

Status: Needs work » Needs review
FileSize
0 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,862 pass(es). View

After inspecting the real purposes of forum_theme(), I discovered that there were really two fundamental callbacks: #theme:form_list and #theme:forum_topic_list. The manner in which #theme:forums was being used was fairly strange and I elected to change that behavior for this patch.

So, #theme:forums, #theme:form_submitted, and #theme:forum_icon have been removed as well as #theme:forum_form and _theme_table_cell().

This takes a completely different approach than previous patches, so I don't think an interdiff is particularly useful.

markcarver’s picture

Status: Needs review » Needs work

Patch is empty

c4rl’s picture

Status: Needs work » Needs review
FileSize
25.2 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-2092343-29.patch. Unable to apply patch. See the log in the details link for more information. View

D'oh

Status: Needs review » Needs work

The last submitted patch, drupal-2092343-29.patch, failed testing.

markcarver’s picture

@c4rl, the forum_list and form_topic_list theme hooks are still ultimately tables. It doesn't make sense to create separate theme hooks and should instead do table__forum_list and just create separate template suggestions like table--forum-list.html.twig for the custom forum markup.

Given that we cannot currently preprocess theme suggestions until #2035055: Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK() is in, perhaps you can simply provide a generic hook_preprocess_table() preprocess and check the theme hook in the $variables to catch preprocessing until that gets in?

c4rl’s picture

Spoke with @Mark Carver about this in IRC.

We have two options here:

  1. Leave forum_list and forum_topic_list as-is with duplicate markup to what table provides (i.e., what I was suggesting in #29.
  2. Per @Mark Carver's suggestion in #31, use separate theme hook suggestions based on table, but that is currently blocked by #2035055: Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK() if we want to do it right.

Aside from the specifics of which hook to use for those two functions, I'm hoping the general approach of #29 is better. In the long-term, these being lists should probably be somehow converted to work as a view (@todo link/create issue?) since they indeed fall into the fabled realm of "lists of stuff."

So I'm going to dive-in to #2035055: Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK() for now.

Aside from fixing my exceptions in #29 this will also need rebase now that #2028113: Make forum module css a library is in. :)

c4rl’s picture

Issue summary: View changes

Updated issue summary.

joelpittet’s picture

Status: Needs work » Needs review

29: drupal-2092343-29.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 29: drupal-2092343-29.patch, failed testing.

joelpittet’s picture

Assigned: c4rl » Unassigned
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs Reroll

joelpittet’s picture

Status: Needs work » Needs review
FileSize
12.42 KB
FAILED: [[SimpleTest]]: [MySQL] 59,284 pass(es), 0 fail(s), and 4 exception(s). View
9.16 KB

Ok here's a compromise. The interdiff is from #2092343-23: Consolidate forum.module and remove call to _theme_table_cell() within template_preprocess_forum_topic_list()

I fixed the pager missing, and removed the helper function as well. I didn't convert forum-list.html.twig because that table is a bit more complicated to get right and probably should be it's own issue, but this patch gets the _theme_table_cell() call out and removes theme function for #theme=>table and makes it a suggestion instead.

I think this is good enough for this patch and make another follow-up issue for forum-list to #theme=>table or #type=>table...

The last submitted patch, 36: 2092343-36-_theme_table_cell.patch, failed testing.

joelpittet’s picture

FileSize
12.85 KB
PASSED: [[SimpleTest]]: [MySQL] 59,353 pass(es). View
779 bytes

Need to set a default variable for the pager.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll

This looks like good cleanup to me, if it's a table let's treat it like one.

Two things stood out to me:

1. We need to set '#responsive' => FALSE in the render array for table__forum_topic_list to get the markup to match up (the active class mentioned in #10 and also responsive-enabled class on the table) and importantly also prevent loading extra JS libraries.

2. I think we should initialize not just topics_pager but also the topics and forums variables as empty arrays at the very top of template_preprocess_forums() (especially since we're making it bigger). That would allow us to remove a bunch of unneeded else statements further down, and we could also consider an early return if this condition doesn't evaluate to TRUE:

if ($variables['forums_defined'] = count($variables['forums']) || count($variables['parents'])) {
Cottser’s picture

One more small thing:

I think $variables['topics'] = $table; should only be set if there are topics to display.

Check the source on /forum and you'll find an extra empty table:

<table id="forum-topic-0">
</table>

On HEAD it's less empty:

<table id="forum-topic-0">
  <thead>
    <tr></tr>
  </thead>
  <tbody>
    </tbody>
</table>

Again an early return might make sense here because if there are no topics then there is no topic pager either and the rest of the preprocess is not needed, including preparing the #theme table render array. The change from HEAD would be that when viewing a forum like /forum/1 with no topics posted we wouldn't get the empty table with headers only. But that's a bugfix IMO:

2092343-forum-topic-empty-table

And I'd just like to acknowledge that yes the global $forum_topic_list_header is yucky but that's not being added here and should be a separate refactor IMO.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
13.32 KB
PASSED: [[SimpleTest]]: [MySQL] 59,873 pass(es). View
  1. Moved the arrays into the theme definition
  2. Removed the elses that set the empty arrays
  3. Removed a redundant loop on topics.
  4. Set #responsive to false for closer markup and less JS load.
  5. And a twig docblock clean-up.
joelpittet’s picture

FileSize
9.8 KB
Cottser’s picture

Status: Needs review » Needs work
+++ b/core/modules/forum/templates/forums.html.twig
@@ -7,7 +7,7 @@
  * Available variables:
  * - forums: The forums to display (as processed by forum-list.html.twig).
- * - topics: The topics to display (as processed by forum-topic-list.html.twig).
+ * - topics: The topics to display.
  * - forums_defined: A flag to indicate that the forums are configured.
  *
  * @see template_preprocess_forums()
@@ -19,5 +19,6 @@

@@ -19,5 +19,6 @@
   <div id="forum">
     {{ forums }}
     {{ topics }}
+    {{ topics_pager }}

We need to document topics_pager in this Twig template.

Other than that this looks great to me. Here are some profiling results, looking very good!

Default install, enable forum module, /forum:

=== 8.x..8.x compared (52d3e812c7cf2..52d3e8415aec3):

ct  : 44,239|44,239|0|0.0%
wt  : 160,218|160,335|117|0.1%
cpu : 149,595|149,483|-112|-0.1%
mu  : 14,283,168|14,283,168|0|0.0%
pmu : 14,361,208|14,361,208|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52d3e812c7cf2&...

=== 8.x..forum-2092343-41 compared (52d3e812c7cf2..52d3e85377bba):

ct  : 44,239|43,864|-375|-0.8%
wt  : 160,218|158,531|-1,687|-1.1%
cpu : 149,595|147,981|-1,614|-1.1%
mu  : 14,283,168|14,164,264|-118,904|-0.8%
pmu : 14,361,208|14,241,632|-119,576|-0.8%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52d3e812c7cf2&...

Default install, enable forum module, /forum/1:

=== 8.x..8.x compared (52d3e93cb560e..52d3e964b719c):

ct  : 46,112|46,112|0|0.0%
wt  : 164,866|165,165|299|0.2%
cpu : 153,528|153,810|282|0.2%
mu  : 14,254,064|14,254,064|0|0.0%
pmu : 14,340,504|14,340,504|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52d3e93cb560e&...

=== 8.x..forum-2092343-41 compared (52d3e93cb560e..52d3e983be01d):

ct  : 46,112|45,120|-992|-2.2%
wt  : 164,866|161,334|-3,532|-2.1%
cpu : 153,528|150,368|-3,160|-2.1%
mu  : 14,254,064|14,109,864|-144,200|-1.0%
pmu : 14,340,504|14,195,704|-144,800|-1.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52d3e93cb560e&...

Default install, enable forum module, add 26 topics, /forum:

=== 8.x..8.x compared (52d4b590e6e18..52d4b5e5ddba1):

ct  : 45,751|45,751|0|0.0%
wt  : 163,509|163,803|294|0.2%
cpu : 151,958|152,315|357|0.2%
mu  : 14,590,240|14,590,240|0|0.0%
pmu : 14,670,320|14,670,320|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52d4b590e6e18&...

=== 8.x..forum-2092343-41 compared (52d4b590e6e18..52d4b60c154f6):

ct  : 45,751|45,376|-375|-0.8%
wt  : 163,509|160,825|-2,684|-1.6%
cpu : 151,958|149,642|-2,316|-1.5%
mu  : 14,590,240|14,472,144|-118,096|-0.8%
pmu : 14,670,320|14,551,648|-118,672|-0.8%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52d4b590e6e18&...

Default install, enable forum module, add 26 topics, /forum/1:

=== 8.x..8.x compared (52d4b18f32fb6..52d4b1d6bf728):

ct  : 95,307|95,305|-2|-0.0%
wt  : 288,324|288,438|114|0.0%
cpu : 274,144|274,101|-43|-0.0%
mu  : 16,788,032|16,788,024|-8|-0.0%
pmu : 16,914,192|16,914,184|-8|-0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52d4b18f32fb6&...

=== 8.x..forum-2092343-41 compared (52d4b18f32fb6..52d4b1f732b14):

ct  : 95,307|94,704|-603|-0.6%
wt  : 288,324|286,502|-1,822|-0.6%
cpu : 274,144|272,304|-1,840|-0.7%
mu  : 16,788,032|16,700,216|-87,816|-0.5%
pmu : 16,914,192|16,829,160|-85,032|-0.5%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52d4b18f32fb6&...

joelpittet’s picture

Status: Needs work » Needs review
FileSize
13.36 KB
PASSED: [[SimpleTest]]: [MySQL] 63,213 pass(es). View
701 bytes

Sweet performance gain:) Here's the docblock.

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

alexpott’s picture

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

2092343-_theme_table_cell-44.patch no longer applies.


alexpott’s picture

Issue summary: View changes
joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
13.41 KB
FAILED: [[SimpleTest]]: [MySQL] 63,610 pass(es), 22 fail(s), and 9 exception(s). View

Re-rolled.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
13.4 KB
PASSED: [[SimpleTest]]: [MySQL] 63,636 pass(es). View
897 bytes

Back to rtbc, the re-roll in 48 was wrong but the diff between that re-roll and the previous patch shows the mistake(change) and i've applied it to this patch.

The last submitted patch, 48: 2092343-_theme_table_cell-48.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 49: 2092343-_theme_table_cell-49.patch, failed testing.

joelpittet’s picture

Cottser’s picture

Status: Needs work » Reviewed & tested by the community

Looks good!

Cottser’s picture

+++ b/core/modules/forum/forum.module
@@ -736,93 +812,6 @@ function template_preprocess_forum_list(&$variables) {
-    /** @var \Drupal\node\NodeInterface $topic */

I guess we might want to add this back, but that's minor.

joelpittet’s picture

FileSize
13.46 KB
PASSED: [[SimpleTest]]: [MySQL] 63,636 pass(es). View
664 bytes

Thanks @Cottser, I thought I had that in there. Added back.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 55: 2092343-_theme_table_cell-55.patch, failed testing.

joelpittet’s picture

Cottser’s picture

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

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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