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

diff --git a/includes/session/Session.php b/includes/session/Session.php
index 4ad69ae..d654ff1 100644 (file)
--- 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 fe446e3..5cf7869 100644 (file)
--- 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 f08a07d..481b693 100644 (file)
--- 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 858996d..3c5090a 100644 (file)
--- 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 1619983..cc20ab5 100644 (file)
--- 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;
        }