Feature/xtense architecture phase1#56
Conversation
…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.
|
There was a problem hiding this comment.
Pull request overview
This PR lays the groundwork for a phased “Xtense handler” architecture by adding a PHPUnit-based test harness + fixtures, introducing a shared AbstractHandler base class, and tightening error handling/logging + coordinate validation.
Changes:
- Added Composer + PHPUnit setup (bootstrap + config) and a suite of unit tests with JSON fixtures (many handler tests are TDD placeholders that skip until handlers exist).
- Introduced
includes/AbstractHandler.phpto centralize common handler logic (grant checks, coordinate parsing, upserts, callbacks, IO responses). - Refactored callback error reporting to log via Monolog and updated coordinate validation to throw on invalid format.
Reviewed changes
Copilot reviewed 53 out of 55 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| xtense.php | Wraps main processing in a try/catch and fixes ranking match for empty type3. |
| includes/AbstractHandler.php | New shared base class for future handler refactors. |
| includes/Io.php | Switches callback error reporting from echo to Monolog logging. |
| includes/Check.php | Changes invalid coordinate format handling to throw InvalidArgumentException. |
| composer.json | Adds dev deps (PHPUnit, Monolog) and composer test script. |
| composer.lock | Locks the added dev dependencies. |
| phpunit.xml | PHPUnit configuration pointing at tests/bootstrap.php. |
| tests/bootstrap.php | PHPUnit bootstrap that loads Composer, defines constants/stubs, loads xtense sources + optional handlers. |
| tests/unit/XtenseTestCase.php | Base test case providing globals, stubs, constants, and helper methods. |
| tests/unit/SpyDatabase.php | Capturing DB spy to assert on generated SQL. |
| tests/unit/SpyCallbackHandler.php | Capturing callback handler spy for assertions. |
| tests/unit/AbstractHandlerTest.php | Unit tests for AbstractHandler helpers via a concrete stub. |
| tests/unit/IoTest.php | Unit tests for Io response building. |
| tests/unit/CheckTest.php | Unit tests for Check validation helpers (notably coords()). |
| tests/unit/OverviewHandlerTest.php | Skipped TDD tests for future OverviewHandler. |
| tests/unit/BuildingsHandlerTest.php | Skipped TDD tests for future BuildingsHandler. |
| tests/unit/ResourceSettingsHandlerTest.php | Skipped TDD tests for future ResourceSettingsHandler. |
| tests/unit/ResearchsHandlerTest.php | Skipped TDD tests for future ResearchsHandler. |
| tests/unit/SystemHandlerTest.php | Skipped TDD tests for future SystemHandler. |
| tests/unit/RankingHandlerTest.php | Skipped TDD tests for future RankingHandler. |
| tests/unit/CombatReportHandlerTest.php | Skipped TDD tests for future CombatReportHandler. |
| tests/fixtures/overview.json | Fixture for overview payload tests. |
| tests/fixtures/buildings.json | Fixture for buildings payload tests. |
| tests/fixtures/buildings_facilities.json | Fixture for facilities buildings payload tests. |
| tests/fixtures/resourceSettings.json | Fixture for resource settings payload tests. |
| tests/fixtures/researchs.json | Fixture for research payload tests. |
| tests/fixtures/system.json | Fixture for system payload tests. |
| tests/fixtures/defense.json | Fixture for defense payload tests. |
| tests/fixtures/rc.json | Fixture for basic combat report payload tests. |
| tests/fixtures/rc_complex.json | Fixture for complex combat report payload tests. |
| tests/fixtures/messages_spy.json | Fixture for spy message payload tests. |
| tests/fixtures/messages_spy_shared.json | Placeholder fixture for shared spy message payload. |
| tests/fixtures/messages_rc_cdr.json | Placeholder fixture for recycling report message payload. |
| tests/fixtures/messages_msg.json | Placeholder fixture for generic message payload. |
| tests/fixtures/messages_expedition.json | Placeholder fixture for expedition message payload. |
| tests/fixtures/messages_ennemy_spy.json | Placeholder fixture for enemy spy message payload. |
| tests/fixtures/messages_ally_msg.json | Placeholder fixture for ally message payload. |
| tests/fixtures/fleet.json | Placeholder fixture for fleet payload. |
| tests/fixtures/ally_list.json | Placeholder fixture for ally list payload. |
| tests/fixtures/ranking_player_points.json | Fixture for player points ranking payload tests. |
| tests/fixtures/ranking_player_economy.json | Fixture for player economy ranking payload tests. |
| tests/fixtures/ranking_player_fleet_built.json | Fixture for player fleet-built ranking payload tests. |
| tests/fixtures/ranking_player_fleet_points.json | Fixture for player fleet ranking payload tests. |
| tests/fixtures/ranking_player_fleet_loose.json | Placeholder fixture for player fleet “loose” ranking payload. |
| tests/fixtures/ranking_player_fleet_honor.json | Placeholder fixture for player fleet honor ranking payload. |
| tests/fixtures/ranking_player_fleet_destruct.json | Placeholder fixture for player fleet destruct ranking payload. |
| tests/fixtures/ranking_player_research.json | Placeholder fixture for player research ranking payload. |
| tests/fixtures/ranking_ally_points.json | Placeholder fixture for ally points ranking payload. |
| tests/fixtures/ranking_ally_economy.json | Placeholder fixture for ally economy ranking payload. |
| tests/fixtures/ranking_ally_fleet_built.json | Placeholder fixture for ally fleet-built ranking payload. |
| tests/fixtures/ranking_ally_fleet_loose.json | Placeholder fixture for ally fleet “loose” ranking payload. |
| tests/fixtures/ranking_ally_fleet_honor.json | Placeholder fixture for ally fleet honor ranking payload. |
| tests/fixtures/ranking_ally_fleet_destruct.json | Placeholder fixture for ally fleet destruct ranking payload. |
| tests/fixtures/ranking_ally_research.json | Placeholder fixture for ally research ranking payload. |
| .gitignore | Ignores vendor/ and PHPUnit cache. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 values directly into SQL without escaping and doesn’t handle nulls (null becomes an empty quoted string and can emit notices). This is both a correctness and injection risk since many handler inputs originate from request payloads. Use the DB escape API for string values (and emit SQL NULL for nulls) rather than concatenating raw values.
| } catch (\Throwable $e) { | ||
| $log->error("Xtense error: " . $e->getMessage(), ['exception' => $e]); | ||
| $io->set('error', $e->getMessage()); | ||
| $io->status(0); | ||
| } |
There was a problem hiding this comment.
The top-level catch sends the raw exception message back to the client in the JSON response. Some exception messages (e.g., DB/SQL errors, file paths) can leak internal details. Consider returning a generic error message to the client and keeping full details only in logs.
| public function append_call_error($call, $message, ?Exception $e = null) | ||
| { | ||
| global $log; | ||
|
|
||
| $this->append_call($call, self::ERROR); | ||
| $this->append_call_message($message, self::ERROR, $call); | ||
|
|
||
|
|
||
| echo "* CALL ERROR ({$call['root']}):\n $message\n"; | ||
|
|
||
| if ($e !== null) { | ||
| echo " Exception Stacktrace\n"; | ||
| $stacktrace = str_replace("\n", "\n ", $e->getTraceAsString()); | ||
| if (isset($db_password)) { | ||
| $stacktrace = str_replace($db_password, "*****", $stacktrace); | ||
| } | ||
| echo " " . $stacktrace; | ||
| } | ||
|
|
||
| echo "\n\n"; | ||
|
|
||
| $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.
Previously, append_call_error() echoed a stacktrace while scrubbing $db_password from the output. The new Monolog logging path drops that scrubbing and may log sensitive values contained in exception messages/traces depending on the formatter/handler. Consider sanitizing known secrets (or avoid attaching the exception object) before logging.
| * 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.
Check::coords() docblock says it returns string, but the function can return null for out-of-bounds coordinates (and now throws for invalid format). Update the return annotation/signature docs to reflect the actual behavior (e.g., ?string) so callers/tests are consistent.
| if ($e !== null) { | ||
| $context['exception'] = $e; | ||
| } | ||
| $log->error("Xtense callback error: $message", $context); |
There was a problem hiding this comment.
Io::append_call_error() now unconditionally calls $log->error() via a global, which will fatal if $log is unset or not a logger instance (e.g., some OGSpy contexts or early bootstrap failures). Consider guarding the call (isset/is_object/method_exists) and falling back to error_log() to preserve error reporting.
| $log->error("Xtense callback error: $message", $context); | |
| $logMessage = "Xtense callback error: $message"; | |
| if (isset($log) && is_object($log) && method_exists($log, 'error')) { | |
| $log->error($logMessage, $context); | |
| } else { | |
| $fallbackMessage = $logMessage . ' [mod=' . $context['mod'] . ']'; | |
| if ($e !== null) { | |
| $fallbackMessage .= ' [exception=' . $e->getMessage() . ']'; | |
| } | |
| error_log($fallbackMessage); | |
| } |
| ] as $constName => $constValue) { | ||
| if (!defined($constName)) define($constName, $constValue); | ||
| } | ||
|
|
There was a problem hiding this comment.
tests/bootstrap.php defines only a subset of TABLE_* constants, but XtenseTestCase::setUpBeforeClass() returns early when Io is already loaded, so its additional constants (e.g., TABLE_PARSEDRC / TABLE_PARSEDRCROUND / TABLE_ROUND_ATTACK / TABLE_ROUND_DEFENSE) won’t be defined when those handlers/tests are added. Either add the missing constants here or restructure XtenseTestCase bootstrap so required constants are always defined.
| foreach ([ | |
| 'TABLE_PARSEDRC' => $p . 'parsedrc', | |
| 'TABLE_PARSEDRCROUND' => $p . 'parsedrcround', | |
| 'TABLE_ROUND_ATTACK' => $p . 'round_attack', | |
| 'TABLE_ROUND_DEFENSE' => $p . 'round_defense', | |
| ] as $constName => $constValue) { | |
| if (!defined($constName)) define($constName, $constValue); | |
| } |
| public static function setUpBeforeClass(): void | ||
| { | ||
| parent::setUpBeforeClass(); | ||
|
|
||
| // 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 Io is already loaded, which also skips defining constants and loading support code that later tests/handlers may rely on. If bootstrap.php is the single source of loading now, consider removing this guard or ensuring any required constant definitions happen regardless of whether classes are already loaded.
| "phpunit/phpunit": "^13.0", | ||
| "monolog/monolog": "^3.9" | ||
| }, | ||
| "scripts": { | ||
| "test": "vendor/bin/phpunit" | ||
| }, | ||
| "config": { | ||
| "optimize-autoloader": true |
There was a problem hiding this comment.
composer.json requires phpunit/phpunit ^13, which (per composer.lock) requires PHP >= 8.4.1. If the project/mod is expected to be testable on earlier PHP versions (common for OGSpy deployments), consider constraining the PHP platform version and selecting a PHPUnit version compatible with it to avoid blocking composer install for contributors/CI.
| "phpunit/phpunit": "^13.0", | |
| "monolog/monolog": "^3.9" | |
| }, | |
| "scripts": { | |
| "test": "vendor/bin/phpunit" | |
| }, | |
| "config": { | |
| "optimize-autoloader": true | |
| "phpunit/phpunit": "^10.5", | |
| "monolog/monolog": "^3.9" | |
| }, | |
| "scripts": { | |
| "test": "vendor/bin/phpunit" | |
| }, | |
| "config": { | |
| "optimize-autoloader": true, | |
| "platform": { | |
| "php": "8.1.0" | |
| } |
| public function append_call_error($call, $message, ?Exception $e = null) | ||
| { | ||
| global $log; | ||
|
|
||
| $this->append_call($call, self::ERROR); | ||
| $this->append_call_message($message, self::ERROR, $call); | ||
|
|
||
|
|
||
| echo "* CALL ERROR ({$call['root']}):\n $message\n"; | ||
|
|
||
| if ($e !== null) { | ||
| echo " Exception Stacktrace\n"; | ||
| $stacktrace = str_replace("\n", "\n ", $e->getTraceAsString()); | ||
| if (isset($db_password)) { | ||
| $stacktrace = str_replace($db_password, "*****", $stacktrace); | ||
| } | ||
| echo " " . $stacktrace; | ||
| } | ||
|
|
||
| echo "\n\n"; | ||
|
|
||
| $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() type-hints ?Exception, but callback execution can also fail with non-Exception Throwables (e.g., TypeError) which currently won’t be passable to this method if you want to log them. Using ?\Throwable instead will cover both Exceptions and Errors.
| 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 (and now throws only on format). If null is returned, explode()/list() will trigger warnings and the handler will proceed with invalid coords. Consider making parseCoordinates explicitly handle a null return (e.g., throw an exception / set IO error) before exploding.
This pull request introduces a comprehensive setup for automated testing and improves error handling and code structure in the Xtense mod. The main changes include adding PHPUnit configuration and fixtures, introducing an abstract handler base class to reduce code duplication, refactoring error handling to use Monolog, and improving coordinate validation.
Testing infrastructure and fixtures:
composer.jsonwith development dependencies (phpunit/phpunit,monolog/monolog) and a test script for Composer-based workflows.phpunit.xml), a bootstrap file (tests/bootstrap.php), and multiple JSON fixtures for handler and message types to support unit testing. [1] [2] [3] [4] [5] [6] [7]Core architecture improvements:
includes/AbstractHandler.php, a new abstract base class for all Xtense data type handlers, providing shared methods for grant checking, coordinate parsing, planet type resolution, upsert queries, callback registration, logging, and IO response handling. This reduces code duplication and centralizes common logic.Error handling and logging:
includes/Io.phpto use Monolog for logging callback errors, including exception context, instead of directechostatements.Validation and robustness:
Check::coordsmethod inincludes/Check.phpto throw anInvalidArgumentExceptionon invalid coordinates instead of terminating the process, making error handling more robust and testable.