Top.php update to use templates instead of hardcoded HTML

[rica-carv]

Motivation and Context

Top.php updated to use forum templates, instead of hardcoded HTML
Hardcoded HTML is still in file for use when template is not available...

Description

Updated top.php to use the newforumposts template
Work in progress...
The top.php file is merging several plugins, so i ask why not use a top.php file in every plugin instead of a common one?

How Has This Been Tested?

Tested in v.2.3.3

Types of Changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (a change to man pages or other documentation)

Checklist

• My code adheres to the e107 code style standard.
• I have read the contributing document.
• I have added tests to cover my changes.
• All new and existing tests pass.

[qlty-cloud-legacy]

Code Climate has analyzed commit e08c9b3 and detected 6 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Bug Risk	6

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 48.9% (0.0% change).

View more on Code Climate.

[e107help]

@rica-carv Bad news on the timing for this one, and it isn't your fault. top.php has been rewritten underneath the branch.

The target moved

Master's active-forum-topics block no longer builds a SQL string at all. It uses the query builder (top.php#L60-L72):

$qb = $sql->createQueryBuilder();
$rows = $qb
	->select('t.*', 'u.user_name')->selectAs('ul.user_name', 'user_last')->addSelect('f.forum_name')
	->from('forum_thread', 't')
	->leftJoin('forum', 'f', $qb->expr()->compareColumns('f.forum_id', 't.thread_forum_id'))
	...
	->fetchAll();

The diff here still patches $sql->gen($qry) and the while($row = $sql->fetch()) loop, and adds f.forum_sef to a hand-written SELECT that no longer exists. $forumList is now the array itself rather than an implode()d string, because whereIn() takes the array.

So this needs redoing against the new shape rather than merging. Sorry, that's a real cost for work you already did. The good news is the new form is shorter: adding the SEF column is ->addSelect('f.forum_sef') and the row loop becomes foreach($rows as $row).

Things to carry over into the rewrite

The rest of the review still applies, so here it is for when you come back to it.

The legacy path becomes unreachable. if ($template = e107::getTemplate('forum','newforumposts_menu','main')) is an assignment, and $NEWFORUMPOSTS_MENU_TEMPLATE['main'] always exists in core (newforumposts_menu_template.php#L37-L57). So the condition is always true, the else block is dead code, and every site switches to the templated output on upgrade whether or not it wanted to. If that's the intent, delete the old branch and say so in the PR. If it isn't, the condition needs to test for something a site can actually influence.

Uninitialised counters. $total_topics, $total_views and $total_replies are incremented with ++ and += but never initialised. PHP 8 emits Undefined variable for each on the first iteration. Set them to 0 before the loop.

Duplication. This reproduces the row-building loop that newforumposts_menu.php already has. Two copies of the same logic drift apart; a shared helper (or having top.php call the menu's renderer) would age better. Not a blocker, but worth thinking about while the code is being rewritten anyway.

Housekeeping. The commented-out scaffolding (//if (1 == 2), // $sql1 = $sql;, the var_dump, the $fsef line) wants deleting, and the else block keeps its original indentation so the braces don't line up with the new if.

$forum->getThreadSef() does exist (forum_class.php#L1430) and $tp is in scope from class2.php, so those two are fine.

Say the word if you'd rather we picked this one up and rewrote it against the builder ourselves, with credit to you on the commit. Given master moved after you'd done the work, that seems only fair.