Forum icon sc template cleanup

[rica-carv]

Motivation and Context

Removal of direct acces to core e107 functions, as stated in #5424

Description

Converted all code to shortcodes calls

How Has This Been Tested?

Latest e107 from github

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 d7702e1 and detected 0 issues on this pull request.

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 41.8% (-7.1% change).

View more on Code Climate.

[e107help]

@rica-carv You opened #5424 asking whether a template should be reaching into core handlers, nobody answered, and you answered it yourself with this PR. Sorry about the silence on the question; let me answer it properly now, because the instinct is right and the mechanism is not available.

Why {GLYPH} cannot carry this. The constants are not consumed as template text. They are concatenated into PHP strings all over the plugin: forum.php, forum_class.php, forum_viewtopic.php, forum_shortcodes.php and viewforum_shortcodes.php between them use IMAGE_new, IMAGE_nonew, IMAGE_sticky, IMAGE_closed, IMAGE_announce, IMAGE_track and the rest in roughly twenty places, none of which parse what they build.

That matters because the shortcode parser is a single pass:

$ret = preg_replace_callback('#\{([A-Z][^\x02]*?\S)\}#', array(&$this, 'doCode'), (string) $text);

(shortcode_handler.php#L1020)

preg_replace_callback does not re-scan what it substituted, so a {GLYPH:...} token that a shortcode returns is never expanded. The parseTemplate() you added to sc_icon() covers that one call site. Everywhere else the reader would see the literal text {GLYPH:type=fa-star&size=2x} on the page. It is the same rule that makes a {LAN=...} token returned from a shortcode render as itself.

Where that leaves the design question. The $tp in forum_icons_template.php is not really a template calling a handler, it is a PHP file that happens to live in templates/ and whose whole job is to define() constants. Untangling it means giving those icons a real accessor that the consuming code calls, not a token, which is a bigger change than this PR and one worth agreeing on before writing. Worth reviving #5424 with that framing.

I have pushed a commit for two things that stand on their own regardless of what happens to the approach. IMAGE_nonew_popular separated its parms with % instead of &, so parse_str() would have swallowed the whole tail as the glyph name. And e107_admin/history.php and lan_admin.php carried whitespace-only edits unrelated to forum icons, one of them inside the ADLAN_ERR_3 warning text, so both are back to their base state.

Beyond that I would hold this one. Not a rejection of the idea, just of the route.

[rica-carv]

Sorry, but i got lost somewhere....

So, what's your suggestion to have those glyphs displayed?
Keep the toglyph call in the constants? I thought that pure php should not be in the design side....
And that's why i used the glyph shortcode instead, and changed the call to a "second" parsetemplate call.....

But if that's the "wrong" approach, what's the right one?

[e107help]

@rica-carv Not lost at all, and the instinct behind the question is a good one. Let me separate the two halves, because I think one of them is right and the other is aiming at the wrong file.

Why the token form can't carry

parseTemplate() is a single pass. It is one preg_replace_callback (shortcode_handler.php#L1020), and preg_replace_callback never re-scans what a callback returns. So a {GLYPH:...} that arrives inside a shortcode's return value is already past the parser by the time it exists. It prints to the visitor as literal text.

That is why the extra parseTemplate() call is needed at all. The trouble is where it sits. sc_icon() has two exits (viewforum_shortcodes.php#L747-L800):

• {ICON: type=new} with a parm goes through keyIcon() and hits your new call. ✔
• Bare {ICON} with no parm falls past the if(!empty($parm)) to line 763 and returns IMAGE_new raw. ✘

The icon key at forum_viewforum_template.php#L375-L385 uses the parm form, so the legend would render correctly. Every actual thread row (#L75, #L98, #L121, #L306) uses the bare form, so those would show {GLYPH:type=fa-comment&size=2x} as text to every visitor. Exactly the wrong half works.

And the shortcode is only one of the consumers. Those constants are read in 59 places across 7 files (forum.php, forum_class.php, forum_viewtopic.php, forum_template.php, and all three shortcode batches), and most of them concatenate straight into a PHP string:

return "<a href='".$url."'>".IMAGE_new.'</a>';

Nothing there parses what it builds. Chasing that with 59 parseTemplate() calls would be a lot of new surface, plus a parser pass per icon per row, to end up exactly where the constants already were.

The file isn't the kind of template you think it is

Here's where I think your instinct is sound and the target is off. You're reading forum_icons_template.php as design-side because of the name and the folder. But core doesn't parse it, core require_onces it (forum_class.php#L64-L81) and expects 40-odd define() calls to come back. It's a constants file wearing a template's name.

So $tp->toGlyph() in it isn't PHP leaking into the design layer. It's the same kind of thing as img_path('new.png') and the LAN_FORUM_4001 interpolation on the non-FontAwesome branch six lines below, which have been there since v2 shipped. The contract of that file is "hand core finished HTML", and every consumer depends on that.

The thing genuinely worth complaining about is that a theme author who wants to swap one icon has to open a PHP file and edit a function call. That's a fair complaint. It's just not fixed by changing what the constants contain.

What I'd actually build

Let the theme file be data, and let core turn data into constants. Roughly:

// theme's forum_icons_template.php, no function calls
$FORUM_ICONS = array(
	'new'          => array('fa-star',     'size=2x'),
	'nonew'        => array('fa-comment',  'size=2x'),
	'new_small'    => array('fa-star'),
	// ...
);

and one loop in core, after the require_once, that does the toGlyph() and the define() for any constant the file didn't already set. Theme side becomes declarative, core keeps handing finished HTML to all 59 consumers, and every theme that still defines IMAGE_* the old way keeps working untouched because core only fills in the gaps.

That's a much smaller change than it sounds, and it's the one that actually delivers what you're after. It also gives you somewhere sensible to put the alt/title text, which the FontAwesome branch currently drops on the floor entirely (compare the two halves of the file: the <img> branch sets alt and title from LAN constants, the glyph branch sets neither).

I'll be straight that whether it's worth the churn is @Deltik's call, not mine. If he's up for it I'd say open it as a fresh PR against the icons file alone, and this one can close.

Meanwhile, on this PR

The % to & fix I pushed in 9c9105d stands on its own merit either way, and the two unrelated files are reverted. If you'd rather keep this branch alive as the "icons deserve better" ticket, that works too, just say which way you want to go and I'll follow.

Obrigado for pushing on it. The question was worth asking and I'd rather you asked than quietly dropped it.