From: Timo Tijhof Date: Wed, 10 Jun 2015 17:43:32 +0000 (+0100) Subject: resourceloader: Use -1 instead of null in DerivativeResourceLoaderContext X-Git-Tag: 1.31.0-rc.0~11073^2 X-Git-Url: http://git.cyclocoop.org/%7B%24www_url%7Dadmin/compta/exercices/modifier.php?a=commitdiff_plain;h=d20583dd19502c8088f7522c07dde42d0846d6b1;p=lhc%2Fweb%2Fwiklou.git 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 5784f2a0b0..85d5434089 100644 --- 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 cee70350c2..a22a437829 100644 --- 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 index 0000000000..08c340e19c --- /dev/null +++ b/tests/phpunit/includes/resourceloader/DerivativeResourceLoaderContextTest.php @@ -0,0 +1,75 @@ + '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|' ); + } + +}