r93258 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r93257‎ | r93258 | r93259 >
Date:08:21, 27 July 2011
Author:catrope
Status:ok (Comments)
Tags:todo 
Comment:
(bug NNNNN) Rewrite most of wfExpandUrl() to handle protocol-relative URLs properly and more flexibly
* Fix a bug in rNNNNN where URLs like '/wiki/Foo' weren't expanded completely if $wgServer was protocol-relative. This caused bug NNNNN.
* Add an optional second parameter to wfExpandUrl(), which takes one the PROT_* constants. This allows the caller to determine which protocol should be used if the given URL is protocol-relative, or the given URL is domain-relative but $wgServer is protocol-relative. The options are PROT_HTTP (use http), PROT_HTTPS (use https), PROT_RELATIVE (keep the URL as protocol-relative), and PROT_CURRENT (use http if the current request is http, or https if the current request is https; this is the default).
* Factor the protocol/port detection part of WebRequest::detectServer() out into detectProtocolAndStdPort(), and add detectProtocol() as a wrapper. The latter is used by wfExpandUrl() in PROT_CURRENT mode.
* Rewrite the test suite to test all possible combinations of $wgServer, $defaultProto, $url and HTTP/HTTPS mode. This means the test suite now has 120 test cases rather than 4.
Modified paths:
  • /trunk/phase3/includes/GlobalFunctions.php (modified) (history)
  • /trunk/phase3/includes/WebRequest.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/GlobalFunctions/wfExpandUrl.php (modified) (history)

Diff [purge]

Index: trunk/phase3/tests/phpunit/includes/GlobalFunctions/wfExpandUrl.php
——@@ -5,8 +5,23 @@
66
77 class wfExpandUrl extends MediaWikiTestCase {
88 /** @dataProvider provideExpandableUrls */
9 - public function testWfExpandUrl( $fullUrl, $shortUrl, $message ) {
10 - $this->assertEquals( $fullUrl, wfExpandUrl( $shortUrl ), $message );
 9+ public function testWfExpandUrl( $fullUrl, $shortUrl, $defaultProto, $server, $httpsMode, $message ) {
 10+ // Fake $wgServer
 11+ global $wgServer;
 12+ $oldServer = $wgServer;
 13+ $wgServer = $server;
 14+
 15+ // Fake $_SERVER['HTTPS'] if needed
 16+ if ( $httpsMode ) {
 17+ $_SERVER['HTTPS'] = 'on';
 18+ } else {
 19+ unset( $_SERVER['HTTPS'] );
 20+ }
 21+
 22+ $this->assertEquals( $fullUrl, wfExpandUrl( $shortUrl, $defaultProto ), $message );
 23+
 24+ // Restore $wgServer
 25+ $wgServer = $oldServer;
1126 }
1227
1328 /**
——@@ -15,13 +30,37 @@
1631 * @return array
1732 */
1833 public function provideExpandableUrls() {
19 - global $wgServer;
20 - return array(
21 - array( "$wgServer/wiki/FooBar", '/wiki/FooBar', 'Testing expanding URL beginning with /' ),
22 - array( 'http://example.com', 'http://example.com', 'Testing fully qualified http URLs (no need to expand)' ),
23 - array( 'https://example.com', 'https://example.com', 'Testing fully qualified https URLs (no need to expand)' ),
24 - # Would be nice to support this, see fixme on wfExpandUrl()
25 - array( "wiki/FooBar", 'wiki/FooBar', "Test non-expandable relative URLs" ),
26 - );
 34+ $modes = array( 'http', 'https' );
 35+ $servers = array( 'http' => 'http://example.com', 'https' => 'https://example.com', 'protocol-relative' => '//example.com' );
 36+ $defaultProtos = array( 'http' => PROT_HTTP, 'https' => PROT_HTTPS, 'protocol-relative' => PROT_RELATIVE, 'current' => PROT_CURRENT );
 37+
 38+ $retval = array();
 39+ foreach ( $modes as $mode ) {
 40+ $httpsMode = $mode == 'https';
 41+ foreach ( $servers as $serverDesc => $server ) {
 42+ foreach ( $defaultProtos as $protoDesc => $defaultProto ) {
 43+ $retval[] = array( 'http://example.com', 'http://example.com', $defaultProto, $server, $httpsMode, "Testing fully qualified http URLs (no need to expand) (defaultProto: $protoDesc , wgServer: $server, current request protocol: $mode )" );
 44+ $retval[] = array( 'https://example.com', 'https://example.com', $defaultProto, $server, $httpsMode, "Testing fully qualified https URLs (no need to expand) (defaultProto: $protoDesc , wgServer: $server, current request protocol: $mode )" );
 45+ # Would be nice to support this, see fixme on wfExpandUrl()
 46+ $retval[] = array( "wiki/FooBar", 'wiki/FooBar', $defaultProto, $server, $httpsMode, "Test non-expandable relative URLs (defaultProto: $protoDesc , wgServer: $server, current request protocol: $mode )" );
 47+
 48+ // Determine expected protocol
 49+ $p = $protoDesc . ':'; // default case
 50+ if ( $protoDesc == 'protocol-relative' ) {
 51+ $p = '';
 52+ } else if ( $protoDesc == 'current' ) {
 53+ $p = "$mode:";
 54+ } else {
 55+ $p = $protoDesc . ':';
 56+ }
 57+ // Determine expected server name
 58+ $srv = $serverDesc == 'protocol-relative' ? $p . $server : $server;
 59+
 60+ $retval[] = array( "$p//wikipedia.org", '//wikipedia.org', $defaultProto, $server, $httpsMode, "Test protocol-relative URL (defaultProto: $protoDesc, wgServer: $server, current request protocol: $mode )" );
 61+ $retval[] = array( "$srv/wiki/FooBar", '/wiki/FooBar', $defaultProto, $server, $httpsMode, "Testing expanding URL beginning with / (defaultProto: $protoDesc , wgServer: $server, current request protocol: $mode )" );
 62+ }
 63+ }
 64+ }
 65+ return $retval;
2766 }
2867 }
Index: trunk/phase3/includes/GlobalFunctions.php
——@@ -428,24 +428,44 @@
429429 return $url;
430430 }
431431
 432+define( 'PROT_HTTP', 'http://' );
 433+define( 'PROT_HTTPS', 'https://' );
 434+define( 'PROT_RELATIVE', '//' );
 435+define( 'PROT_CURRENT', null );
 436+
