Version 2.12.0#58
Conversation
Update actions/checkout action to v6
…l error handler - Check::coords() now throws InvalidArgumentException instead of die() - Added try/catch around main switch+callbacks in xtense.php - Exceptions now produce proper JSON error response with logging - Previously, unhandled UnexpectedValueException would also crash raw
- Removed direct echo of error messages and stack traces to stdout - Callback errors now logged via global \ (Monolog) with context - Prevents raw text from corrupting JSON response to browser extension - Also removes potential password leak (was echoing stacktrace without sanitization)
Provides shared methods to eliminate code duplication across the 18 case handlers: - requireGrant(): grant check with IO error response (repeated 18x) - parseCoordinates(): Check::coords() + explode + casting (repeated 12x) - resolvePlanetType(): TYPE_PLANET/TYPE_MOON ternary (repeated 15x) - planetTypeToString(): planet type to DB string - executeUpsert(): INSERT...ON DUPLICATE KEY UPDATE builder (repeated 8x) - registerCallback(): wraps CallbackHandler::add() - logAction(): wraps add_log() with auto toolbar info - setPageUpdatedResponse(): standard IO response pattern No behavior changes - this is a foundation class for Phase 2 handler extraction.
Feature/xtense architecture phase1
|
There was a problem hiding this comment.
Pull request overview
This PR bumps Xtense to 2.12.0, adds a PHPUnit-based unit testing harness (bootstrap, spies, fixtures, and initial tests), and refactors shared handler logic/error handling to improve robustness and testability.
Changes:
- Add Composer + PHPUnit configuration and a standalone test bootstrap with fixtures/spies for isolated unit testing.
- Introduce
includes/AbstractHandler.phpas a shared base for handler implementations, consolidating common helpers. - Improve runtime error handling (entrypoint try/catch, callback error logging via Monolog) and adjust coordinate validation behavior.
Reviewed changes
Copilot reviewed 55 out of 57 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
xtense.php |
Wrap processing in try/catch and fix ranking match default routing for empty type3. |
version.txt |
Bump mod version to 2.12.0. |
includes/AbstractHandler.php |
Add shared handler base class with grant checks, coordinate parsing, upsert helper, callbacks, and logging helpers. |
includes/Io.php |
Switch callback error reporting from echo to $log->error() with exception context. |
includes/Check.php |
Change invalid coordinate format handling from die() to throwing InvalidArgumentException; update docblock. |
composer.json |
Add dev dependencies (PHPUnit, Monolog) and a composer test script. |
composer.lock |
Lock PHPUnit/Monolog and transitive dev dependencies. |
phpunit.xml |
Define the PHPUnit test suite and bootstrap. |
tests/bootstrap.php |
PHPUnit bootstrap: autoloading, guard constants, OGSpy stubs, TABLE_* constants, and handler conditional loading. |
tests/unit/XtenseTestCase.php |
Base test case with globals, spies, fixture helpers, and handler factory helpers. |
tests/unit/SpyDatabase.php |
Spy DB capturing SQL queries for assertions. |
tests/unit/SpyCallbackHandler.php |
Spy callback handler capturing callback registrations. |
tests/unit/AbstractHandlerTest.php |
Unit tests for AbstractHandler helper methods via a concrete test handler. |
tests/unit/IoTest.php |
Unit tests for IO response building. |
tests/unit/CheckTest.php |
Unit tests for Check validation helpers. |
tests/unit/OverviewHandlerTest.php |
TDD tests scaffold for OverviewHandler (skipped until implemented). |
tests/unit/BuildingsHandlerTest.php |
TDD tests scaffold for BuildingsHandler (skipped until implemented). |
tests/unit/ResourceSettingsHandlerTest.php |
TDD tests scaffold for ResourceSettingsHandler (skipped until implemented). |
tests/unit/ResearchsHandlerTest.php |
TDD tests scaffold for ResearchsHandler (skipped until implemented). |
tests/unit/SystemHandlerTest.php |
TDD tests scaffold for SystemHandler (skipped until implemented). |
tests/unit/RankingHandlerTest.php |
TDD tests scaffold for RankingHandler (skipped until implemented). |
tests/unit/CombatReportHandlerTest.php |
TDD tests scaffold for CombatReportHandler (skipped until implemented). |
tests/fixtures/overview.json |
Add overview payload fixture. |
tests/fixtures/buildings.json |
Add buildings payload fixture. |
tests/fixtures/buildings_facilities.json |
Add facilities buildings payload fixture. |
tests/fixtures/resourceSettings.json |
Add resource settings payload fixture. |
tests/fixtures/researchs.json |
Add research payload fixture. |
tests/fixtures/defense.json |
Add defense payload fixture. |
tests/fixtures/system.json |
Add galaxy/system payload fixture. |
tests/fixtures/messages_spy.json |
Add spy message payload fixture. |
tests/fixtures/rc.json |
Add basic combat report payload fixture. |
tests/fixtures/rc_complex.json |
Add complex combat report payload fixture. |
tests/fixtures/ranking_player_points.json |
Add player points ranking payload fixture. |
tests/fixtures/ranking_player_economy.json |
Add player economy ranking payload fixture. |
tests/fixtures/ranking_player_fleet_built.json |
Add player fleet-built ranking payload fixture. |
tests/fixtures/ranking_player_fleet_points.json |
Add player fleet-points ranking payload fixture. |
tests/fixtures/ranking_player_research.json |
Placeholder ranking fixture (empty JSON object). |
tests/fixtures/ranking_player_fleet_loose.json |
Placeholder ranking fixture (empty JSON object). |
tests/fixtures/ranking_player_fleet_honor.json |
Placeholder ranking fixture (empty JSON object). |
tests/fixtures/ranking_player_fleet_destruct.json |
Placeholder ranking fixture (empty JSON object). |
tests/fixtures/ranking_ally_points.json |
Placeholder ranking fixture (empty JSON object). |
tests/fixtures/ranking_ally_economy.json |
Placeholder ranking fixture (empty JSON object). |
tests/fixtures/ranking_ally_research.json |
Placeholder ranking fixture (empty JSON object). |
tests/fixtures/ranking_ally_fleet_built.json |
Placeholder ranking fixture (empty JSON object). |
tests/fixtures/ranking_ally_fleet_loose.json |
Placeholder ranking fixture (empty JSON object). |
tests/fixtures/ranking_ally_fleet_honor.json |
Placeholder ranking fixture (empty JSON object). |
tests/fixtures/ranking_ally_fleet_destruct.json |
Placeholder ranking fixture (empty JSON object). |
tests/fixtures/messages_spy_shared.json |
Placeholder messages fixture (empty JSON object). |
tests/fixtures/messages_rc_cdr.json |
Placeholder messages fixture (empty JSON object). |
tests/fixtures/messages_msg.json |
Placeholder messages fixture (empty JSON object). |
tests/fixtures/messages_expedition.json |
Placeholder messages fixture (empty JSON object). |
tests/fixtures/messages_ennemy_spy.json |
Placeholder messages fixture (empty JSON object). |
tests/fixtures/messages_ally_msg.json |
Placeholder messages fixture (empty JSON object). |
tests/fixtures/fleet.json |
Placeholder fleet fixture (empty JSON object). |
tests/fixtures/ally_list.json |
Placeholder ally list fixture (empty JSON object). |
.gitignore |
Ignore vendor/ and PHPUnit cache file. |
.github/workflows/sonarcloud.yml |
Update actions/checkout version in SonarCloud workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "type": "library", | ||
| "license": "GPL-3.0-or-later", | ||
| "require-dev": { | ||
| "phpunit/phpunit": "^13.0", |
There was a problem hiding this comment.
composer.json requires phpunit/phpunit ^13, and the locked version requires PHP >= 8.4.1. If the mod (or contributors/CI) need to run tests on earlier PHP versions, this dependency choice will prevent composer install --dev. Consider pinning PHPUnit to a version compatible with your supported PHP (or setting config.platform.php / adding an explicit php requirement to document the minimum).
| "phpunit/phpunit": "^13.0", | |
| "phpunit/phpunit": "^10.5", |
| protected function parseCoordinates(string $coordString, int $exp = 0): array | ||
| { | ||
| $coords = Check::coords($coordString, $exp); | ||
| list($g, $s, $r) = explode(':', $coords); | ||
| return [ | ||
| 'coords' => $coords, | ||
| 'galaxy' => (int)$g, | ||
| 'system' => (int)$s, | ||
| 'row' => (int)$r, | ||
| ]; |
There was a problem hiding this comment.
AbstractHandler::parseCoordinates() assumes Check::coords() always returns a string, but Check::coords() returns null for out-of-bounds coordinates. That means explode(':', $coords) will trigger a TypeError instead of a controlled validation failure. Consider handling a null return (or updating Check::coords to throw) and raising a clear exception when coordinates are out of range.
| protected function executeUpsert(string $table, array $columns, array $values, array $updateColumns): void | ||
| { | ||
| $colStr = '`' . implode('`, `', $columns) . '`'; | ||
| $valStr = implode(', ', array_map(function ($val) { | ||
| return is_int($val) || is_float($val) ? $val : "'" . $val . "'"; | ||
| }, $values)); | ||
| $updatePairs = array_map(function ($col) { | ||
| return "`$col` = VALUES(`$col`)"; | ||
| }, $updateColumns); | ||
|
|
||
| $query = "INSERT INTO $table ($colStr) VALUES ($valStr) ON DUPLICATE KEY UPDATE " . implode(', ', $updatePairs); | ||
| $this->db->sql_query($query); |
There was a problem hiding this comment.
executeUpsert() interpolates unescaped string values directly into SQL (it only wraps them in quotes). This is vulnerable to SQL injection/broken queries when values contain quotes, and it diverges from the rest of the codebase which uses $db->sql_escape_string(). Please escape string values via the DB layer (and handle null/boolean values explicitly) before building the query.
| $context = ['mod' => $call['root'] ?? 'unknown']; | ||
| if ($e !== null) { | ||
| $context['exception'] = $e; | ||
| } | ||
| $log->error("Xtense callback error: $message", $context); |
There was a problem hiding this comment.
append_call_error() now unconditionally calls $log->error(), but $log is pulled from global scope and may be undefined/null in some execution paths (or non-Monolog in older OGSpy installs). To avoid turning a callback failure into a fatal error, guard the logger call (e.g., check is_object/has method) or fall back to error_log when $log is unavailable.
| $context = ['mod' => $call['root'] ?? 'unknown']; | |
| if ($e !== null) { | |
| $context['exception'] = $e; | |
| } | |
| $log->error("Xtense callback error: $message", $context); | |
| $context = array('mod' => $call['root'] ?? 'unknown'); | |
| if ($e !== null) { | |
| $context['exception'] = $e; | |
| } | |
| if (is_object($log) && method_exists($log, 'error')) { | |
| $log->error("Xtense callback error: $message", $context); | |
| } else { | |
| $fallbackMessage = "Xtense callback error: $message (mod: " . $context['mod'] . ")"; | |
| if ($e !== null) { | |
| $fallbackMessage .= ' Exception: ' . $e->getMessage(); | |
| } | |
| error_log($fallbackMessage); | |
| } |
| * Sanitize Coordinates string | ||
| * @param $string | ||
| * @param int $exp | ||
| * @return mixed | ||
| * @return string | ||
| * @throws \InvalidArgumentException | ||
| */ | ||
| static function coords($string, $exp = 0) { | ||
| global $server_config; | ||
| //if ($string == "unknown") return true; //cas avec une seule planète | ||
| if (!preg_match('!^([0-9]{1,2}):([0-9]{1,3}):([0-9]{1,2})$!Usi', $string, $match)) die("coords : Hack"); | ||
| if (!preg_match('!^([0-9]{1,2}):([0-9]{1,3}):([0-9]{1,2})$!Usi', $string, $match)) { | ||
| throw new \InvalidArgumentException("Invalid coordinates format: " . substr((string)$string, 0, 30)); | ||
| } | ||
| if (($match[1] < 1 || $match[2] < 1 || $match[3] < 1 || $match[1] > $server_config['num_of_galaxies'] || $match[2] > $server_config['num_of_systems'] || ($exp ? ($match[3] != 16) : ($match[3] > 15))) == false) return $string; | ||
| } |
There was a problem hiding this comment.
The coords() docblock says it returns string, but the function still returns null implicitly when coordinates are out of bounds. This mismatch makes it easy for callers to assume a string and then hit runtime errors. Please update the docblock to reflect the nullable return (and/or consider throwing for out-of-range coordinates to make validation behavior consistent).
| 'TABLE_RANK_ALLY_MILITARY_BUILT' => $p . 'game_rank_ally_military_built', | ||
| 'TABLE_RANK_ALLY_MILITARY_LOOSE' => $p . 'game_rank_ally_military_loose', | ||
| 'TABLE_RANK_ALLY_MILITARY_DESTRUCT' => $p . 'game_rank_ally_military_destruct', | ||
| 'TABLE_RANK_ALLY_HONOR' => $p . 'game_rank_ally_honor', |
There was a problem hiding this comment.
tests/bootstrap.php defines many TABLE_* constants, but it omits TABLE_PARSEDRC / TABLE_PARSEDRCROUND / TABLE_ROUND_ATTACK / TABLE_ROUND_DEFENSE which xtense.php uses for combat reports. Since XtenseTestCase::setUpBeforeClass() currently early-returns when Io is already loaded, those constants won’t be defined elsewhere. Consider adding the missing constants here (or removing the duplication and centralizing constant definition in one place).
| 'TABLE_RANK_ALLY_HONOR' => $p . 'game_rank_ally_honor', | |
| 'TABLE_RANK_ALLY_HONOR' => $p . 'game_rank_ally_honor', | |
| 'TABLE_PARSEDRC' => $p . 'parsedrc', | |
| 'TABLE_PARSEDRCROUND' => $p . 'parsedrcround', | |
| 'TABLE_ROUND_ATTACK' => $p . 'round_attack', | |
| 'TABLE_ROUND_DEFENSE' => $p . 'round_defense', |
| // Guard: files only need to be loaded once across all test classes. | ||
| if (class_exists('Io', false)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
XtenseTestCase::setUpBeforeClass() returns early when class Io already exists. Because tests/bootstrap.php loads Io up front, everything below this guard (OGSpy stubs, TABLE_* constants, requires) becomes dead code and won’t run. Either remove this guard, move it below any definitions you still need here, or delete the duplicate bootstrap logic and rely solely on tests/bootstrap.php.
Version 2.12.0
This pull request introduces a comprehensive testing infrastructure for the Xtense module, including PHPUnit configuration, fixtures, and a bootstrap file to enable isolated unit testing. It also refactors and improves core handler and utility classes to support better error handling, code reuse, and testability.
Testing infrastructure and configuration:
composer.jsonwith development dependencies (phpunit/phpunit,monolog/monolog) and test scripts for running PHPUnit.phpunit.xmlto define the test suite and configure PHPUnit for the Xtense module.tests/bootstrap.phpto set up autoloading, define constants, stub OGSpy functions, and load source and test-support files, enabling standalone and integrated testing.tests/fixtures/for various handler types (e.g.,buildings.json,defense.json,messages_spy.json) to support unit tests. [1] [2] [3] [4] [5]Core handler and utility improvements:
includes/AbstractHandler.php, providing a base class for all Xtense data handlers with shared helper methods for grant checking, coordinate parsing, planet type resolution, upsert queries, callback registration, and logging. This reduces code duplication and standardizes handler behavior.includes/Io.phpby logging callback errors via Monolog instead of echoing to the console, and including exception details in the log context.includes/Check.phpto throw an exception on invalid input instead of terminating execution, improving robustness and testability.CI/CD and dependency updates:
.github/workflows/sonarcloud.ymlto useactions/checkout@v6.0.2for better security and compatibility.