Fall back instead of rendering broken roster images (#389)

portal/modules/roster.php

@@ -302,8 +302,8 @@ protected function display_listing(array $characters, string $ext_path_images, s
 				'ARMORY'      => $char['player_armory_url'],
 				'PHPBBUID'    => $char['username'],
 				'ACHIEVPTS'   => $char['player_achiev'],
-				'CLASS_IMAGE' => $ext_path_images . 'class_images/' . basename($char['class_image']),
-				'RACE_IMAGE'  => $ext_path_images . 'race_images/' . basename($char['race_image']),
+				'CLASS_IMAGE' => $this->resolve_game_image($ext_path_images, $this->character_image_candidates('class_images', (string) $char['class_image'])),
+				'RACE_IMAGE'  => $this->resolve_game_image($ext_path_images, $this->character_image_candidates('race_images', (string) $char['race_image'])),
 				'SPEC'        => $spec['name'],
 				'SPEC_ICON'   => $spec['icon'],
 				'U_PLAYER_DETAIL' => $this->helper->route('avathar_bbguild_player', [
@@ -374,7 +374,7 @@ protected function display_grid(player $players, array $characters, string $ext_
 
 			foreach ($classes as $classid => $class)
 			{
-				$classimgurl = $ext_path_images . 'roster_classes/' . $class['imagename'] . '.png';
+				$classimgurl = $this->resolve_game_image($ext_path_images, $this->class_image_candidates($class['imagename']));
 
 				$this->template->assign_block_vars('class', [
 					'CLASSNAME' => $class['name'],
@@ -402,8 +402,8 @@ protected function display_grid(player $players, array $characters, string $ext_
 							'SPEC'      => $grid_spec['name'],
 							'SPEC_ICON' => $grid_spec['icon'],
 							'ACHIEVPTS' => $char['player_achiev'],
-							'CLASS_IMAGE' => $ext_path_images . 'class_images/' . basename($char['class_image']),
-							'RACE_IMAGE'  => $ext_path_images . 'race_images/' . basename($char['race_image']),
+							'CLASS_IMAGE' => $this->resolve_game_image($ext_path_images, $this->character_image_candidates('class_images', (string) $char['class_image'])),
+							'RACE_IMAGE'  => $this->resolve_game_image($ext_path_images, $this->character_image_candidates('race_images', (string) $char['race_image'])),
 							'U_PLAYER_DETAIL' => $this->helper->route('avathar_bbguild_player', [
 								'guild_id'  => $this->guild_id,
 								'player_id' => $char['player_id'],
@@ -514,6 +514,113 @@ protected function resolve_portrait_with_fallback(array $char, string $ext_path_
 		return '';
 	}
 
+	/**
+	 * Ordered candidate paths for a class's roster image, relative to a
+	 * game's images/ directory.
+	 *
+	 * The grid's preferred asset is the large roster_classes/ artwork, but
+	 * the smaller class_images/ detail icon is a better fallback than a
+	 * broken image, and a game's <prefix>_unknown icon is better than
+	 * nothing at all. Plugins name every asset <game>_<thing>, so the
+	 * prefix of the class's own imagename identifies its unknown icon.
+	 *
+	 * @param string $imagename Class imagename as stored in the DB
+	 * @return list<string> Candidate paths, best first; empty if unusable
+	 */
+	protected function class_image_candidates(string $imagename): array
+	{
+		// DB-sourced value used in a path: keep the basename only.
+		$name = basename(trim($imagename));
+
+		if ($name === '' || $name === '.' || $name === '..')
+		{
+			return [];
+		}
+
+		$candidates = [
+			'roster_classes/' . $name . '.png',
+			'class_images/' . $name . '.png',
+		];
+
+		$pos = strpos($name, '_');
+
+		if ($pos !== false && $pos > 0)
+		{
+			$prefix = substr($name, 0, $pos);
+			$candidates[] = 'roster_classes/' . $prefix . '_unknown.png';
+			$candidates[] = 'class_images/' . $prefix . '_unknown.png';
+		}
+
+		return $candidates;
+	}
+
+	/**
+	 * Ordered candidate paths for a character's own class/race image.
+	 *
+	 * Unlike class_image_candidates() the DB already stores a filename with
+	 * its extension here, and there is no second directory to fall back to
+	 * — so the only fallback is the game's unknown icon in the same
+	 * directory, which still beats a broken image.
+	 *
+	 * @param string $dir      Image directory, e.g. class_images
+	 * @param string $filename Filename as stored in the DB, with extension
+	 * @return list<string> Candidate paths, best first; empty if unusable
+	 */
+	protected function character_image_candidates(string $dir, string $filename): array
+	{
+		$name = basename(trim($filename));
+
+		if ($name === '' || $name === '.' || $name === '..')
+		{
+			return [];
+		}
+
+		$candidates = [$dir . '/' . $name];
+
+		$pos = strpos($name, '_');
+
+		if ($pos !== false && $pos > 0)
+		{
+			$candidates[] = $dir . '/' . substr($name, 0, $pos) . '_unknown.png';
+		}
+
+		return $candidates;
+	}
+
+	/**
+	 * First candidate that exists on disk, as a web URL.
+	 *
+	 * $ext_path_images is a web path, so it is re-anchored at its ext/
+	 * segment to test the file on disk — same approach as
+	 * resolve_portrait_with_fallback().
+	 *
+	 * @param string       $ext_path_images Web path to a game's images/
+	 * @param list<string> $rel_candidates  Paths relative to that directory
+	 * @return string Web URL of the first existing file, or ''
+	 */
+	protected function resolve_game_image(string $ext_path_images, array $rel_candidates): string
+	{
+		$pos = strpos($ext_path_images, 'ext/');
+
+		if ($pos === false)
+		{
+			return '';
+		}
+
+		global $phpbb_root_path;
+		$fs_base = $phpbb_root_path . substr($ext_path_images, $pos);
+
+		foreach ($rel_candidates as $candidate)
+		{
+			if (file_exists($fs_base . $candidate))
+			{
+				return $ext_path_images . $candidate;
+			}
+		}
+
+		return '';
+	}
+
 	protected function get_game_images_path(string $game_id): string
 	{
 		$web_root = $this->path_helper->get_web_root_path();

styles/all/template/portal/modules/roster_center.html

@@ -103,9 +103,9 @@
 					{% if S_SHOWACH == 1 %}
 					<td style="text-align: center;">{{ row.ACHIEVPTS }}</td>
 					{% endif %}
-					<td style="text-align: left;"><img src="{{ row.RACE_IMAGE }}" alt="" />&nbsp;{{ row.RACE }}</td>
+					<td style="text-align: left;">{% if row.RACE_IMAGE %}<img src="{{ row.RACE_IMAGE }}" alt="" />&nbsp;{% endif %}{{ row.RACE }}</td>
 					<td style="text-align: left;">
-						<img src="{{ row.CLASS_IMAGE }}" alt="{{ row.CLASS }}" />&nbsp;
+						{% if row.CLASS_IMAGE %}<img src="{{ row.CLASS_IMAGE }}" alt="{{ row.CLASS }}" />&nbsp;{% endif %}
 						<span style="white-space: nowrap; color:{{ row.COLORCODE }}">{{ row.CLASS }}</span>
 					</td>
 					{% if S_SHOWSPEC %}
@@ -137,8 +137,8 @@
 	{# ---- Grid view (grouped by class) ---- #}
 	{% for class in loops.class %}
 	<h2>
-		<img src="{{ class.CLASSIMG }}" style="max-width:100px; max-height:100px;" alt="{{ class.CLASSNAME }}" />
-		&nbsp;
+		{% if class.CLASSIMG %}<img src="{{ class.CLASSIMG }}" style="max-width:100px; max-height:100px;" alt="{{ class.CLASSNAME }}" />
+		&nbsp;{% endif %}
 		<span style="color: {{ class.COLORCODE }};">{{ class.CLASSNAME }}</span>
 	</h2>
 
@@ -160,7 +160,7 @@ <h2>
 		{% endif %}
 			<td style="text-align:center; width:100;" class="bg1">
 				<a href="{{ players_row.U_PLAYER_DETAIL }}">
-					{% if players_row.PORTRAIT %}<img src="{{ players_row.PORTRAIT }}" alt=" " width="64" height="64" />{% else %}<img src="{{ players_row.CLASS_IMAGE }}" alt="{{ players_row.CLASS }}" width="64" height="64" />{% endif %}</a>
+					{% if players_row.PORTRAIT %}<img src="{{ players_row.PORTRAIT }}" alt=" " width="64" height="64" />{% elseif players_row.CLASS_IMAGE %}<img src="{{ players_row.CLASS_IMAGE }}" alt="{{ players_row.CLASS }}" width="64" height="64" />{% endif %}</a>
 			</td>
 			<td style="text-align:left; width:100" class="bg2">
 				<strong><a style="color: {{ class.COLORCODE }};" href="{{ players_row.U_PLAYER_DETAIL }}">{{ players_row.NAME }}</a></strong>

tests/portal/roster_image_fallback_test.php

@@ -0,0 +1,160 @@
+<?php
+/**
+ * @package bbGuild Extension
+ * @copyright (c) 2026 avathar.be
+ * @license GNU General Public License, version 2 (GPL-2.0)
+ *
+ * Roster image fallback tests — roster grid / listing icon resolution
+ */
+
+namespace avathar\bbguild\tests\portal;
+
+use avathar\bbguild\portal\modules\roster;
+use PHPUnit\Framework\TestCase;
+
+/**
+ * Exercises the roster's class-image resolution.
+ *
+ * The grid used to build roster_classes/<imagename>.png unconditionally,
+ * so any game plugin missing that one asset rendered a broken image with
+ * no server-side check — the shared root cause behind the icon tickets in
+ * bbguildeq/eq2/gw2/lineage2/lotro/swtor. Behaviour matrix:
+ *  - roster_classes/ has the file         → use it
+ *  - only class_images/ has it            → fall back to the detail icon
+ *  - neither, but <prefix>_unknown exists → fall back to the game's unknown
+ *  - nothing resolves                     → '' (template omits the <img>)
+ *
+ * Resolution is checked against core's own images/{roster_classes,
+ * class_images}/custom_*.png, which happen to cover every branch:
+ * custom_warrior is in both dirs, custom_unknown only in class_images/.
+ * No game plugin is involved, so this holds in core's CI where none are
+ * installed.
+ */
+class roster_image_fallback_test extends TestCase
+{
+	private roster $roster;
+
+	/** Web path in the shape get_game_images_path() returns. */
+	private const IMAGES = './ext/avathar/bbguild/images/';
+
+	protected function setUp(): void
+	{
+		parent::setUp();
+		// Bypass the wide constructor; image resolution touches no
+		// dependency beyond $phpbb_root_path.
+		$this->roster = (new \ReflectionClass(roster::class))->newInstanceWithoutConstructor();
+
+		// tests/portal → tests → bbguild → avathar → ext → board root
+		global $phpbb_root_path;
+		$phpbb_root_path = dirname(__DIR__, 5) . '/';
+	}
+
+	private function invoke(string $method, ...$args)
+	{
+		$m = (new \ReflectionClass(roster::class))->getMethod($method);
+		$m->setAccessible(true);
+		return $m->invoke($this->roster, ...$args);
+	}
+
+	private function resolve(string $imagename): string
+	{
+		return $this->invoke('resolve_game_image', self::IMAGES, $this->invoke('class_image_candidates', $imagename));
+	}
+
+	public function test_candidates_prefer_roster_art_then_detail_icon_then_unknown(): void
+	{
+		$this->assertSame(
+			[
+				'roster_classes/gw2_thief.png',
+				'class_images/gw2_thief.png',
+				'roster_classes/gw2_unknown.png',
+				'class_images/gw2_unknown.png',
+			],
+			$this->invoke('class_image_candidates', 'gw2_thief')
+		);
+	}
+
+	public function test_candidates_for_an_unprefixed_imagename_skip_the_unknown_pair(): void
+	{
+		$this->assertSame(
+			[
+				'roster_classes/warrior.png',
+				'class_images/warrior.png',
+			],
+			$this->invoke('class_image_candidates', 'warrior')
+		);
+	}
+
+	public function test_candidates_strip_path_traversal_from_imagename(): void
+	{
+		$this->assertSame(
+			[
+				'roster_classes/passwd.png',
+				'class_images/passwd.png',
+			],
+			$this->invoke('class_image_candidates', '../../../etc/passwd')
+		);
+	}
+
+	public function test_candidates_are_empty_for_an_empty_imagename(): void
+	{
+		$this->assertSame([], $this->invoke('class_image_candidates', ''));
+	}
+
+	public function test_uses_roster_art_when_present(): void
+	{
+		$this->assertSame(self::IMAGES . 'roster_classes/custom_warrior.png', $this->resolve('custom_warrior'));
+	}
+
+	public function test_falls_back_to_class_images_when_roster_art_is_missing(): void
+	{
+		$this->assertSame(self::IMAGES . 'class_images/custom_unknown.png', $this->resolve('custom_unknown'));
+	}
+
+	public function test_falls_back_to_the_games_unknown_icon_when_the_class_has_no_art(): void
+	{
+		$this->assertSame(self::IMAGES . 'class_images/custom_unknown.png', $this->resolve('custom_nosuchclass'));
+	}
+
+	public function test_is_empty_when_not_even_an_unknown_icon_exists(): void
+	{
+		$this->assertSame('', $this->resolve('zzz_nosuchclass'));
+	}
+
+	public function test_character_candidates_fall_back_to_the_unknown_icon_in_the_same_dir(): void
+	{
+		$this->assertSame(
+			[
+				'race_images/ffxiv_viera_female.png',
+				'race_images/ffxiv_unknown.png',
+			],
+			$this->invoke('character_image_candidates', 'race_images', 'ffxiv_viera_female.png')
+		);
+	}
+
+	public function test_character_candidates_for_an_unprefixed_file_skip_the_unknown(): void
+	{
+		$this->assertSame(
+			['class_images/warrior.png'],
+			$this->invoke('character_image_candidates', 'class_images', 'warrior.png')
+		);
+	}
+
+	public function test_character_candidates_are_empty_for_an_empty_filename(): void
+	{
+		$this->assertSame([], $this->invoke('character_image_candidates', 'class_images', ''));
+	}
+
+	public function test_character_image_falls_back_to_the_unknown_icon_on_disk(): void
+	{
+		$this->assertSame(
+			self::IMAGES . 'class_images/custom_unknown.png',
+			$this->invoke('resolve_game_image', self::IMAGES, $this->invoke('character_image_candidates', 'class_images', 'custom_nosuchclass.png'))
+		);
+	}
+
+	public function test_is_empty_for_a_path_without_an_ext_segment(): void
+	{
+		$this->assertSame('', $this->invoke('resolve_game_image', '/not/an/extension/path/', ['roster_classes/custom_warrior.png']));
+	}
+}