Consolidate forum.module and remove call to _theme_table_cell() within template_preprocess_forum_topic_list()

I think there's already an issue for this

Check in forum module perhaps

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

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: You are right. Tried with a clean installation and works like a charm ;)

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

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

Patch no longer applies.

+++ 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?

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:

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

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

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.

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

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

Taking a shot at this.

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.

Patch is empty

D'oh

@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?

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

Updated issue summary.

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'])) {

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:

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.

+++ 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&...

Thanks!

2092343-_theme_table_cell-44.patch no longer applies.

Looks good!

+++ 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.

Committed and pushed to 8.x. Thanks!