diff --git a/lib/classes/session/database.php b/lib/classes/session/database.php index c8c842fcf7b..6b15884018b 100644 --- a/lib/classes/session/database.php +++ b/lib/classes/session/database.php @@ -24,7 +24,7 @@ namespace core\session; -defined('MOODLE_INTERNAL') || die(); +use SessionHandlerInterface; /** * Database based session handler. @@ -33,7 +33,7 @@ defined('MOODLE_INTERNAL') || die(); * @copyright 2013 Petr Skoda {@link http://skodak.org} * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class database extends handler { +class database extends handler implements SessionHandlerInterface { /** @var \stdClass $record session record */ protected $recordid = null; @@ -70,12 +70,7 @@ class database extends handler { throw new exception('sessionhandlerproblem', 'error', '', null, 'Database does not support session locking'); } - $result = session_set_save_handler(array($this, 'handler_open'), - array($this, 'handler_close'), - array($this, 'handler_read'), - array($this, 'handler_write'), - array($this, 'handler_destroy'), - array($this, 'handler_gc')); + $result = session_set_save_handler($this); if (!$result) { throw new exception('dbsessionhandlerproblem', 'error'); } @@ -117,11 +112,11 @@ class database extends handler { * * {@see http://php.net/manual/en/function.session-set-save-handler.php} * - * @param string $save_path - * @param string $session_name + * @param string $path + * @param string $name * @return bool success */ - public function handler_open($save_path, $session_name) { + public function open(string $path, string $name): bool { // Note: we use the already open database. return true; } @@ -133,7 +128,7 @@ class database extends handler { * * @return bool success */ - public function handler_close() { + public function close(): bool { if ($this->recordid) { try { $this->database->release_session_lock($this->recordid); @@ -152,9 +147,9 @@ class database extends handler { * {@see http://php.net/manual/en/function.session-set-save-handler.php} * * @param string $sid - * @return string + * @return string|false */ - public function handler_read($sid) { + public function read(string $sid): string|false { try { if (!$record = $this->database->get_record('sessions', array('sid'=>$sid), 'id')) { // Let's cheat and skip locking if this is the first access, @@ -221,17 +216,18 @@ class database extends handler { * NOTE: Do not write to output or throw any exceptions! * Hopefully the next page is going to display nice error or it recovers... * - * @param string $sid - * @param string $session_data + * @param string $id + * @param string $data * @return bool success */ - public function handler_write($sid, $session_data) { + public function write(string $id, string $data): bool { if ($this->failed) { // Do not write anything back - we failed to start the session properly. return false; } - $sessdata = base64_encode($session_data); // There might be some binary mess :-( + // There might be some binary mess. + $sessdata = base64_encode($data); $hash = sha1($sessdata); if ($hash === $this->lasthash) { @@ -240,14 +236,17 @@ class database extends handler { try { if ($this->recordid) { - $this->database->set_field('sessions', 'sessdata', $sessdata, array('id'=>$this->recordid)); + $this->database->set_field('sessions', 'sessdata', $sessdata, ['id' => $this->recordid]); } else { // This happens in the first request when session record was just created in manager. - $this->database->set_field('sessions', 'sessdata', $sessdata, array('sid'=>$sid)); + $this->database->set_field('sessions', 'sessdata', $sessdata, ['sid' => $id]); } } catch (\Exception $ex) { // Do not rethrow exceptions here, this should not happen. - error_log('Unknown exception when writing database session data : '.$sid.' - '.$ex->getMessage()); + // phpcs:ignore moodle.PHP.ForbiddenFunctions.FoundWithAlternative + error_log( + "Unknown exception when writing database session data : {$id} - " . $ex->getMessage(), + ); } return true; @@ -258,19 +257,19 @@ class database extends handler { * * {@see http://php.net/manual/en/function.session-set-save-handler.php} * - * @param string $sid + * @param string $id * @return bool success */ - public function handler_destroy($sid) { - if (!$session = $this->database->get_record('sessions', array('sid'=>$sid), 'id, sid')) { - if ($sid == session_id()) { + public function destroy(string $id): bool { + if (!$session = $this->database->get_record('sessions', ['sid' => $id], 'id, sid')) { + if ($id == session_id()) { $this->recordid = null; $this->lasthash = null; } return true; } - if ($this->recordid and $session->id == $this->recordid) { + if ($this->recordid && ($session->id == $this->recordid)) { try { $this->database->release_session_lock($this->recordid); } catch (\Exception $ex) { @@ -280,7 +279,7 @@ class database extends handler { $this->lasthash = null; } - $this->database->delete_records('sessions', array('id'=>$session->id)); + $this->database->delete_records('sessions', ['id' => $session->id]); return true; } @@ -290,16 +289,19 @@ class database extends handler { * * {@see http://php.net/manual/en/function.session-set-save-handler.php} * - * @param int $ignored_maxlifetime moodle uses special timeout rules + * @param int $max_lifetime moodle uses special timeout rules * @return bool success */ - public function handler_gc($ignored_maxlifetime) { + // phpcs:ignore moodle.NamingConventions.ValidVariableName.VariableNameUnderscore + public function gc(int $max_lifetime): int|false { // This should do something only if cron is not running properly... if (!$stalelifetime = ini_get('session.gc_maxlifetime')) { - return true; + return false; } - $params = array('purgebefore' => (time() - $stalelifetime)); + $params = ['purgebefore' => (time() - $stalelifetime)]; + $count = $this->database->count_records_select('sessions', 'userid = 0 AND timemodified < :purgebefore', $params); $this->database->delete_records_select('sessions', 'userid = 0 AND timemodified < :purgebefore', $params); - return true; + + return $count; } } diff --git a/lib/classes/session/redis.php b/lib/classes/session/redis.php index c3b623d78f2..b6b3cf95404 100644 --- a/lib/classes/session/redis.php +++ b/lib/classes/session/redis.php @@ -25,8 +25,7 @@ namespace core\session; use RedisException; - -defined('MOODLE_INTERNAL') || die(); +use SessionHandlerInterface; /** * Redis based session handler. @@ -39,7 +38,7 @@ defined('MOODLE_INTERNAL') || die(); * @copyright 2016 Russell Smith * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class redis extends handler { +class redis extends handler implements SessionHandlerInterface { /** * Compressor: none. */ @@ -204,12 +203,8 @@ class redis extends handler { $this->connection = new \Redis(); - $result = session_set_save_handler(array($this, 'handler_open'), - array($this, 'handler_close'), - array($this, 'handler_read'), - array($this, 'handler_write'), - array($this, 'handler_destroy'), - array($this, 'handler_gc')); + $result = session_set_save_handler($this); + if (!$result) { throw new exception('redissessionhandlerproblem', 'error'); } @@ -288,11 +283,11 @@ class redis extends handler { /** * Update our session search path to include session name when opened. * - * @param string $savepath unused session save path. (ignored) - * @param string $sessionname Session name for this session. (ignored) + * @param string $path unused session save path. (ignored) + * @param string $name Session name for this session. (ignored) * @return bool true always as we will succeed. */ - public function handler_open($savepath, $sessionname) { + public function open(string $path, string $name): bool { return true; } @@ -301,7 +296,7 @@ class redis extends handler { * * @return bool true on success. false on unable to unlock sessions. */ - public function handler_close() { + public function close(): bool { $this->lasthash = null; try { foreach ($this->locks as $id => $expirytime) { @@ -317,6 +312,7 @@ class redis extends handler { return true; } + /** * Read the session data from storage * @@ -325,7 +321,7 @@ class redis extends handler { * * @throws RedisException when we are unable to talk to the Redis server. */ - public function handler_read($id) { + public function read(string $id): string|false { try { if ($this->requires_write_lock()) { $this->lock_session($id); @@ -402,7 +398,7 @@ class redis extends handler { * @param string $data session data * @return bool true on write success, false on failure */ - public function handler_write($id, $data) { + public function write(string $id, string $data): bool { $hash = sha1(base64_encode($data)); @@ -438,7 +434,7 @@ class redis extends handler { * @param string $id the session id to destroy. * @return bool true if the session was deleted, false otherwise. */ - public function handler_destroy($id) { + public function destroy(string $id): bool { $this->lasthash = null; try { $this->connection->del($id); @@ -454,11 +450,12 @@ class redis extends handler { /** * Garbage collect sessions. We don't we any as Redis does it for us. * - * @param integer $maxlifetime All sessions older than this should be removed. + * @param integer $max_lifetime All sessions older than this should be removed. * @return bool true, as Redis handles expiry for us. */ - public function handler_gc($maxlifetime) { - return true; + // phpcs:ignore moodle.NamingConventions.ValidVariableName.VariableNameUnderscore + public function gc(int $max_lifetime): int|false { + return false; } /** @@ -601,7 +598,7 @@ class redis extends handler { $rs = $DB->get_recordset('sessions', array(), 'id DESC', 'id, sid'); foreach ($rs as $record) { - $this->handler_destroy($record->sid); + $this->destroy($record->sid); } $rs->close(); } @@ -616,6 +613,6 @@ class redis extends handler { return; } - $this->handler_destroy($sid); + $this->destroy($sid); } } diff --git a/lib/dml/tests/dml_read_slave_test.php b/lib/dml/tests/dml_read_slave_test.php index 4ded903cb7d..d0f04e0c09e 100644 --- a/lib/dml/tests/dml_read_slave_test.php +++ b/lib/dml/tests/dml_read_slave_test.php @@ -562,7 +562,7 @@ class dml_read_slave_test extends \base_testcase { $this->assertNull($DB->get_dbhwrite()); $session = new \core\session\database(); - $session->handler_read('dummy'); + $session->read('dummy'); $this->assertEquals(0, $DB->perf_get_reads_slave()); $this->assertTrue($DB->perf_get_reads() > 0); diff --git a/lib/tests/session_redis_test.php b/lib/tests/session_redis_test.php index b5d9253c5e8..a77a3375f00 100644 --- a/lib/tests/session_redis_test.php +++ b/lib/tests/session_redis_test.php @@ -113,24 +113,24 @@ class session_redis_test extends \advanced_testcase { $sess = new \core\session\redis(); $sess->set_requires_write_lock(false); $sess->init(); - $this->assertSame('', $sess->handler_read('sess1')); - $this->assertTrue($sess->handler_close()); + $this->assertSame('', $sess->read('sess1')); + $this->assertTrue($sess->close()); } public function test_normal_session_start_stop_works() { $sess = new \core\session\redis(); $sess->init(); $sess->set_requires_write_lock(true); - $this->assertTrue($sess->handler_open('Not used', 'Not used')); - $this->assertSame('', $sess->handler_read('sess1')); - $this->assertTrue($sess->handler_write('sess1', 'DATA')); - $this->assertTrue($sess->handler_close()); + $this->assertTrue($sess->open('Not used', 'Not used')); + $this->assertSame('', $sess->read('sess1')); + $this->assertTrue($sess->write('sess1', 'DATA')); + $this->assertTrue($sess->close()); // Read the session again to ensure locking did what it should. - $this->assertTrue($sess->handler_open('Not used', 'Not used')); - $this->assertSame('DATA', $sess->handler_read('sess1')); - $this->assertTrue($sess->handler_write('sess1', 'DATA-new')); - $this->assertTrue($sess->handler_close()); + $this->assertTrue($sess->open('Not used', 'Not used')); + $this->assertSame('DATA', $sess->read('sess1')); + $this->assertTrue($sess->write('sess1', 'DATA-new')); + $this->assertTrue($sess->close()); $this->assertSessionNoLocks(); } @@ -141,18 +141,18 @@ class session_redis_test extends \advanced_testcase { $sess = new \core\session\redis(); $sess->init(); - $this->assertTrue($sess->handler_write('sess1', 'DATA')); - $this->assertSame('DATA', $sess->handler_read('sess1')); - $this->assertTrue($sess->handler_close()); + $this->assertTrue($sess->write('sess1', 'DATA')); + $this->assertSame('DATA', $sess->read('sess1')); + $this->assertTrue($sess->close()); if (extension_loaded('zstd')) { $CFG->session_redis_compressor = \core\session\redis::COMPRESSION_ZSTD; $sess = new \core\session\redis(); $sess->init(); - $this->assertTrue($sess->handler_write('sess2', 'DATA')); - $this->assertSame('DATA', $sess->handler_read('sess2')); - $this->assertTrue($sess->handler_close()); + $this->assertTrue($sess->write('sess2', 'DATA')); + $this->assertSame('DATA', $sess->read('sess2')); + $this->assertTrue($sess->close()); } $CFG->session_redis_compressor = \core\session\redis::COMPRESSION_NONE; @@ -162,25 +162,25 @@ class session_redis_test extends \advanced_testcase { $sess = new \core\session\redis(); $sess->init(); $sess->set_requires_write_lock(true); - $this->assertTrue($sess->handler_open('Not used', 'Not used')); - $this->assertSame('', $sess->handler_read('sess1')); - $this->assertTrue($sess->handler_write('sess1', 'DATA')); - $this->assertTrue($sess->handler_close()); + $this->assertTrue($sess->open('Not used', 'Not used')); + $this->assertSame('', $sess->read('sess1')); + $this->assertTrue($sess->write('sess1', 'DATA')); + $this->assertTrue($sess->close()); // Sessions are not locked until they have been saved once. - $this->assertTrue($sess->handler_open('Not used', 'Not used')); - $this->assertSame('DATA', $sess->handler_read('sess1')); + $this->assertTrue($sess->open('Not used', 'Not used')); + $this->assertSame('DATA', $sess->read('sess1')); $sessblocked = new \core\session\redis(); $sessblocked->init(); $sessblocked->set_requires_write_lock(true); - $this->assertTrue($sessblocked->handler_open('Not used', 'Not used')); + $this->assertTrue($sessblocked->open('Not used', 'Not used')); // Trap the error log and send it to stdOut so we can expect output at the right times. $errorlog = tempnam(sys_get_temp_dir(), "rediserrorlog"); $this->iniSet('error_log', $errorlog); try { - $sessblocked->handler_read('sess1'); + $sessblocked->read('sess1'); $this->fail('Session lock must fail to be obtained.'); } catch (\core\session\exception $e) { $this->assertStringContainsString("Unable to obtain lock for session id sess1", $e->getMessage()); @@ -189,9 +189,9 @@ class session_redis_test extends \advanced_testcase { $this->assertStringContainsString('Cannot obtain session lock for sid: sess1', file_get_contents($errorlog)); } - $this->assertTrue($sessblocked->handler_close()); - $this->assertTrue($sess->handler_write('sess1', 'DATA-new')); - $this->assertTrue($sess->handler_close()); + $this->assertTrue($sessblocked->close()); + $this->assertTrue($sess->write('sess1', 'DATA-new')); + $this->assertTrue($sess->close()); $this->assertSessionNoLocks(); } @@ -199,8 +199,8 @@ class session_redis_test extends \advanced_testcase { $sess = new \core\session\redis(); $sess->init(); $sess->set_requires_write_lock(true); - $this->assertTrue($sess->handler_open('Not used', 'Not used')); - $this->assertTrue($sess->handler_destroy('sess-destroy')); + $this->assertTrue($sess->open('Not used', 'Not used')); + $this->assertTrue($sess->destroy('sess-destroy')); $this->assertSessionNoLocks(); } @@ -208,10 +208,10 @@ class session_redis_test extends \advanced_testcase { $sess = new \core\session\redis(); $sess->init(); $sess->set_requires_write_lock(true); - $this->assertTrue($sess->handler_open('Not used', 'Not used')); - $this->assertSame('', $sess->handler_read('sess-destroy')); - $this->assertTrue($sess->handler_destroy('sess-destroy')); - $this->assertTrue($sess->handler_close()); + $this->assertTrue($sess->open('Not used', 'Not used')); + $this->assertSame('', $sess->read('sess-destroy')); + $this->assertTrue($sess->destroy('sess-destroy')); + $this->assertTrue($sess->close()); $this->assertSessionNoLocks(); } @@ -224,36 +224,36 @@ class session_redis_test extends \advanced_testcase { $sess2->init(); // Initialize session 1. - $this->assertTrue($sess1->handler_open('Not used', 'Not used')); - $this->assertSame('', $sess1->handler_read('sess1')); - $this->assertTrue($sess1->handler_write('sess1', 'DATA')); - $this->assertTrue($sess1->handler_close()); + $this->assertTrue($sess1->open('Not used', 'Not used')); + $this->assertSame('', $sess1->read('sess1')); + $this->assertTrue($sess1->write('sess1', 'DATA')); + $this->assertTrue($sess1->close()); // Initialize session 2. - $this->assertTrue($sess2->handler_open('Not used', 'Not used')); - $this->assertSame('', $sess2->handler_read('sess2')); - $this->assertTrue($sess2->handler_write('sess2', 'DATA2')); - $this->assertTrue($sess2->handler_close()); + $this->assertTrue($sess2->open('Not used', 'Not used')); + $this->assertSame('', $sess2->read('sess2')); + $this->assertTrue($sess2->write('sess2', 'DATA2')); + $this->assertTrue($sess2->close()); // Open and read session 1 and 2. - $this->assertTrue($sess1->handler_open('Not used', 'Not used')); - $this->assertSame('DATA', $sess1->handler_read('sess1')); - $this->assertTrue($sess2->handler_open('Not used', 'Not used')); - $this->assertSame('DATA2', $sess2->handler_read('sess2')); + $this->assertTrue($sess1->open('Not used', 'Not used')); + $this->assertSame('DATA', $sess1->read('sess1')); + $this->assertTrue($sess2->open('Not used', 'Not used')); + $this->assertSame('DATA2', $sess2->read('sess2')); // Write both sessions. - $this->assertTrue($sess1->handler_write('sess1', 'DATAX')); - $this->assertTrue($sess2->handler_write('sess2', 'DATA2X')); + $this->assertTrue($sess1->write('sess1', 'DATAX')); + $this->assertTrue($sess2->write('sess2', 'DATA2X')); // Read both sessions. - $this->assertTrue($sess1->handler_open('Not used', 'Not used')); - $this->assertTrue($sess2->handler_open('Not used', 'Not used')); - $this->assertEquals('DATAX', $sess1->handler_read('sess1')); - $this->assertEquals('DATA2X', $sess2->handler_read('sess2')); + $this->assertTrue($sess1->open('Not used', 'Not used')); + $this->assertTrue($sess2->open('Not used', 'Not used')); + $this->assertEquals('DATAX', $sess1->read('sess1')); + $this->assertEquals('DATA2X', $sess2->read('sess2')); // Close both sessions - $this->assertTrue($sess1->handler_close()); - $this->assertTrue($sess2->handler_close()); + $this->assertTrue($sess1->close()); + $this->assertTrue($sess2->close()); // Read the session again to ensure locking did what it should. $this->assertSessionNoLocks(); @@ -265,19 +265,19 @@ class session_redis_test extends \advanced_testcase { $sess->set_requires_write_lock(true); // Initialize session 1. - $this->assertTrue($sess->handler_open('Not used', 'Not used')); - $this->assertSame('', $sess->handler_read('sess1')); - $this->assertTrue($sess->handler_write('sess1', 'DATA')); - $this->assertSame('', $sess->handler_read('sess2')); - $this->assertTrue($sess->handler_write('sess2', 'DATA2')); - $this->assertSame('DATA', $sess->handler_read('sess1')); - $this->assertSame('DATA2', $sess->handler_read('sess2')); - $this->assertTrue($sess->handler_destroy('sess2')); + $this->assertTrue($sess->open('Not used', 'Not used')); + $this->assertSame('', $sess->read('sess1')); + $this->assertTrue($sess->write('sess1', 'DATA')); + $this->assertSame('', $sess->read('sess2')); + $this->assertTrue($sess->write('sess2', 'DATA2')); + $this->assertSame('DATA', $sess->read('sess1')); + $this->assertSame('DATA2', $sess->read('sess2')); + $this->assertTrue($sess->destroy('sess2')); - $this->assertTrue($sess->handler_close()); + $this->assertTrue($sess->close()); $this->assertSessionNoLocks(); - $this->assertTrue($sess->handler_close()); + $this->assertTrue($sess->close()); } public function test_session_exists_returns_valid_values() { @@ -285,13 +285,13 @@ class session_redis_test extends \advanced_testcase { $sess->init(); $sess->set_requires_write_lock(true); - $this->assertTrue($sess->handler_open('Not used', 'Not used')); - $this->assertSame('', $sess->handler_read('sess1')); + $this->assertTrue($sess->open('Not used', 'Not used')); + $this->assertSame('', $sess->read('sess1')); $this->assertFalse($sess->session_exists('sess1'), 'Session must not exist yet, it has not been saved'); - $this->assertTrue($sess->handler_write('sess1', 'DATA')); + $this->assertTrue($sess->write('sess1', 'DATA')); $this->assertTrue($sess->session_exists('sess1'), 'Session must exist now.'); - $this->assertTrue($sess->handler_destroy('sess1')); + $this->assertTrue($sess->destroy('sess1')); $this->assertFalse($sess->session_exists('sess1'), 'Session should be destroyed.'); } @@ -301,10 +301,10 @@ class session_redis_test extends \advanced_testcase { $sess = new \core\session\redis(); $sess->init(); - $this->assertTrue($sess->handler_open('Not used', 'Not used')); - $this->assertTrue($sess->handler_write('sess1', 'DATA')); - $this->assertTrue($sess->handler_write('sess2', 'DATA')); - $this->assertTrue($sess->handler_write('sess3', 'DATA')); + $this->assertTrue($sess->open('Not used', 'Not used')); + $this->assertTrue($sess->write('sess1', 'DATA')); + $this->assertTrue($sess->write('sess2', 'DATA')); + $this->assertTrue($sess->write('sess3', 'DATA')); $sessiondata = new \stdClass(); $sessiondata->userid = 2; @@ -318,9 +318,9 @@ class session_redis_test extends \advanced_testcase { $sessiondata->sid = 'sess3'; $DB->insert_record('sessions', $sessiondata); - $this->assertNotEquals('', $sess->handler_read('sess1')); + $this->assertNotEquals('', $sess->read('sess1')); $sess->kill_session('sess1'); - $this->assertEquals('', $sess->handler_read('sess1')); + $this->assertEquals('', $sess->read('sess1')); $this->assertEmpty($this->redis->keys($this->keyprefix.'sess1.lock'));