Some forum code cleanup

[rica-carv]

Motivation and Context

Forum code has unused variables, and some hardcoded shortcodes. This is probably the start of that changes...

Description

Cleared some variables unecessary inside code scope

How Has This Been Tested?

Tested with v2.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 35748ff and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 81.8% (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 I have left this branch alone, because the three things inside it want three different answers and I did not want to decide that for you.

The dead-variable removals are fine. $tp in e107forum::__construct(), $e107 in getTrackedThreadList(), $firstpost: all genuinely unused, and your comments asking why they were declared are fair questions. This part could go in on its own tomorrow.

Retiring the LAN shortcodes is a compatibility break. Commenting out sc_threadtitle(), sc_replytitle(), sc_lastpostitle(), sc_infotitle(), sc_newthreadtitle(), sc_postedtitle() and sc_tracktitle() while switching core's template to {LAN=FORUM_0002} is fine for core, which now supplies both sides. It is not fine for a theme carrying its own forum/forum_template.php: those files still contain {THREADTITLE} and friends, and with the methods gone the column headings render as nothing. The safe version of this change is to switch core's template to the {LAN=...} form, which is what the "How it should be???" comment above each block has been asking for, and leave the shortcode methods in place for the templates that still call them. Cheap, and nobody's forum loses its table headers on upgrade.

The redirect in forum_track() will not fire. This part:

if(!$text)
{
	e107::redirect();
	exit;
}

runs at forum.php:369 onwards, but HEADERF was already included at forum.php#L106, two lines before forum_track() is called. The page header is out the door, so header() cannot be sent and the reader gets a "headers already sent" warning on a half-drawn page. Separately, e107::redirect() defaults to 301 (e107_class.php#L4902), so even where it did work the browser would cache "no tracked threads" and refuse to load the page again after the reader tracked something. If the goal is "do not show an empty tracking page", the fix belongs before HEADERF, and as a 302 at most.

The branch also conflicts with master now, in forum_class.php and forum_shortcodes.php.

My suggestion is to split the dead-variable commit out into its own PR so it can land, and let the other two be discussed separately. Happy to do the splitting if you would rather not; just say. @Deltik has the final word on all three.

[rica-carv]

Well, if you can do the splits, go ahead... 👍

[Deltik]

@e107help: Sure, let's land the pure cleanup in a separate pull request of your own. I don't have the capacity for the other parts, so I encourage you and @rica-carv to hash it out.

[e107help]

@rica-carv Following @Deltik's steer above, I've lifted the pure cleanup out into #5876 so it can land on its own. You're credited as co-author on the commit.

That's the four dead locals you spotted ($tp in the constructor, $e107 in getTrackedThreadList(), global $forum in sc_track(), and the $trackedThreadList capture in forum_track()), plus one thing they led me to: $tracktext is only assigned inside that if and concatenated unconditionally afterwards, so a user with nothing tracked was reading an undefined variable. Initialising it to '' fixes that without changing the rendered output.

All four of your comments were right.

Good news on the {ICONKEY} hunk: it's already in

I went to review that one and found master already does exactly what this PR proposes, down to the same line:

$FORUM_TEMPLATE = varset($GLOBALS['FORUM_TEMPLATE']);
return (defset('BOOTSTRAP') && !empty($FORUM_TEMPLATE['iconkey'])) ? e107::getParser()->parseTemplate($FORUM_TEMPLATE['iconkey'], true, $this) : ...

That's forum_shortcodes.php#L87-L88 on master today, and it came from your own commit cb2c1803, 11 August 2024. It went in through a different PR while this one sat. So that hunk is now a duplicate, and it's part of why this branch conflicts.

(For what it's worth I'd rather core reached that through e107::getTemplate('forum', 'forum', 'iconkey', true, true) than through $GLOBALS, because the globals route misses theme overrides that arrive via the registry. But that's a criticism of master, not of your PR, so it's a separate change.)

That leaves two things here, and Deltik has said he doesn't have capacity to arbitrate, so it's yours and mine to settle.

1. The redirect when nothing is tracked

if(!$text)
{
	e107::redirect();
	exit;
}

This can't fire where it sits. HEADERF is included at forum.php#L106, two lines before forum_track() is called at #L108, so the page head has already gone to the browser and header() has nothing left to do. And e107::redirect() defaults to 301 (e107_class.php#L4902); if it ever did fire, browsers would cache "the tracking page goes to the forum index" and the user would have a hard time getting back to it once they'd tracked something.

The complaint underneath it is fair: an empty tracking page currently renders a table header with nothing under it. I'd rather fix that by rendering a proper empty state in place of the table (which needs a new LAN string, since I couldn't find an existing one that fits) than by navigating the user somewhere they didn't ask to go. Would that work for you?

2. The seven legacy title shortcodes

Commenting out sc_threadtitle() and friends and swapping the template to {LAN=FORUM_0002} is the part I'd slow down on. Those tokens are in the default template, which means any theme that copied it (which is how forum theming has worked for twenty years) has {THREADTITLE} in its own file. Delete the method and those themes render an empty table header, with nothing in the logs to explain it.

I'm not against the direction; {LAN=...} is plainly better than seven methods that each return one constant. It wants to be a deprecation rather than a deletion: leave the methods, mark them @deprecated pointing at the {LAN=} form, change the core template, and drop them in a later major. Reshape it that way and I'll review it properly.

3. The lastpost shortcodes

sc_topic_lastpost_date and sc_topic_lastpost_author lose their empty-replies guards here, and the return ' - '; fallback goes with them, so a thread with no replies returns something different. I couldn't tell from the diff whether that's the point of the change or a side effect of clearing the guards. What were you seeing that prompted it?

Suggested way forward

Once #5876 lands, rebase this on master and both the cleanup and the {ICONKEY} hunk will drop out of your diff on their own. What's left is small enough to decide in one pass. If you'd rather hand any of it over, say so and I'll pick it up with credit, same as last time.