resourceloader: Use -1 instead of null in DerivativeResourceLoaderContext

The ResourceLoaderContext class used null to determine absence of
an overridde in the derivative object.

However three of the members in question allow null as legitimate value.
(Namely 'only', 'user', and 'version').

This makes is impossible for a derivative context to remove one
of those values if the parent context has them set.

Use case: I782df43c needs to create a derivative context of
          load.php?only=scripts&modules=startup without 'only'.

Use -1 instead as internal placeholder value.

Also:

* ResourceLoaderContext::getSkin() was documented as returning 'string|null' when in
  fact it always has a default value. Never returns null.

* DerivativeResourceLoaderContext::setOnly() and setVersion() were missing
  type hint for 'null' (as it was incompatible with their getter). Adding 'false'.

* Swap if/else statements to handle the special case first (inheriting).
  Allowing the rest of the function body to handle the local value.
  In preparation for further development.

Change-Id: I058884525237effe8aef35469ed7693bb7cea591

diff --git a/includes/resourceloader/DerivativeResourceLoaderContext.php b/includes/resourceloader/DerivativeResourceLoaderContext.php
index 5784f2a..85d5434 100644 (file)
--- a/includes/resourceloader/DerivativeResourceLoaderContext.php
+++ b/includes/resourceloader/DerivativeResourceLoaderContext.php
@@ -28,32 +28,32 @@
  * @since 1.24
  */
 class DerivativeResourceLoaderContext extends ResourceLoaderContext {
+       const INHERIT_VALUE = -1;
 
        /**
         * @var ResourceLoaderContext
         */
        private $context;
-       protected $modules;
-       protected $language;
-       protected $direction;
-       protected $skin;
-       protected $user;
-       protected $debug;
-       protected $only;
-       protected $version;
-       protected $hash;
-       protected $raw;
+
+       protected $modules = self::INHERIT_VALUE;
+       protected $language = self::INHERIT_VALUE;
+       protected $direction = self::INHERIT_VALUE;
+       protected $skin = self::INHERIT_VALUE;
+       protected $user = self::INHERIT_VALUE;
+       protected $debug = self::INHERIT_VALUE;
+       protected $only = self::INHERIT_VALUE;
+       protected $version = self::INHERIT_VALUE;
+       protected $raw = self::INHERIT_VALUE;
 
        public function __construct( ResourceLoaderContext $context ) {
                $this->context = $context;
        }
 
        public function getModules() {
-               if ( !is_null( $this->modules ) ) {
-                       return $this->modules;
-               } else {
+               if ( $this->modules === self::INHERIT_VALUE ) {
                        return $this->context->getModules();
                }
+               return $this->modules;
        }
 
        /**
@@ -64,11 +64,10 @@ class DerivativeResourceLoaderContext extends ResourceLoaderContext {
        }
 
        public function getLanguage() {
-               if ( !is_null( $this->language ) ) {
-                       return $this->language;
-               } else {
+               if ( $this->language === self::INHERIT_VALUE ) {
                        return $this->context->getLanguage();
                }
+               return $this->language;
        }
 
        /**
@@ -76,16 +75,16 @@ class DerivativeResourceLoaderContext extends ResourceLoaderContext {
         */
        public function setLanguage( $language ) {
                $this->language = $language;
-               $this->direction = null; // Invalidate direction since it might be based on language
+               // Invalidate direction since it is based on language
+               $this->direction = self::INHERIT_VALUE;
                $this->hash = null;
        }
 
        public function getDirection() {
-               if ( !is_null( $this->direction ) ) {
-                       return $this->direction;
-               } else {
+               if ( $this->direction === self::INHERIT_VALUE ) {
                        return $this->context->getDirection();
                }
+               return $this->direction;
        }
 
        /**
@@ -97,11 +96,10 @@ class DerivativeResourceLoaderContext extends ResourceLoaderContext {
        }
 
        public function getSkin() {
-               if ( !is_null( $this->skin ) ) {
-                       return $this->skin;
-               } else {
+               if ( $this->skin === self::INHERIT_VALUE ) {
                        return $this->context->getSkin();
                }
+               return $this->skin;
        }
 
        /**
@@ -113,11 +111,10 @@ class DerivativeResourceLoaderContext extends ResourceLoaderContext {
        }
 
        public function getUser() {
-               if ( !is_null( $this->user ) ) {
-                       return $this->user;
-               } else {
+               if ( $this->user === self::INHERIT_VALUE ) {
                        return $this->context->getUser();
                }
+               return $this->user;
        }
 
        /**
@@ -130,11 +127,10 @@ class DerivativeResourceLoaderContext extends ResourceLoaderContext {
        }
 
        public function getDebug() {
-               if ( !is_null( $this->debug ) ) {
-                       return $this->debug;
-               } else {
+               if ( $this->debug === self::INHERIT_VALUE ) {
                        return $this->context->getDebug();
                }
+               return $this->debug;
        }
 
        /**
@@ -146,15 +142,14 @@ class DerivativeResourceLoaderContext extends ResourceLoaderContext {
        }
 
        public function getOnly() {
-               if ( !is_null( $this->only ) ) {
-                       return $this->only;
-               } else {
+               if ( $this->only === self::INHERIT_VALUE ) {
                        return $this->context->getOnly();
                }
+               return $this->only;
        }
 
        /**
-        * @param string $only
+        * @param string|null $only
         */
        public function setOnly( $only ) {
                $this->only = $only;
@@ -162,15 +157,14 @@ class DerivativeResourceLoaderContext extends ResourceLoaderContext {
        }
 
        public function getVersion() {
-               if ( !is_null( $this->version ) ) {
-                       return $this->version;
-               } else {
+               if ( $this->version === self::INHERIT_VALUE ) {
                        return $this->context->getVersion();
                }
+               return $this->version;
        }
 
        /**
-        * @param string $version
+        * @param string|null $version
         */
        public function setVersion( $version ) {
                $this->version = $version;
@@ -178,11 +172,10 @@ class DerivativeResourceLoaderContext extends ResourceLoaderContext {
        }
 
        public function getRaw() {
-               if ( !is_null( $this->raw ) ) {
-                       return $this->raw;
-               } else {
+               if ( $this->raw === self::INHERIT_VALUE ) {
                        return $this->context->getRaw();
                }
+               return $this->raw;
        }
 
        /**

diff --git a/includes/resourceloader/ResourceLoaderContext.php b/includes/resourceloader/ResourceLoaderContext.php
index cee7035..a22a437 100644 (file)
--- a/includes/resourceloader/ResourceLoaderContext.php
+++ b/includes/resourceloader/ResourceLoaderContext.php
@@ -59,24 +59,27 @@ class ResourceLoaderContext {
                $this->resourceLoader = $resourceLoader;
                $this->request = $request;
 
-               // Interpret request
                // List of modules
                $modules = $request->getVal( 'modules' );
                $this->modules = $modules ? self::expandModuleNames( $modules ) : array();
+
+
                // Various parameters
-               $this->skin = $request->getVal( 'skin' );
                $this->user = $request->getVal( 'user' );
                $this->debug = $request->getFuzzyBool(
-                       'debug', $resourceLoader->getConfig()->get( 'ResourceLoaderDebug' )
+                       'debug',
+                       $resourceLoader->getConfig()->get( 'ResourceLoaderDebug' )
                );
-               $this->only = $request->getVal( 'only' );
-               $this->version = $request->getVal( 'version' );
+               $this->only = $request->getVal( 'only', null );
+               $this->version = $request->getVal( 'version', null );
                $this->raw = $request->getFuzzyBool( 'raw' );
+
                // Image requests
                $this->image = $request->getVal( 'image' );
                $this->variant = $request->getVal( 'variant' );
                $this->format = $request->getVal( 'format' );
 
+               $this->skin = $request->getVal( 'skin' );
                $skinnames = Skin::getSkinNames();
                // If no skin is specified, or we don't recognize the skin, use the default skin
                if ( !$this->skin || !isset( $skinnames[$this->skin] ) ) {
@@ -177,7 +180,7 @@ class ResourceLoaderContext {
        }
 
        /**
-        * @return string|null
+        * @return string
         */
        public function getSkin() {
                return $this->skin;
@@ -305,21 +308,21 @@ class ResourceLoaderContext {
         * @return bool
         */
        public function shouldIncludeScripts() {
-               return is_null( $this->getOnly() ) || $this->getOnly() === 'scripts';
+               return $this->getOnly() === null || $this->getOnly() === 'scripts';
        }
 
        /**
         * @return bool
         */
        public function shouldIncludeStyles() {
-               return is_null( $this->getOnly() ) || $this->getOnly() === 'styles';
+               return $this->getOnly() === null || $this->getOnly() === 'styles';
        }
 
        /**
         * @return bool
         */
        public function shouldIncludeMessages() {
-               return is_null( $this->getOnly() );
+               return $this->getOnly() === null;
        }
 
        /**

diff --git a/tests/phpunit/includes/resourceloader/DerivativeResourceLoaderContextTest.php b/tests/phpunit/includes/resourceloader/DerivativeResourceLoaderContextTest.php
new file mode 100644 (file)
index 0000000..08c340e
--- /dev/null
+++ b/tests/phpunit/includes/resourceloader/DerivativeResourceLoaderContextTest.php
@@ -0,0 +1,75 @@
+<?php
+
+/**
+ * @group ResourceLoader
+ */
+class DerivativeResourceLoaderContextTest extends PHPUnit_Framework_TestCase {
+
+       protected static function getResourceLoaderContext() {
+               $resourceLoader = new ResourceLoader();
+               $request = new FauxRequest( array(
+                               'lang' => 'zh',
+                               'modules' => 'test.context',
+                               'only' => 'scripts',
+                               'skin' => 'fallback',
+                               'target' => 'test',
+               ) );
+               return new ResourceLoaderContext( $resourceLoader, $request );
+       }
+
+       public function testGet() {
+               $context = self::getResourceLoaderContext();
+               $derived = new DerivativeResourceLoaderContext( $context );
+
+               $this->assertEquals( $derived->getLanguage(), 'zh' );
+               $this->assertEquals( $derived->getModules(), array( 'test.context' ) );
+               $this->assertEquals( $derived->getOnly(), 'scripts' );
+               $this->assertEquals( $derived->getSkin(), 'fallback' );
+               $this->assertEquals( $derived->getHash(), 'zh|ltr|fallback||||||scripts|' );
+       }
+
+       public function testSetLanguage() {
+               $context = self::getResourceLoaderContext();
+               $derived = new DerivativeResourceLoaderContext( $context );
+
+               $derived->setLanguage( 'nl' );
+               $this->assertEquals( $derived->getLanguage(), 'nl' );
+       }
+
+       public function testSetModules() {
+               $context = self::getResourceLoaderContext();
+               $derived = new DerivativeResourceLoaderContext( $context );
+
+               $derived->setModules( array( 'test.override' ) );
+               $this->assertEquals( $derived->getModules(), array( 'test.override' ) );
+       }
+
+       public function testSetOnly() {
+               $context = self::getResourceLoaderContext();
+               $derived = new DerivativeResourceLoaderContext( $context );
+
+               $derived->setOnly( 'styles' );
+               $this->assertEquals( $derived->getOnly(), 'styles' );
+
+               $derived->setOnly( null );
+               $this->assertEquals( $derived->getOnly(), null );
+       }
+
+       public function testSetSkin() {
+               $context = self::getResourceLoaderContext();
+               $derived = new DerivativeResourceLoaderContext( $context );
+
+               $derived->setSkin( 'override' );
+               $this->assertEquals( $derived->getSkin(), 'override' );
+       }
+
+       public function testGetHash() {
+               $context = self::getResourceLoaderContext();
+               $derived = new DerivativeResourceLoaderContext( $context );
+
+               $derived->setLanguage( 'nl' );
+               // Assert that subclass is able to clear parent class "hash" member
+               $this->assertEquals( $derived->getHash(), 'nl|ltr|fallback||||||scripts|' );
+       }
+
+}