From 5c5d1d914481c60ab1c2299aa659dd627a32668a Mon Sep 17 00:00:00 2001 From: Dan Hemberger Date: Fri, 18 Nov 2022 00:50:37 -0800 Subject: [PATCH 1/3] Session: modify how pages are handled internally The purpose of this is to make the management of valid page links more straightforward and to better separate the concepts of page links and the current page. It should also fix the issue of the number of links in the session growing linearly with the number of pages loaded during the session (in some common cases, such as refreshing Current Sector). There are still some edge cases that may not work as intended yet. For example, if we click an ajax link (e.g. updating sector connections) and that SN uses the same page data as the current page, then it will be incorrectly identified by the Session as an ajax auto-refresh, and won't actually load the desired page. In the future, we may want to change the query parameters so that we can explicitly identify when an auto-refresh request is made. Three main aspects of this change: 1. Replace CommonID hashing with direct page equality comparison. We use loose object equality comparison with the page being added. This should be plenty fast, because in most cases any two pages will share no common properties and will return early from the equality check. It also avoids the need to constantly recompute the common ID of each page, which may even be more expensive (it has to serialize and hash every Page object and thus has no early return mechanism). 2. Save the current page separately from the stored page links. (This allows us to reuse it if we detect an auto-refresh.) Since we won't modify links associated with an SN when the current page is modified (e.g. by adding a request variable to the page via Session::getRequestVar), we ensure that when we try to add reusable pages that were already added, they won't get a new SN. It also fixes a memory leak of sorts, because clicking on certain reusable pages would *always* create a new SN and so the session var would just get larger and larger (`current_sector.php` was the most prominent page affected by this). 3. Convert remaining page loads from a dynamic int to a constant bool. (We were basically using the int as a bool, and having it be a constant allows us not to care when a page was created.) Left panel links should all be reusable, so include them in the "always available" list. The following concepts were tested manually for proper behavior: * Assigning GP Editor * Plot to Nearest * Custom display range in Rankings * Making multiple chess moves * Editing sector links * Toggling ship weapon display --- src/lib/Default/Page.php | 48 +++------ src/lib/Smr/Session.php | 102 ++++++++---------- .../lib/DefaultGame/PageIntegrationTest.php | 29 ----- .../DefaultGame/SessionIntegrationTest.php | 47 +++++++- 4 files changed, 103 insertions(+), 123 deletions(-) diff --git a/src/lib/Default/Page.php b/src/lib/Default/Page.php index f366d801f..a64a97da3 100644 --- a/src/lib/Default/Page.php +++ b/src/lib/Default/Page.php @@ -11,12 +11,12 @@ */ class Page extends ArrayObject { - private const ALWAYS_AVAILABLE = 999999; + private const ALWAYS_AVAILABLE = true; - // Defines the number of pages that can be loaded after - // this page before the links on this page become invalid - // (i.e. before you get a back button error). - private const URL_DEFAULT_REMAINING_PAGE_LOADS = [ + // Defines if the page is is always available, or if it is invalid after one + // use (i.e. if you get a back button error when navigating back to it). + public const ALWAYS_AVAILABLE_PAGES = [ + 'album_edit.php' => self::ALWAYS_AVAILABLE, 'alliance_broadcast.php' => self::ALWAYS_AVAILABLE, 'alliance_forces.php' => self::ALWAYS_AVAILABLE, 'alliance_list.php' => self::ALWAYS_AVAILABLE, @@ -33,7 +33,7 @@ class Page extends ArrayObject { 'course_plot.php' => self::ALWAYS_AVAILABLE, 'changelog_view.php' => self::ALWAYS_AVAILABLE, 'chat_rules.php' => self::ALWAYS_AVAILABLE, - 'chess_play.php' => self::ALWAYS_AVAILABLE, + 'chess.php' => self::ALWAYS_AVAILABLE, 'combat_log_list.php' => self::ALWAYS_AVAILABLE, 'combat_log_viewer.php' => self::ALWAYS_AVAILABLE, 'current_sector.php' => self::ALWAYS_AVAILABLE, @@ -50,11 +50,13 @@ class Page extends ArrayObject { 'feature_request.php' => self::ALWAYS_AVAILABLE, 'forces_list.php' => self::ALWAYS_AVAILABLE, 'forces_mass_refresh.php' => self::ALWAYS_AVAILABLE, - 'hall_of_fame_player_new.php' => self::ALWAYS_AVAILABLE, + 'galactic_post_current.php' => self::ALWAYS_AVAILABLE, + 'hall_of_fame_new.php' => self::ALWAYS_AVAILABLE, 'hall_of_fame_player_detail.php' => self::ALWAYS_AVAILABLE, 'leave_newbie.php' => self::ALWAYS_AVAILABLE, 'logoff.php' => self::ALWAYS_AVAILABLE, 'map_local.php' => self::ALWAYS_AVAILABLE, + 'message_box.php' => self::ALWAYS_AVAILABLE, 'message_view.php' => self::ALWAYS_AVAILABLE, 'message_send.php' => self::ALWAYS_AVAILABLE, 'news_read_advanced.php' => self::ALWAYS_AVAILABLE, @@ -82,6 +84,7 @@ class Page extends ArrayObject { 'rankings_race.php' => self::ALWAYS_AVAILABLE, 'rankings_sector_kill.php' => self::ALWAYS_AVAILABLE, 'rankings_view.php' => self::ALWAYS_AVAILABLE, + 'smr_file_create.php' => self::ALWAYS_AVAILABLE, 'trader_bounties.php' => self::ALWAYS_AVAILABLE, 'trader_relations.php' => self::ALWAYS_AVAILABLE, 'trader_savings.php' => self::ALWAYS_AVAILABLE, @@ -93,7 +96,7 @@ class Page extends ArrayObject { 'alliance_message_add_processing.php' => self::ALWAYS_AVAILABLE, 'alliance_message_delete_processing.php' => self::ALWAYS_AVAILABLE, 'alliance_pick_processing.php' => self::ALWAYS_AVAILABLE, - 'chess_move_processing.php' => self::ALWAYS_AVAILABLE, + 'game_leave_processing.php' => self::ALWAYS_AVAILABLE, 'toggle_processing.php' => self::ALWAYS_AVAILABLE, //Admin pages 'admin/account_edit.php' => self::ALWAYS_AVAILABLE, @@ -116,7 +119,7 @@ class Page extends ArrayObject { 'admin/unigen/universe_create_warps.php' => self::ALWAYS_AVAILABLE, ]; - public int $remainingPageLoads; + public readonly bool $reusable; /** * @param array $data @@ -127,7 +130,9 @@ protected function __construct( public readonly bool $skipRedirect // to skip redirect hooks at beginning of page processing ) { parent::__construct($data); - $this->remainingPageLoads = self::URL_DEFAULT_REMAINING_PAGE_LOADS[$file] ?? 1; // Allow refreshing + + // Pages are single-use unless explicitly whitelisted + $this->reusable = self::ALWAYS_AVAILABLE_PAGES[$file] ?? false; } /** @@ -188,11 +193,8 @@ public function addVar(string $source, string $dest = null): void { * the next request, we can grab the container out of the Smr\Session. */ public function href(bool $forceFullURL = false): string { - - // We need to make a clone because the instance saved in the Session - // must not be modified if we re-use this instance to create more - // links. Ideally this would not be necessary, but the usage of this - // method would need to change globally first (no Page re-use). + // We need to clone this instance in case it is modified after being added + // to the session links. This would not be necessary if Page was readonly. $sn = Smr\Session::getInstance()->addLink(self::copy($this)); $href = '?sn=' . $sn; @@ -202,22 +204,6 @@ public function href(bool $forceFullURL = false): string { return $href; } - /** - * Returns a hash of the contents of the container to identify when two - * containers are equivalent (apart from page-load tracking metadata, which - * we strip out to prevent false differences). - * - * CommonID MUST be unique to a specific action. If there will be different - * outcomes from containers given the same ID then problems will arise. - */ - public function getCommonID(): string { - $commonContainer = $this->getArrayCopy(); - $commonContainer['_FILE'] = $this->file; // must include file in ID - // NOTE: This ID will change if the order of elements in the container - // changes. If this causes unnecessary SN changes, sort the container! - return md5(serialize($commonContainer)); - } - /** * Process this page by executing the associated file. */ diff --git a/src/lib/Smr/Session.php b/src/lib/Smr/Session.php index cc0580044..4be4f51bc 100644 --- a/src/lib/Smr/Session.php +++ b/src/lib/Smr/Session.php @@ -30,9 +30,8 @@ class Session { private string $sessionID; private int $gameID; /** @var array */ - private array $var; - /** @var array */ - private array $commonIDs = []; + private array $links = []; + private ?Page $currentPage = null; private bool $generate; private string $SN; private string $lastSN; @@ -86,9 +85,9 @@ public function __construct() { $this->SN = Request::get('sn', ''); $this->fetchVarInfo(); - if (!USING_AJAX && !empty($this->SN) && !empty($this->var[$this->SN])) { - $var = $this->var[$this->SN]; - $loadDelay = self::URL_LOAD_DELAY[$var->file] ?? 0; + if (!USING_AJAX && $this->hasCurrentVar()) { + $file = $this->getCurrentVar()->file; + $loadDelay = self::URL_LOAD_DELAY[$file] ?? 0; $timeBetweenLoads = microtime(true) - $this->lastAccessed; if ($timeBetweenLoads < $loadDelay) { $sleepTime = IRound(($loadDelay - $timeBetweenLoads) * 1000000); @@ -97,7 +96,7 @@ public function __construct() { } if (ENABLE_DEBUG) { $this->db->insert('debug', [ - 'debug_type' => $this->db->escapeString('Delay: ' . $var->file), + 'debug_type' => $this->db->escapeString('Delay: ' . $file), 'account_id' => $this->db->escapeNumber($this->accountID), 'value' => $this->db->escapeNumber($timeBetweenLoads), ]); @@ -118,24 +117,22 @@ public function fetchVarInfo(): void { // We may not have ajax_returns if ajax was disabled $this->previousAjaxReturns = $dbRecord->getObject('ajax_returns', true, true); - $this->var = $dbRecord->getObject('session_var', true); - - foreach ($this->var as $sn => $var) { - if ($var->remainingPageLoads < 0) { - //This link is no longer valid - unset($this->var[$sn]); - } else { - // The following is skipped for the current SN, because: - // a) If we decremented RemainingPageLoads, we wouldn't be - // able to refresh the current page. - // b) If we register its CommonID and then subsequently - // modify its data (which is quite common for the - // "current var"), the CommonID is not updated. Then any - // var with the same data as the original will wrongly - // share its CommonID. - if ($sn !== $this->SN) { - $var->remainingPageLoads -= 1; - $this->commonIDs[$var->getCommonID()] = $sn; + [$this->links, $lastPage] = $dbRecord->getObject('session_var', true); + + $ajaxRefresh = USING_AJAX && !$this->hasChangedSN(); + if ($ajaxRefresh) { + $this->currentPage = $lastPage; + } elseif (isset($this->links[$this->SN])) { + // If the current page is modified during page processing, we need + // to make sure the original link is unchanged. So we clone it here. + $this->currentPage = clone $this->links[$this->SN]; + } + + if (!$ajaxRefresh) { // since form pages don't ajax refresh properly + foreach ($this->links as $sn => $link) { + if (!$link->reusable) { + // This link is no longer valid + unset($this->links[$sn]); } } } @@ -143,19 +140,13 @@ public function fetchVarInfo(): void { $this->generate = true; $this->accountID = 0; $this->gameID = 0; - $this->var = []; } } public function update(): void { - foreach ($this->var as $sn => $var) { - if ($var->remainingPageLoads <= 0) { - //This link was valid this load but will not be in the future, removing it now saves database space and data transfer. - unset($this->var[$sn]); - } - } + $sessionVar = [$this->links, $this->currentPage]; if (!$this->generate) { - $this->db->write('UPDATE active_session SET account_id=' . $this->db->escapeNumber($this->accountID) . ',game_id=' . $this->db->escapeNumber($this->gameID) . (!USING_AJAX ? ',last_accessed=' . $this->db->escapeNumber(Epoch::microtime()) : '') . ',session_var=' . $this->db->escapeObject($this->var, true) . + $this->db->write('UPDATE active_session SET account_id=' . $this->db->escapeNumber($this->accountID) . ',game_id=' . $this->db->escapeNumber($this->gameID) . (!USING_AJAX ? ',last_accessed=' . $this->db->escapeNumber(Epoch::microtime()) : '') . ',session_var=' . $this->db->escapeObject($sessionVar, true) . ',last_sn=' . $this->db->escapeString($this->SN) . ' WHERE session_id=' . $this->db->escapeString($this->sessionID) . (USING_AJAX ? ' AND last_sn=' . $this->db->escapeString($this->lastSN) : '')); } else { @@ -165,7 +156,7 @@ public function update(): void { 'account_id' => $this->db->escapeNumber($this->accountID), 'game_id' => $this->db->escapeNumber($this->gameID), 'last_accessed' => $this->db->escapeNumber(Epoch::microtime()), - 'session_var' => $this->db->escapeObject($this->var, true), + 'session_var' => $this->db->escapeObject($sessionVar, true), ]); $this->generate = false; } @@ -257,14 +248,14 @@ public function getLastAccessed(): float { * Check if the session has a var associated with the current SN. */ public function hasCurrentVar(): bool { - return isset($this->var[$this->SN]); + return $this->currentPage !== null; } /** * Returns the session var associated with the current SN. */ public function getCurrentVar(): Page { - return $this->var[$this->SN]; + return $this->currentPage; } /** @@ -302,36 +293,29 @@ public function getRequestVarIntArray(string $varName, array $default = null): a * Replace the global $var with the given $container. */ public function setCurrentVar(Page $container): void { - //Do not allow sharing SN, useful for forwarding. - if ($this->hasCurrentVar()) { - $var = $this->getCurrentVar(); - unset($this->commonIDs[$var->getCommonID()]); //Do not store common id for reset page, to allow refreshing to always give the same page in response - } - - $this->var[$this->SN] = $container; + $this->currentPage = $container; } public function clearLinks(): void { - $this->var = [$this->SN => $this->var[$this->SN]]; - $this->commonIDs = []; + $this->links = []; } + /** + * Add a page to the session so that it can be used on next page load. + * It will be associated with an SN that will be used for linking. + */ public function addLink(Page $container): string { - $sn = $this->generateSN($container); - $this->var[$sn] = $container; - return $sn; - } - - protected function generateSN(Page $container): string { - $commonID = $container->getCommonID(); - if (isset($this->commonIDs[$commonID])) { - $sn = $this->commonIDs[$commonID]; - } else { - do { - $sn = random_alphabetic_string(6); - } while (isset($this->var[$sn])); - $this->commonIDs[$commonID] = $sn; + // If we already had a link to this exact page, use the existing SN for it. + foreach ($this->links as $sn => $link) { + if ($container == $link) { // loose equality to compare contents + return $sn; + } } + // This page isn't an existing link, so give it a new SN. + do { + $sn = random_alphabetic_string(6); + } while (isset($this->links[$sn])); + $this->links[$sn] = $container; return $sn; } diff --git a/test/SmrTest/lib/DefaultGame/PageIntegrationTest.php b/test/SmrTest/lib/DefaultGame/PageIntegrationTest.php index 0de8d71c2..420bf007d 100644 --- a/test/SmrTest/lib/DefaultGame/PageIntegrationTest.php +++ b/test/SmrTest/lib/DefaultGame/PageIntegrationTest.php @@ -75,16 +75,6 @@ public function test_copy(): void { self::assertEquals($page, $copy); } - public function test_remainingPageLoads(): void { - // Create a Page that does not use the default page loads - $page = Page::create('current_sector.php'); - self::assertSame(999999, $page->remainingPageLoads); - - // Create a Page that does use the default page loads - $page = Page::create('test'); - self::assertSame(1, $page->remainingPageLoads); - } - public function test_href(): void { // Create an arbitrary Page $page = Page::create('file'); @@ -142,23 +132,4 @@ public function test_skipRedirect(): void { self::assertFalse($page2->skipRedirect); } - public function test_getCommonID(): void { - // Create an arbitrary Page with data - $data = ['some' => 'data']; - $page1 = Page::create('test1', $data); - // Check the value of the common ID - $expected = 'b5cce3676aa819381ad18dc1a5f41710'; - self::assertSame($expected, $page1->getCommonID()); - - // Create a page with the same data but a different file - $page2 = Page::create('test2', $page1); - // Check that the common ID changes - self::assertNotEquals($page1->getCommonID(), $page2->getCommonID()); - - // Create a page with the same file but different data - $page3 = Page::create('test1', ['different' => 'data']); - // Check that the common ID changes - self::assertNotEquals($page1->getCommonID(), $page3->getCommonID()); - } - } diff --git a/test/SmrTest/lib/DefaultGame/SessionIntegrationTest.php b/test/SmrTest/lib/DefaultGame/SessionIntegrationTest.php index a19e6d19f..997306053 100644 --- a/test/SmrTest/lib/DefaultGame/SessionIntegrationTest.php +++ b/test/SmrTest/lib/DefaultGame/SessionIntegrationTest.php @@ -112,10 +112,6 @@ public function test_current_var(): void { $var = $session->getCurrentVar(); self::assertSame('some_page', $var->file); - // The RemainingPageLoads should still be 1 because we effectively - // reloaded the page by creating a new Session. - self::assertSame(1, $var->remainingPageLoads); - // We can now change the current var $page2 = Page::create('another_page'); $session->setCurrentVar($page2); @@ -125,6 +121,13 @@ public function test_current_var(): void { $var2 = $session->getCurrentVar(); self::assertSame('another_page', $var2->file); + // If we make a new session, but keep the same SN (e.g. ajax), + // we should still get the updated var, even though it wasn't + // the one originally associated with this SN. + $session->update(); + $session = DiContainer::make(Session::class); + self::assertEquals($var2, $session->getCurrentVar()); + // If we destroy the Session, then the current var should no longer // be accessible to a new Session. $session->destroy(); @@ -132,6 +135,42 @@ public function test_current_var(): void { self::assertFalse($session->hasCurrentVar()); } + public function test_addLink(): void { + // If we add two different pages, we should get different SNs. + $page = Page::create('some_page'); + $sn = $this->session->addLink($page); + + $page2 = Page::create('another_page'); + self::assertNotEquals($sn, $this->session->addLink($page2)); + } + + public function test_addLink_page_already_added(): void { + // If we add the same page object twice, it will give the same SN. + $page = Page::create('some_page'); + $sn = $this->session->addLink($page); + self::assertSame($sn, $this->session->addLink($page)); + + // This works if the pages are equal, but not the same object. + $page2 = clone $page; + self::assertNotSame($page, $page2); + self::assertSame($sn, $this->session->addLink($page2)); + + // It also works if we modify the page object (though this isn't + // recommended, we clone when adding from Page::href to avoid this). + $page['bla'] = true; + self::assertSame($sn, $this->session->addLink($page)); + } + + public function test_clearLinks(): void { + srand(0); // seed rng to avoid getting the same random SN twice + $page = Page::create('some_page'); + $sn = $this->session->addLink($page); + + // After clearing links, the same page will return a different SN. + $this->session->clearLinks(); + self::assertNotSame($sn, $this->session->addLink($page)); + } + public function test_getRequestVar(): void { // Initialize the current var so that we can update it $page = Page::create('some_page'); From d0481356848048a068e83e519a11b52142afcd0f Mon Sep 17 00:00:00 2001 From: Dan Hemberger Date: Sat, 19 Nov 2022 21:02:20 -0800 Subject: [PATCH 2/3] message_view.php: consolidate set read logic Move the database query to mark messages as read into the SmrPlayer method `setMessagesRead`. This is helpful because it consolidates the queries that should not be performed on ajax updates. --- src/engine/Default/message_view.php | 12 ++++-------- src/lib/Default/AbstractSmrPlayer.php | 2 ++ 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/engine/Default/message_view.php b/src/engine/Default/message_view.php index 05823ef3e..4bc1266f3 100644 --- a/src/engine/Default/message_view.php +++ b/src/engine/Default/message_view.php @@ -51,11 +51,6 @@ $template->assign('NextPageHREF', $container->href()); } -// remove entry for this folder from unread msg table -if ($page == 0 && !USING_AJAX) { - $player->setMessagesRead($messageBox['Type']); -} - $messageBox['Name'] = Messages::getMessageTypeNames($folderID); $template->assign('PageTopic', 'Viewing ' . $messageBox['Name']); @@ -94,10 +89,11 @@ $messageBox['Messages'][] = displayMessage($dbRecord->getInt('message_id'), $dbRecord->getInt('account_id'), $dbRecord->getInt('sender_id'), $player->getGameID(), $dbRecord->getString('message_text'), $dbRecord->getInt('send_time'), $dbRecord->getBoolean('msg_read'), $folderID, $player->getAccount()); } } -if (!USING_AJAX) { - $db->write('UPDATE message SET msg_read = \'TRUE\' - WHERE message_type_id = ' . $db->escapeNumber($folderID) . ' AND ' . $player->getSQL()); + +if ($page == 0 && !USING_AJAX) { + $player->setMessagesRead($folderID); } + $template->assign('MessageBox', $messageBox); diff --git a/src/lib/Default/AbstractSmrPlayer.php b/src/lib/Default/AbstractSmrPlayer.php index e8b7c5fcf..0c53320d0 100644 --- a/src/lib/Default/AbstractSmrPlayer.php +++ b/src/lib/Default/AbstractSmrPlayer.php @@ -873,6 +873,8 @@ public static function sendMessageFromRace(int $raceID, int $gameID, int $receiv public function setMessagesRead(int $messageTypeID): void { $this->db->write('DELETE FROM player_has_unread_messages WHERE ' . $this->SQL . ' AND message_type_id = ' . $this->db->escapeNumber($messageTypeID)); + $this->db->write('UPDATE message SET msg_read = ' . $this->db->escapeBoolean(true) . ' + WHERE message_type_id = ' . $this->db->escapeNumber($messageTypeID) . ' AND ' . $this->SQL); } public function getSafeAttackRating(): int { From 6cda6a9ed1a1ffb9d374e40aff5472bff9872edd Mon Sep 17 00:00:00 2001 From: Dan Hemberger Date: Sat, 19 Nov 2022 21:24:42 -0800 Subject: [PATCH 3/3] Move USING_AJAX into the Session class Defining `USING_AJAX` as a global constant made it hard to test, since it cannot be modified once set. Moving it into the Session class as the `ajax` property makes it easier to reset, and also much more clear when it will be defined. In general, we should avoid global constants when they are not configuration parameters. --- phpstan.neon.dist | 1 - src/bootstrap.php | 5 +++-- src/config.php | 2 -- src/engine/Default/message_view.php | 3 ++- src/engine/Default/toggle_processing.php | 2 +- src/htdocs/loader.php | 6 ++---- src/lib/Default/AbstractSmrPlayer.php | 2 +- src/lib/Default/smr.inc.php | 13 +++++++------ src/lib/Smr/Session.php | 10 ++++++---- .../lib/DefaultGame/SessionIntegrationTest.php | 12 ++++++++++++ 10 files changed, 34 insertions(+), 22 deletions(-) diff --git a/phpstan.neon.dist b/phpstan.neon.dist index 3fab75b6c..c001207c7 100644 --- a/phpstan.neon.dist +++ b/phpstan.neon.dist @@ -27,7 +27,6 @@ parameters: - RECAPTCHA_PRIVATE - SMTP_HOSTNAME - TWITTER_CONSUMER_KEY - - USING_AJAX # We code in protection against a value of 0 - SmrMines::TOTAL_ENEMY_MINES_MODIFIER diff --git a/src/bootstrap.php b/src/bootstrap.php index 5af39c6b7..c6eb4f5a8 100644 --- a/src/bootstrap.php +++ b/src/bootstrap.php @@ -27,6 +27,8 @@ function logException(Throwable $e): void { $message .= $delim; } + $message .= 'ajax: ' . var_export($session->ajax, true) . "\n"; + $var = $session->hasCurrentVar() ? $session->getCurrentVar() : null; $message .= '$var: ' . print_r($var, true) . $delim; } @@ -41,7 +43,6 @@ function logException(Throwable $e): void { $message .= 'User IP: ' . getIpAddress() . "\n" . 'User Agent: ' . ($_SERVER['HTTP_USER_AGENT'] ?? 'undefined') . "\n" . - 'USING_AJAX: ' . (defined('USING_AJAX') ? var_export(USING_AJAX, true) : 'undefined') . "\n" . 'URL: ' . (defined('URL') ? URL : 'undefined'); // Try to release lock so they can carry on normally @@ -122,7 +123,7 @@ function handleException(Throwable $e): void { // If this is an ajax update, we don't really have a way to redirect // to an error page at this time. - if (!ENABLE_DEBUG && (!defined('USING_AJAX') || !USING_AJAX)) { + if (!ENABLE_DEBUG) { header('location: /error.php?msg=' . urlencode($errorType)); } } diff --git a/src/config.php b/src/config.php index 28aa96aeb..66719078f 100644 --- a/src/config.php +++ b/src/config.php @@ -471,5 +471,3 @@ const AJAX_DEFAULT_REFRESH_TIME = 1500; const AJAX_UNPROTECTED_REFRESH_TIME = 800; - -define('USING_AJAX', isset($_REQUEST['ajax']) && $_REQUEST['ajax'] == 1); diff --git a/src/engine/Default/message_view.php b/src/engine/Default/message_view.php index 4bc1266f3..9e09c42df 100644 --- a/src/engine/Default/message_view.php +++ b/src/engine/Default/message_view.php @@ -90,7 +90,8 @@ } } -if ($page == 0 && !USING_AJAX) { +// This should really be part of a (pre)processing page +if ($page == 0 && !$session->ajax) { $player->setMessagesRead($folderID); } diff --git a/src/engine/Default/toggle_processing.php b/src/engine/Default/toggle_processing.php index 0260c62b3..18d15182f 100644 --- a/src/engine/Default/toggle_processing.php +++ b/src/engine/Default/toggle_processing.php @@ -5,7 +5,7 @@ $player->setDisplayWeapons(!$player->isDisplayWeapons()); // If this is called by ajax, we don't want to do any forwarding -if (USING_AJAX) { +if ($session->ajax) { exit; } diff --git a/src/htdocs/loader.php b/src/htdocs/loader.php index 5ab491b7a..6cabcf903 100644 --- a/src/htdocs/loader.php +++ b/src/htdocs/loader.php @@ -19,8 +19,6 @@ // * // ******************************** - //echo '
';echo_r($session);echo'
'; - //exit; // do we have a session? $session = Smr\Session::getInstance(); if (!$session->hasAccount()) { @@ -30,7 +28,7 @@ // check if we got a sn number with our url if (empty($session->getSN())) { - if (!USING_AJAX) { + if (!$session->ajax) { create_error('Your browser lost the SN. Try to reload the page!'); } else { exit; @@ -39,7 +37,7 @@ // do we have such a container object in the db? if ($session->hasCurrentVar() === false) { - if (!USING_AJAX) { + if (!$session->ajax) { create_error('Please avoid using the back button!'); } else { exit; diff --git a/src/lib/Default/AbstractSmrPlayer.php b/src/lib/Default/AbstractSmrPlayer.php index 0c53320d0..b26515469 100644 --- a/src/lib/Default/AbstractSmrPlayer.php +++ b/src/lib/Default/AbstractSmrPlayer.php @@ -2256,7 +2256,7 @@ public function removeUnderAttack(): bool { return $var['UnderAttack']; } $underAttack = $this->isUnderAttack(); - if ($underAttack && !USING_AJAX) { + if ($underAttack && !$session->ajax) { $var['UnderAttack'] = $underAttack; //Remember we are under attack for AJAX } $this->setUnderAttack(false); diff --git a/src/lib/Default/smr.inc.php b/src/lib/Default/smr.inc.php index 806263abf..eb64c024c 100644 --- a/src/lib/Default/smr.inc.php +++ b/src/lib/Default/smr.inc.php @@ -202,7 +202,8 @@ function handleUserError(string $message): never { // Template from the original page. DiContainer::getContainer()->reset(Template::class); - if (Session::getInstance()->hasGame()) { + $session = Session::getInstance(); + if ($session->hasGame()) { $container = Page::create('current_sector.php'); $errorMsg = 'ERROR: ' . $message; $container['errorMsg'] = $errorMsg; @@ -211,7 +212,7 @@ function handleUserError(string $message): never { $container['message'] = $message; } - if (USING_AJAX) { + if ($session->ajax) { // To avoid the page just not refreshing when an error is encountered // during ajax updates, use javascript to auto-redirect to the // appropriate error page. @@ -308,7 +309,7 @@ function do_voodoo(): never { define('AJAX_CONTAINER', isset($var['AJAX']) && $var['AJAX'] === true); } - if (!AJAX_CONTAINER && USING_AJAX && $session->hasChangedSN()) { + if (!AJAX_CONTAINER && $session->ajax && $session->hasChangedSN()) { exit; } //ob_clean(); @@ -322,7 +323,7 @@ function do_voodoo(): never { $player = $session->getPlayer(); $sectorID = $player->getSectorID(); - if (!USING_AJAX //AJAX should never do anything that requires a lock. + if (!$session->ajax //AJAX should never do anything that requires a lock. //&& ($var->file == 'current_sector.php' || $var->file == 'map_local.php') //Neither should CS or LM and they gets loaded a lot so should reduce lag issues with big groups. ) { // We skip locking if we've already failed to display error page @@ -358,7 +359,7 @@ function do_voodoo(): never { $player->updateTurns(); // Check if we need to redirect to a different page - if (!$var->skipRedirect && !USING_AJAX) { + if (!$var->skipRedirect && !$session->ajax) { if ($player->getGame()->hasEnded()) { Page::create('game_leave_processing.php', ['forward_to' => 'game_play.php', 'errorMsg' => 'The game has ended.'], skipRedirect: true)->go(); } @@ -408,7 +409,7 @@ function do_voodoo(): never { } $template->assign('AJAX_ENABLE_REFRESH', $ajaxRefresh); - $template->display('skeleton.php', USING_AJAX || AJAX_CONTAINER); + $template->display('skeleton.php', $session->ajax || AJAX_CONTAINER); $session->update(); diff --git a/src/lib/Smr/Session.php b/src/lib/Smr/Session.php index 4be4f51bc..c2b454c86 100644 --- a/src/lib/Smr/Session.php +++ b/src/lib/Smr/Session.php @@ -33,6 +33,7 @@ class Session { private array $links = []; private ?Page $currentPage = null; private bool $generate; + public readonly bool $ajax; private string $SN; private string $lastSN; private int $accountID; @@ -82,10 +83,11 @@ public function __construct() { $this->db->write('DELETE FROM active_session WHERE last_accessed < ' . $this->db->escapeNumber(time() - self::TIME_BEFORE_EXPIRY)); // try to get current session + $this->ajax = Request::getInt('ajax', 0) === 1; $this->SN = Request::get('sn', ''); $this->fetchVarInfo(); - if (!USING_AJAX && $this->hasCurrentVar()) { + if (!$this->ajax && $this->hasCurrentVar()) { $file = $this->getCurrentVar()->file; $loadDelay = self::URL_LOAD_DELAY[$file] ?? 0; $timeBetweenLoads = microtime(true) - $this->lastAccessed; @@ -119,7 +121,7 @@ public function fetchVarInfo(): void { [$this->links, $lastPage] = $dbRecord->getObject('session_var', true); - $ajaxRefresh = USING_AJAX && !$this->hasChangedSN(); + $ajaxRefresh = $this->ajax && !$this->hasChangedSN(); if ($ajaxRefresh) { $this->currentPage = $lastPage; } elseif (isset($this->links[$this->SN])) { @@ -146,9 +148,9 @@ public function fetchVarInfo(): void { public function update(): void { $sessionVar = [$this->links, $this->currentPage]; if (!$this->generate) { - $this->db->write('UPDATE active_session SET account_id=' . $this->db->escapeNumber($this->accountID) . ',game_id=' . $this->db->escapeNumber($this->gameID) . (!USING_AJAX ? ',last_accessed=' . $this->db->escapeNumber(Epoch::microtime()) : '') . ',session_var=' . $this->db->escapeObject($sessionVar, true) . + $this->db->write('UPDATE active_session SET account_id=' . $this->db->escapeNumber($this->accountID) . ',game_id=' . $this->db->escapeNumber($this->gameID) . (!$this->ajax ? ',last_accessed=' . $this->db->escapeNumber(Epoch::microtime()) : '') . ',session_var=' . $this->db->escapeObject($sessionVar, true) . ',last_sn=' . $this->db->escapeString($this->SN) . - ' WHERE session_id=' . $this->db->escapeString($this->sessionID) . (USING_AJAX ? ' AND last_sn=' . $this->db->escapeString($this->lastSN) : '')); + ' WHERE session_id=' . $this->db->escapeString($this->sessionID) . ($this->ajax ? ' AND last_sn=' . $this->db->escapeString($this->lastSN) : '')); } else { $this->db->write('DELETE FROM active_session WHERE account_id = ' . $this->db->escapeNumber($this->accountID) . ' AND game_id = ' . $this->db->escapeNumber($this->gameID)); $this->db->insert('active_session', [ diff --git a/test/SmrTest/lib/DefaultGame/SessionIntegrationTest.php b/test/SmrTest/lib/DefaultGame/SessionIntegrationTest.php index 997306053..b903817c5 100644 --- a/test/SmrTest/lib/DefaultGame/SessionIntegrationTest.php +++ b/test/SmrTest/lib/DefaultGame/SessionIntegrationTest.php @@ -89,6 +89,17 @@ public function test_getSessionID(): void { self::assertNotEquals($sessionID, $session->getSessionID()); } + public function test_ajax(): void { + // If $_REQUEST is empty, ajax is false + self::assertFalse($this->session->ajax); + + // Test other values in $_REQUEST + $_REQUEST['ajax'] = 1; + self::assertTrue(DiContainer::make(Session::class)->ajax); + $_REQUEST['ajax'] = 'anything other than 1'; + self::assertFalse(DiContainer::make(Session::class)->ajax); + } + public function test_current_var(): void { // With an empty session, there should be no current var self::assertFalse($this->session->hasCurrentVar()); @@ -125,6 +136,7 @@ public function test_current_var(): void { // we should still get the updated var, even though it wasn't // the one originally associated with this SN. $session->update(); + $_REQUEST['ajax'] = 1; $session = DiContainer::make(Session::class); self::assertEquals($var2, $session->getCurrentVar());