From 8af1c9503d59b0ef2fa4fef39b2fa8be2d9323c8 Mon Sep 17 00:00:00 2001 From: Tim Starling Date: Wed, 21 Aug 2013 15:35:40 +1000 Subject: [PATCH] Introduce WebRequest::getProtocol() The use of static server detection outside of its intended use case (i.e. at the start of DefaultSettings.php), for example in r93258, was an architectural error. Every other bit of information about the web request in non-setup code comes from non-static methods of WebRequest, which allows the request object to be meaningfully replaced or subclassed. The situation became increasingly ridiculous as more callers of WebRequest::detectProtocol() were introduced. Two of the callers were calling it non-statically! I suppose they had the right idea, in a way. Using a non-static call allows caching, which is a nice additional benefit. WebRequest::detectProtocolAndStdPort() was introduced in r93258 as part of the introduction of WebRequest::detectProtocol(). It was basically useless. Grep indicates there was only one caller in core and WMF deployed extensions, and it is patched here. Change-Id: Ia0a61e98fbff7a46ceaeebcb02236e5eac3df0e1 --- includes/GlobalFunctions.php | 4 +- includes/WebRequest.php | 40 +++++++++++++++---- includes/Wiki.php | 2 +- includes/specials/SpecialUserlogin.php | 6 +-- .../GlobalFunctions/wfExpandUrlTest.php | 10 +---- 5 files changed, 40 insertions(+), 22 deletions(-) diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php index ef0ec2c00c..5900d07423 100644 --- a/includes/GlobalFunctions.php +++ b/includes/GlobalFunctions.php @@ -495,7 +495,7 @@ function wfAppendQuery( $url, $query ) { * no valid URL can be constructed */ function wfExpandUrl( $url, $defaultProto = PROTO_CURRENT ) { - global $wgServer, $wgCanonicalServer, $wgInternalServer; + global $wgServer, $wgCanonicalServer, $wgInternalServer, $wgRequest; $serverUrl = $wgServer; if ( $defaultProto === PROTO_CANONICAL ) { $serverUrl = $wgCanonicalServer; @@ -505,7 +505,7 @@ function wfExpandUrl( $url, $defaultProto = PROTO_CURRENT ) { $serverUrl = $wgInternalServer; } if ( $defaultProto === PROTO_CURRENT ) { - $defaultProto = WebRequest::detectProtocol() . '://'; + $defaultProto = $wgRequest->getProtocol() . '://'; } // Analyze $serverUrl to obtain its protocol diff --git a/includes/WebRequest.php b/includes/WebRequest.php index b17cb9ec5e..462b312114 100644 --- a/includes/WebRequest.php +++ b/includes/WebRequest.php @@ -50,6 +50,12 @@ class WebRequest { */ private $ip; + /** + * Cached URL protocol + * @var string + */ + private $protocol; + public function __construct() { /// @todo FIXME: This preemptive de-quoting can interfere with other web libraries /// and increases our memory footprint. It would be cleaner to do on @@ -160,7 +166,8 @@ class WebRequest { * @return string */ public static function detectServer() { - list( $proto, $stdPort ) = self::detectProtocolAndStdPort(); + $proto = self::detectProtocol(); + $stdPort = $proto === 'https' ? 443 : 80; $varNames = array( 'HTTP_HOST', 'SERVER_NAME', 'HOSTNAME', 'SERVER_ADDR' ); $host = 'localhost'; @@ -189,25 +196,32 @@ class WebRequest { } /** + * Detect the protocol from $_SERVER. + * This is for use prior to Setup.php, when no WebRequest object is available. + * At other times, use the non-static function getProtocol(). + * * @return array */ - public static function detectProtocolAndStdPort() { + public static function detectProtocol() { if ( ( isset( $_SERVER['HTTPS'] ) && $_SERVER['HTTPS'] == 'on' ) || ( isset( $_SERVER['HTTP_X_FORWARDED_PROTO'] ) && $_SERVER['HTTP_X_FORWARDED_PROTO'] == 'https' ) ) { - $arr = array( 'https', 443 ); + return 'https'; } else { - $arr = array( 'http', 80 ); + return 'http'; } return $arr; } /** + * Get the current URL protocol (http or https) * @return string */ - public static function detectProtocol() { - list( $proto, ) = self::detectProtocolAndStdPort(); - return $proto; + public function getProtocol() { + if ( $this->protocol === null ) { + $this->protocol = self::detectProtocol(); + } + return $this->protocol; } /** @@ -1312,9 +1326,10 @@ class FauxRequest extends WebRequest { * fake GET/POST values * @param bool $wasPosted whether to treat the data as POST * @param $session Mixed: session array or null + * @param string $protocol 'http' or 'https' * @throws MWException */ - public function __construct( $data = array(), $wasPosted = false, $session = null ) { + public function __construct( $data = array(), $wasPosted = false, $session = null, $protocol = 'http' ) { if ( is_array( $data ) ) { $this->data = $data; } else { @@ -1324,6 +1339,7 @@ class FauxRequest extends WebRequest { if ( $session ) { $this->session = $session; } + $this->protocol = $protocol; } /** @@ -1385,6 +1401,10 @@ class FauxRequest extends WebRequest { $this->notImplemented( __METHOD__ ); } + public function getProtocol() { + return $this->protocol; + } + /** * @param string $name The name of the header to get (case insensitive). * @return bool|string @@ -1524,4 +1544,8 @@ class DerivativeRequest extends FauxRequest { public function getIP() { return $this->base->getIP(); } + + public function getProtocol() { + return $this->base->getProtocol(); + } } diff --git a/includes/Wiki.php b/includes/Wiki.php index a690176d5d..50bba7b9fc 100644 --- a/includes/Wiki.php +++ b/includes/Wiki.php @@ -540,7 +540,7 @@ class MediaWiki { && $this->context->getUser()->requiresHTTPS() ) ) && - $request->detectProtocol() == 'http' + $request->getProtocol() == 'http' ) { $oldUrl = $request->getFullRequestURL(); $redirUrl = str_replace( 'http://', 'https://', $oldUrl ); diff --git a/includes/specials/SpecialUserlogin.php b/includes/specials/SpecialUserlogin.php index 5ac3e6546e..f40966c2cc 100644 --- a/includes/specials/SpecialUserlogin.php +++ b/includes/specials/SpecialUserlogin.php @@ -106,7 +106,7 @@ class LoginForm extends SpecialPage { $this->mAction = $request->getVal( 'action' ); $this->mRemember = $request->getCheck( 'wpRemember' ); $this->mFromHTTP = $request->getBool( 'fromhttp', false ); - $this->mStickHTTPS = ( !$this->mFromHTTP && $request->detectProtocol() === 'https' ) || $request->getBool( 'wpForceHttps', false ); + $this->mStickHTTPS = ( !$this->mFromHTTP && $request->getProtocol() === 'https' ) || $request->getBool( 'wpForceHttps', false ); $this->mLanguage = $request->getText( 'uselang' ); $this->mSkipCookieCheck = $request->getCheck( 'wpSkipCookieCheck' ); $this->mToken = ( $this->mType == 'signup' ) ? $request->getVal( 'wpCreateaccountToken' ) : $request->getVal( 'wpLoginToken' ); @@ -168,7 +168,7 @@ class LoginForm extends SpecialPage { // If logging in and not on HTTPS, either redirect to it or offer a link. global $wgSecureLogin; - if ( WebRequest::detectProtocol() !== 'https' ) { + if ( $this->mRequest->getProtocol() !== 'https' ) { $title = $this->getFullTitle(); $query = array( 'returnto' => $this->mReturnTo, @@ -1196,7 +1196,7 @@ class LoginForm extends SpecialPage { $template->set( 'secureLoginUrl', $this->mSecureLoginUrl ); // Use loginend-https for HTTPS requests if it's not blank, loginend otherwise // Ditto for signupend. New forms use neither. - $usingHTTPS = WebRequest::detectProtocol() == 'https'; + $usingHTTPS = $this->mRequest->getProtocol() == 'https'; $loginendHTTPS = $this->msg( 'loginend-https' ); $signupendHTTPS = $this->msg( 'signupend-https' ); if ( $usingHTTPS && !$loginendHTTPS->isBlank() ) { diff --git a/tests/phpunit/includes/GlobalFunctions/wfExpandUrlTest.php b/tests/phpunit/includes/GlobalFunctions/wfExpandUrlTest.php index 41230a1ea3..b06f3d29a5 100644 --- a/tests/phpunit/includes/GlobalFunctions/wfExpandUrlTest.php +++ b/tests/phpunit/includes/GlobalFunctions/wfExpandUrlTest.php @@ -7,19 +7,13 @@ class WfExpandUrlTest extends MediaWikiTestCase { * @dataProvider provideExpandableUrls */ public function testWfExpandUrl( $fullUrl, $shortUrl, $defaultProto, $server, $canServer, $httpsMode, $message ) { - // Fake $wgServer and $wgCanonicalServer + // Fake $wgServer, $wgCanonicalServer and $wgRequest->getProtocol() $this->setMwGlobals( array( 'wgServer' => $server, 'wgCanonicalServer' => $canServer, + 'wgRequest' => new FauxRequest( array(), false, null, $httpsMode ? 'https' : 'http' ) ) ); - // Fake $_SERVER['HTTPS'] if needed - if ( $httpsMode ) { - $_SERVER['HTTPS'] = 'on'; - } else { - unset( $_SERVER['HTTPS'] ); - } - $this->assertEquals( $fullUrl, wfExpandUrl( $shortUrl, $defaultProto ), $message ); } -- 2.20.1