From 1aedd2df73ad2be69fb24148bfaff31c118fddba Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Wed, 10 Feb 2016 11:43:23 -0500 Subject: [PATCH] Session: Implement ArrayAccess Now that we dropped support for PHP 5.3.3, we can do this. The behavior of $session['foo'] when that key doesn't already exist is a little unexpected (it implicitly assigns null), but it's the best we can do. Change-Id: Ibef878867d46591a8bf542139a1719dfec3b83ab --- includes/session/Session.php | 47 +++++++++++-- includes/session/SessionBackend.php | 2 +- .../includes/session/SessionBackendTest.php | 1 + .../phpunit/includes/session/SessionTest.php | 68 ++++++++++++++++++- tests/phpunit/includes/session/TestUtils.php | 10 ++- 5 files changed, 120 insertions(+), 8 deletions(-) diff --git a/includes/session/Session.php b/includes/session/Session.php index 4ad69ae802..d654ff1449 100644 --- a/includes/session/Session.php +++ b/includes/session/Session.php @@ -23,6 +23,7 @@ namespace MediaWiki\Session; +use Psr\Log\LoggerInterface; use User; use WebRequest; @@ -41,24 +42,28 @@ use WebRequest; * The Session object also serves as a replacement for PHP's $_SESSION, * managing access to per-session data. * - * @todo Once we drop support for PHP 5.3.3, implementing ArrayAccess would be nice. * @ingroup Session * @since 1.27 */ -final class Session implements \Countable, \Iterator { +final class Session implements \Countable, \Iterator, \ArrayAccess { /** @var SessionBackend Session backend */ private $backend; /** @var int Session index */ private $index; + /** @var LoggerInterface */ + private $logger; + /** * @param SessionBackend $backend * @param int $index + * @param LoggerInterface $logger */ - public function __construct( SessionBackend $backend, $index ) { + public function __construct( SessionBackend $backend, $index, LoggerInterface $logger ) { $this->backend = $backend; $this->index = $index; + $this->logger = $logger; } public function __destruct() { @@ -271,7 +276,7 @@ final class Session implements \Countable, \Iterator { /** * Fetch a value from the session * @param string|int $key - * @param mixed $default + * @param mixed $default Returned if $this->exists( $key ) would be false * @return mixed */ public function get( $key, $default = null ) { @@ -281,6 +286,7 @@ final class Session implements \Countable, \Iterator { /** * Test if a value exists in the session + * @note Unlike isset(), null values are considered to exist. * @param string|int $key * @return bool */ @@ -419,6 +425,39 @@ final class Session implements \Countable, \Iterator { return key( $data ) !== null; } + /** + * @note Despite the name, this seems to be intended to implement isset() + * rather than array_key_exists(). So do that. + */ + public function offsetExists( $offset ) { + $data = &$this->backend->getData(); + return isset( $data[$offset] ); + } + + /** + * @note This supports indirect modifications but can't mark the session + * dirty when those happen. SessionBackend::save() checks the hash of the + * data to detect such changes. + * @note Accessing a nonexistent key via this mechanism causes that key to + * be created with a null value, and does not raise a PHP warning. + */ + public function &offsetGet( $offset ) { + $data = &$this->backend->getData(); + if ( !array_key_exists( $offset, $data ) ) { + $ex = new \Exception( "Undefined index (auto-adds to session with a null value): $offset" ); + $this->logger->debug( $ex->getMessage(), array( 'exception' => $ex ) ); + } + return $data[$offset]; + } + + public function offsetSet( $offset, $value ) { + $this->set( $offset, $value ); + } + + public function offsetUnset( $offset ) { + $this->remove( $offset ); + } + /**@}*/ } diff --git a/includes/session/SessionBackend.php b/includes/session/SessionBackend.php index fe446e356b..5cf7869f41 100644 --- a/includes/session/SessionBackend.php +++ b/includes/session/SessionBackend.php @@ -170,7 +170,7 @@ final class SessionBackend { public function getSession( WebRequest $request ) { $index = ++$this->curIndex; $this->requests[$index] = $request; - $session = new Session( $this, $index ); + $session = new Session( $this, $index, $this->logger ); return $session; } diff --git a/tests/phpunit/includes/session/SessionBackendTest.php b/tests/phpunit/includes/session/SessionBackendTest.php index f08a07d7d4..481b693ae6 100644 --- a/tests/phpunit/includes/session/SessionBackendTest.php +++ b/tests/phpunit/includes/session/SessionBackendTest.php @@ -569,6 +569,7 @@ class SessionBackendTest extends MediaWikiTestCase { 'making sure it did save to backend' ); // Not marked dirty, but dirty data + // (e.g. indirect modification from ArrayAccess::offsetGet) $this->provider = $neverProvider; $this->onSessionMetadataCalled = false; $this->mergeMwGlobalArrayValue( 'wgHooks', array( 'SessionMetadata' => array( $this ) ) ); diff --git a/tests/phpunit/includes/session/SessionTest.php b/tests/phpunit/includes/session/SessionTest.php index 858996d63a..3c5090a9a9 100644 --- a/tests/phpunit/includes/session/SessionTest.php +++ b/tests/phpunit/includes/session/SessionTest.php @@ -2,6 +2,7 @@ namespace MediaWiki\Session; +use Psr\Log\LogLevel; use MediaWikiTestCase; use User; @@ -16,7 +17,7 @@ class SessionTest extends MediaWikiTestCase { \TestingAccessWrapper::newFromObject( $backend )->requests = array( -1 => 'dummy' ); \TestingAccessWrapper::newFromObject( $backend )->id = new SessionId( 'abc' ); - $session = new Session( $backend, 42 ); + $session = new Session( $backend, 42, new \TestLogger ); $priv = \TestingAccessWrapper::newFromObject( $session ); $this->assertSame( $backend, $priv->backend ); $this->assertSame( 42, $priv->index ); @@ -152,6 +153,71 @@ class SessionTest extends MediaWikiTestCase { $this->assertFalse( $backend->dirty ); } + public function testArrayAccess() { + $logger = new \TestLogger; + $session = TestUtils::getDummySession( null, -1, $logger ); + $backend = \TestingAccessWrapper::newFromObject( $session )->backend; + + $this->assertEquals( 1, $session['foo'] ); + $this->assertEquals( 'zero', $session[0] ); + $this->assertFalse( $backend->dirty ); + + $logger->setCollect( true ); + $this->assertEquals( null, $session['null'] ); + $logger->setCollect( false ); + $this->assertFalse( $backend->dirty ); + $this->assertSame( array( + array( LogLevel::DEBUG, 'Undefined index (auto-adds to session with a null value): null' ) + ), $logger->getBuffer() ); + $logger->clearBuffer(); + + $session['foo'] = 55; + $this->assertEquals( 55, $backend->data['foo'] ); + $this->assertTrue( $backend->dirty ); + $backend->dirty = false; + + $session[1] = 'one'; + $this->assertEquals( 'one', $backend->data[1] ); + $this->assertTrue( $backend->dirty ); + $backend->dirty = false; + + $session[1] = 'one'; + $this->assertFalse( $backend->dirty ); + + $session['bar'] = array( 'baz' => array() ); + $session['bar']['baz']['quux'] = 2; + $this->assertEquals( array( 'baz' => array( 'quux' => 2 ) ), $backend->data['bar'] ); + + $logger->setCollect( true ); + $session['bar2']['baz']['quux'] = 3; + $logger->setCollect( false ); + $this->assertEquals( array( 'baz' => array( 'quux' => 3 ) ), $backend->data['bar2'] ); + $this->assertSame( array( + array( LogLevel::DEBUG, 'Undefined index (auto-adds to session with a null value): bar2' ) + ), $logger->getBuffer() ); + $logger->clearBuffer(); + + $backend->dirty = false; + $this->assertTrue( isset( $session['foo'] ) ); + $this->assertTrue( isset( $session[1] ) ); + $this->assertFalse( isset( $session['null'] ) ); + $this->assertFalse( isset( $session['missing'] ) ); + $this->assertFalse( isset( $session[100] ) ); + $this->assertFalse( $backend->dirty ); + + unset( $session['foo'] ); + $this->assertArrayNotHasKey( 'foo', $backend->data ); + $this->assertTrue( $backend->dirty ); + $backend->dirty = false; + unset( $session[1] ); + $this->assertArrayNotHasKey( 1, $backend->data ); + $this->assertTrue( $backend->dirty ); + $backend->dirty = false; + + unset( $session[101] ); + $this->assertFalse( $backend->dirty ); + } + public function testClear() { $session = TestUtils::getDummySession(); $priv = \TestingAccessWrapper::newFromObject( $session ); diff --git a/tests/phpunit/includes/session/TestUtils.php b/tests/phpunit/includes/session/TestUtils.php index 16199837ee..cc20ab5035 100644 --- a/tests/phpunit/includes/session/TestUtils.php +++ b/tests/phpunit/includes/session/TestUtils.php @@ -2,6 +2,8 @@ namespace MediaWiki\Session; +use Psr\Log\LoggerInterface; + /** * Utility functions for Session unit tests */ @@ -67,7 +69,9 @@ class TestUtils { ); } - return $rc->newInstanceWithoutConstructor(); + $ret = $rc->newInstanceWithoutConstructor(); + \TestingAccessWrapper::newFromObject( $ret )->logger = new \TestLogger; + return $ret; } /** @@ -75,9 +79,10 @@ class TestUtils { * construct one, use this. * @param object $backend Object to serve as the SessionBackend * @param int $index Index + * @param LoggerInterface $logger * @return Session */ - public static function getDummySession( $backend = null, $index = -1 ) { + public static function getDummySession( $backend = null, $index = -1, $logger = null ) { $rc = new \ReflectionClass( 'MediaWiki\\Session\\Session' ); if ( !method_exists( $rc, 'newInstanceWithoutConstructor' ) ) { \PHPUnit_Framework_Assert::markTestSkipped( @@ -93,6 +98,7 @@ class TestUtils { $priv = \TestingAccessWrapper::newFromObject( $session ); $priv->backend = $backend; $priv->index = $index; + $priv->logger = $logger ?: new \TestLogger; return $session; } -- 2.20.1