432437 /**
433438 * Expand a potentially local URL to a fully-qualified URL. Assumes $wgServer
434439 * is correct.
 440+ *
 441+ * The meaning of the PROT_* constants is as follows:
 442+ * PROT_HTTP: Output a URL starting with http://
 443+ * PROT_HTTPS: Output a URL starting with https://
 444+ * PROT_RELATIVE: Output a URL starting with // (protocol-relative URL)
 445+ * PROT_CURRENT: Output a URL starting with either http:// or https:// , depending on which protocol was used for the current incoming request
435446 *
436447 * @todo this won't work with current-path-relative URLs
437448 * like "subdir/foo.html", etc.
438449 *
439450 * @param $url String: either fully-qualified or a local path + query
 451+ * @param $defaultProto Mixed: one of the PROT_* constants. Determines the protocol to use if $url or $wgServer is protocol-relative
440452 * @return string Fully-qualified URL
441453 */
442 -function wfExpandUrl( $url ) {
 454+function wfExpandUrl( $url, $defaultProto = PROT_CURRENT ) {
443455 global $wgServer;
 456+ if ( $defaultProto === PROT_CURRENT ) {
 457+ $defaultProto = WebRequest::detectProtocol() . '://';
 458+ }
 459+
 460+ // Analyze $wgServer to obtain its protocol
 461+ $bits = wfParseUrl( $wgServer );
 462+ $serverHasProto = $bits && $bits['scheme'] != '';
 463+ $defaultProtoWithoutSlashes = substr( $defaultProto, 0, -2 );
 464+
444465 if( substr( $url, 0, 2 ) == '//' ) {
445 - $bits = wfParseUrl( $wgServer );
446 - $scheme = $bits && $bits['scheme'] !== '' ? $bits['scheme'] : 'http';
447 - return $scheme . ':' . $url;
 466+ return $defaultProtoWithoutSlashes . $url;
448467 } elseif( substr( $url, 0, 1 ) == '/' ) {
449 - return $wgServer . $url;
 468+ // If $wgServer is protocol-relative, prepend $defaultProtoWithoutSlashes, otherwise leave it alone
 469+ return ( $serverHasProto ? '' : $defaultProtoWithoutSlashes ) . $wgServer . $url;
450470 } else {
451471 return $url;
452472 }
Index: trunk/phase3/includes/WebRequest.php
——@@ -131,13 +131,7 @@
132132 * @return string
133133 */
134134 public static function detectServer() {
135 - if ( isset( $_SERVER['HTTPS'] ) && $_SERVER['HTTPS'] == 'on') {
136 - $proto = 'https';
137 - $stdPort = 443;
138 - } else {
139 - $proto = 'http';
140 - $stdPort = 80;
141 - }
 135+ list( $proto, $stdPort ) = self::detectProtocolAndStdPort();
142136
143137 $varNames = array( 'HTTP_HOST', 'SERVER_NAME', 'HOSTNAME', 'SERVER_ADDR' );
144138 $host = 'localhost';
——@@ -164,6 +158,15 @@
165159
166160 return $proto . '://' . IP::combineHostAndPort( $host, $port, $stdPort );
167161 }
 162+
 163+ public static function detectProtocolAndStdPort() {
 164+ return ( isset( $_SERVER['HTTPS'] ) && $_SERVER['HTTPS'] == 'on' ) ? array( 'https', 443 ) : array( 'http', 80 );
 165+ }
 166+
 167+ public static function detectProtocol() {
 168+ list( $proto, $stdPort ) = self::detectProtocolAndStdPort();
 169+ return $proto;
 170+ }
168171
169172 /**
170173 * Check for title, action, and/or variant data in the URL

Follow-up revisions

RevisionCommit summaryAuthorDate
r93266Rename PROT_* constants to PROTO_*...platonides14:06, 27 July 2011
r93847Follow-up r93258, r93266, r93266: Move the defines to Defines.phpplatonides18:25, 3 August 2011
r938861.17wmf1: Somewhat creative MFT of r93258, because the diff of that rev does ...catrope13:38, 4 August 2011
r94472MFT to REL1_18:...hashar19:43, 14 August 2011

Comments

#Comment by MZMcBride (talk | contribs)   08:28, 27 July 2011

Your commit message contains some placeholders. ;-)

#Comment by Catrope (talk | contribs)   09:16, 27 July 2011

Whoops. This is why I had doubts about committing at 2am.

The first placeholder refers to a non-existent bug. The revision is r92028 (associated it too), and the second bug is bug 29981.

#Comment by Bryan (talk | contribs)   07:00, 4 August 2011

How will this work for servers that are exclusively accessible over HTTPS? If wfExpandUrl( ..., PROTO_HTTP ) is called, will that yield an invalid HTTP url?

#Comment by Catrope (talk | contribs)   13:10, 10 August 2011

No. The PROTO_ parameter is only taken into account if the first parameter is a protocol-relative URL, or if the first parameter is a relative URL and $wgServer is protocol-relative. In other words, only if after the traditional expansion steps the return value would be protocol-relative, is the PROTO_ parameter consulted.

#Comment by Aaron Schulz (talk | contribs)   19:54, 26 September 2011

Heh, with current code: $wgServer = '//localhost'; var_dump( wfExpandUrl( '/wiki/Sp%C3%A9cial:Pages_sp%C3%A9ciales', 35362 ) );

Gives: string '353//localhost/wiki/Sp%C3%A9cial:Pages_sp%C3%A9ciales' (length=53)

$defaultProto only gets set to something sane of one of the comparisons matches. Maybe a little sanity check could be added?

Status & tagging log