WebRequest: Change getFullRequestURL() to use local getProtocol()

Previously it relied on wfGetServerUrl to decide the url,
which passed PROTO_CURRENT on to wfExpandUrl(), when then uses
global $wgRequest and checks getProtocol() on that.

For typical web requests, these would be the same object by
reference, but the needless indirection makes the code harder
to reason about and harder to test.

Instead, check this locally and pass on an explicit HTTP or HTTPS
to wfGetServerUrl/wfExpandUrl.

Change-Id: I797bd2f909625536c3af36f015fb2e94bf922ba9

diff --git a/includes/FauxRequest.php b/includes/FauxRequest.php
index 906e5e2..ecbc6e3 100644 (file)
--- a/includes/FauxRequest.php
+++ b/includes/FauxRequest.php
@@ -170,17 +170,6 @@ class FauxRequest extends WebRequest {
                return $this->requestUrl;
        }
 
-       public function getFullRequestURL() {
-               // Pass an explicit PROTO constant instead of PROTO_CURRENT so that we
-               // do not rely on state from the global $wgRequest object (which it would,
-               // via wfGetServerUrl/wfExpandUrl/$wgRequest->protocol).
-               if ( $this->protocol === 'http' ) {
-                       return wfGetServerUrl( PROTO_HTTP ) . $this->getRequestURL();
-               } else {
-                       return wfGetServerUrl( PROTO_HTTPS ) . $this->getRequestURL();
-               }
-       }
-
        public function getProtocol() {
                return $this->protocol;
        }

diff --git a/includes/WebRequest.php b/includes/WebRequest.php
index 2a03d2d..7da092f 100644 (file)
--- a/includes/WebRequest.php
+++ b/includes/WebRequest.php
@@ -849,12 +849,19 @@ class WebRequest {
         * in HTML or other output.
         *
         * If $wgServer is protocol-relative, this will return a fully
-        * qualified URL with the protocol that was used for this request.
+        * qualified URL with the protocol of this request object.
         *
         * @return string
         */
        public function getFullRequestURL() {
-               return wfGetServerUrl( PROTO_CURRENT ) . $this->getRequestURL();
+               // Pass an explicit PROTO constant instead of PROTO_CURRENT so that we
+               // do not rely on state from the global $wgRequest object (which it would,
+               // via wfGetServerUrl/wfExpandUrl/$wgRequest->protocol).
+               if ( $this->getProtocol() === 'http' ) {
+                       return wfGetServerUrl( PROTO_HTTP ) . $this->getRequestURL();
+               } else {
+                       return wfGetServerUrl( PROTO_HTTPS ) . $this->getRequestURL();
+               }
        }
 
        /**

diff --git a/tests/phpunit/includes/WebRequestTest.php b/tests/phpunit/includes/WebRequestTest.php
index 07c307e..cda5660 100644 (file)
--- a/tests/phpunit/includes/WebRequestTest.php
+++ b/tests/phpunit/includes/WebRequestTest.php
@@ -4,16 +4,19 @@
  * @group WebRequest
  */
 class WebRequestTest extends MediaWikiTestCase {
-       protected $oldServer;
 
        protected function setUp() {
                parent::setUp();
 
                $this->oldServer = $_SERVER;
+               $this->oldWgRequest = $GLOBALS['wgRequest'];
+               $this->oldWgServer = $GLOBALS['wgServer'];
        }
 
        protected function tearDown() {
                $_SERVER = $this->oldServer;
+               $GLOBALS['wgRequest'] = $this->oldWgRequest;
+               $GLOBALS['wgServer'] = $this->oldWgServer;
 
                parent::tearDown();
        }
@@ -368,6 +371,22 @@ class WebRequestTest extends MediaWikiTestCase {
                $this->assertSame( [ 'x' ], $req->getValueNames( [ 'y' ] ), 'Exclude keys' );
        }
 
+       /**
+        * @covers WebRequest
+        */
+       public function testGetFullRequestURL() {
+               // Stub this for wfGetServerUrl()
+               $GLOBALS['wgServer'] = '//wiki.test';
+               $req = $this->getMock( WebRequest::class, [ 'getRequestURL', 'getProtocol' ] );
+               $req->method( 'getRequestURL' )->willReturn( '/path' );
+               $req->method( 'getProtocol' )->willReturn( 'https' );
+
+               $this->assertSame(
+                       'https://wiki.test/path',
+                       $req->getFullRequestURL()
+               );
+       }
+
        /**
         * @dataProvider provideGetIP
         * @covers WebRequest::getIP