context: Make the getSkin() fallback logic more explicit

Previously the first two post-hook checks where mutually exclusive
and the fallback was generic (not conditional) which means we
would also allow the first two conditions to succeed in a way that
leaves $this->skin as null.

Aside from that not being possible right now, it also duplicates a
null check in two places, which isn't ideal.

Codify that the first two blocks always result in $this->skin
being assigned non-null by using an 'else' branch instead of another
generic check.

Change-Id: I7986684bf1bb2daad3aaddfbcd2ca02e652f78c2

diff --git a/includes/context/RequestContext.php b/includes/context/RequestContext.php
index 4393abb..afc7045 100644 (file)
--- a/includes/context/RequestContext.php
+++ b/includes/context/RequestContext.php
@@ -371,33 +371,27 @@ class RequestContext implements IContextSource, MutableContext {
                        Hooks::run( 'RequestContextCreateSkin', [ $this, &$skin ] );
                        $factory = MediaWikiServices::getInstance()->getSkinFactory();
 
-                       // If the hook worked try to set a skin from it
                        if ( $skin instanceof Skin ) {
+                               // The hook provided a skin object
                                $this->skin = $skin;
                        } elseif ( is_string( $skin ) ) {
+                               // The hook provided a skin name
                                // Normalize the key, just in case the hook did something weird.
                                $normalized = Skin::normalizeKey( $skin );
                                $this->skin = $factory->makeSkin( $normalized );
-                       }
-
-                       // If this is still null (the hook didn't run or didn't work)
-                       // then go through the normal processing to load a skin
-                       if ( $this->skin === null ) {
+                       } else {
+                               // No hook override, go through normal processing
                                if ( !in_array( 'skin', $this->getConfig()->get( 'HiddenPrefs' ) ) ) {
-                                       # get the user skin
                                        $userSkin = $this->getUser()->getOption( 'skin' );
                                        $userSkin = $this->getRequest()->getVal( 'useskin', $userSkin );
                                } else {
-                                       # if we're not allowing users to override, then use the default
                                        $userSkin = $this->getConfig()->get( 'DefaultSkin' );
                                }
 
-                               // Normalize the key in case the user is passing gibberish
-                               // or has old preferences (T71566).
+                               // Normalize the key in case the user is passing gibberish query params
+                               // or has old user preferences (T71566).
+                               // Skin::normalizeKey will also validate it, so makeSkin() won't throw.
                                $normalized = Skin::normalizeKey( $userSkin );
-
-                               // Skin::normalizeKey will also validate it, so
-                               // this won't throw an exception
                                $this->skin = $factory->makeSkin( $normalized );
                        }