diff --git a/portal/modules/roster.php b/portal/modules/roster.php index 186cd950..f9a15972 100644 --- a/portal/modules/roster.php +++ b/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 _unknown icon is better than + * nothing at all. Plugins name every asset _, 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 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 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 $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(); diff --git a/styles/all/template/portal/modules/roster_center.html b/styles/all/template/portal/modules/roster_center.html index f6075d15..957794f4 100644 --- a/styles/all/template/portal/modules/roster_center.html +++ b/styles/all/template/portal/modules/roster_center.html @@ -103,9 +103,9 @@ {% if S_SHOWACH == 1 %} {{ row.ACHIEVPTS }} {% endif %} -  {{ row.RACE }} + {% if row.RACE_IMAGE %} {% endif %}{{ row.RACE }} - {{ row.CLASS }}  + {% if row.CLASS_IMAGE %}{{ row.CLASS }} {% endif %} {{ row.CLASS }} {% if S_SHOWSPEC %} @@ -137,8 +137,8 @@ {# ---- Grid view (grouped by class) ---- #} {% for class in loops.class %}

- {{ class.CLASSNAME }} -   + {% if class.CLASSIMG %}{{ class.CLASSNAME }} +  {% endif %} {{ class.CLASSNAME }}

@@ -160,7 +160,7 @@

{% endif %} - {% if players_row.PORTRAIT %} {% else %}{{ players_row.CLASS }}{% endif %} + {% if players_row.PORTRAIT %} {% elseif players_row.CLASS_IMAGE %}{{ players_row.CLASS }}{% endif %} {{ players_row.NAME }} diff --git a/tests/portal/roster_image_fallback_test.php b/tests/portal/roster_image_fallback_test.php new file mode 100644 index 00000000..392376ff --- /dev/null +++ b/tests/portal/roster_image_fallback_test.php @@ -0,0 +1,160 @@ +.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 _unknown exists → fall back to the game's unknown + * - nothing resolves → '' (template omits the ) + * + * 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'])); + } +}