Updated: Comment #N

Problem/Motivation

Forum uses a global for the table header, this is built in the forum manager and accessed in the theme layer

Proposed resolution

12% Less crazy*

Remaining tasks

Patch
Review
Rejoice

User interface changes

None

API changes

New ::getHeader() method on ForumManagerInterface
No global available for those rogues who like them

*Actual results may vary. Please see a doctor if symptoms persist.

CommentFileSizeAuthor
#11 forum-bizarre-globals.2.patch5.5 KBlarowlan
#11 interdiff.txt6.14 KBlarowlan
#2 forum-bizarre-globals.1.patch3.59 KBlarowlan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Assigned: Unassigned » larowlan

/me takes out crazy stick

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Issue summary: View changes
Status: Active » Needs review
Issue tags: +API change
FileSize
forum-bizarre-globals.1.patch3.59 KB

Tagging as API change because new method on ForumManagerInterface.
Updated issue summary as such.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Issue summary: View changes
Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

Any way to not replace a global by a more convoluted global?

Why isn't this passed to the template directly?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

It looks that form manager need split like #2188287: Split CommentManager in two to separate query and render code

Status: Needs review » Needs work

The last submitted patch, 2: forum-bizarre-globals.1.patch, failed testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

oh and forum.manager -> forum_manager idiot

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

It looks that form manager need split like #2188287: Split CommentManager in two to separate query and render code

agreed, this would make sense to move the other SQL queries out of forum.module too

Why isn't this passed to the template directly?

yes that is better again

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

Added #2205185: Split up ForumManager to create ForumIndexStorage and added to #2068325: [META] Convert entity SQL queries to the Entity Query API

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

other fun thing here is table sort for repository...

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs work » Needs review
FileSize
interdiff.txt6.14 KB
forum-bizarre-globals.2.patch5.5 KB

Changes as per #4

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

other fun thing here is table sort for repository...

Yeah I need to start looking at views again for /forum /forum/x now that we have #1867642: Expose forum tables to views

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

patch is green - chance of more reviews?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/forum/forum.module
@@ -74,7 +74,7 @@ function forum_theme() {
-      'variables' => array('forums' => array(), 'topics' => array(), 'topics_pager' => array(), 'parents' => NULL, 'term' => NULL, 'sortby' => NULL, 'forum_per_page' => NULL),
+      'variables' => array('forums' => array(), 'topics' => array(), 'topics_pager' => array(), 'parents' => NULL, 'term' => NULL, 'sortby' => NULL, 'forum_per_page' => NULL, 'header' => array()),

@@ -657,7 +657,7 @@ function template_preprocess_forums(&$variables) {
+      $forum_topic_list_header = $variables['header'];

this var used in preprocess only, so template do not need adjustments.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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