r112752 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r112751‎ | r112752 | r112753 >
Date:01:19, 1 March 2012
Author:awjrichards
Status:ok (Comments)
Tags:
Comment:
Refactored checks to see if we should be displaying the MobileFrontend view and removed dependency on looking for /only/ x-device headers so we handle previously unhandled cases where a user is using useformat=mobile; As part of refactor added methods to check if a user is coming from a mobile device (denoted by presense of x-device headers) or if a user is coming from a 'faux' mobile device (when using something like useformat=mobile); Added query string handling for mobile URL construction; Added corresponding tests; Added some additional cleanup to the tearDown routine in tests to unset x-device headers and refresh the value of ExtMobileFrontend:: to its default state.
Modified paths:
  • /trunk/extensions/MobileFrontend/MobileFrontend.body.php (modified) (history)
  • /trunk/extensions/MobileFrontend/tests/MobileFrontendTest.php (modified) (history)

Diff [purge]

Index: trunk/extensions/MobileFrontend/tests/MobileFrontendTest.php
——@@ -26,6 +26,8 @@
2727 protected function tearDown() {
2828 global $wgExtMobileFrontend;
2929 unset( $wgExtMobileFrontend );
 30+ unset( $_SERVER[ 'HTTP_X_DEVICE' ] );
 31+ ExtMobileFrontend::$useFormat = null;
3032 parent::tearDown();
3133 }
3234
——@@ -102,4 +104,203 @@
103105 $updateMobileUrlHost->invokeArgs( $wgExtMobileFrontend, array( &$parsedUrl ) );
104106 $this->assertEquals( "http://en.wikipedia.org/wiki/mobile/Gustavus_Airport", wfAssembleUrl( $parsedUrl ) );
105107 }
 108+
 109+ /**
 110+ * @dataProvider updateMobileUrlQueryStringProvider
 111+ */
 112+ public function testUpdateMobileUrlQueryString( $assert, $useFormat ) {
 113+ global $wgRequest, $wgExtMobileFrontend;
 114+
 115+ $testMethod = ( $assert ) ? 'assertTrue' : 'assertFalse';
 116+ $url = 'http://en.wikipedia.org/wiki/Article/?something=bananas';
 117+ if ( !empty( $useFormat ) ) $url .= "&useformat=" . $useFormat;
 118+ ExtMobileFrontend::$useFormat = $useFormat;
 119+
 120+ $parsedUrl = wfParseUrl( $url );
 121+
 122+ $updateMobileUrlQueryString = self::getMethod( 'updateMobileUrlQueryString' );
 123+ $updateMobileUrlQueryString->invokeArgs( $wgExtMobileFrontend, array( &$parsedUrl) );
 124+
 125+ $targetUrl = wfAssembleUrl( $parsedUrl );
 126+ $this->$testMethod( $url == $targetUrl, $targetUrl );
 127+ }
 128+
 129+ public function updateMobileUrlQueryStringProvider() {
 130+ return array(
 131+ array( true, 'mobile' ),
 132+ array( true, 'mobile-wap' ),
 133+ array( true, '' ),
 134+ );
 135+ }
 136+
 137+ /**
 138+ * @dataProvider isMobileDeviceProvider
 139+ */
 140+ public function testIsMobileDevice( $isDevice, $msg, $xDevice = null ) {
 141+ global $wgReqeust, $wgExtMobileFrontend;
 142+ $isMobileDevice = self::getMethod( 'isMobileDevice' );
 143+
 144+ $testMethod = ( $isDevice ) ? 'assertTrue' : 'assertFalse';
 145+
 146+ if ( !is_null( $xDevice )) {
 147+ $_SERVER[ 'HTTP_X_DEVICE' ] = $xDevice;
 148+ }
 149+
 150+ $this->$testMethod( $isMobileDevice->invokeArgs( $wgExtMobileFrontend, array() ), $msg );
 151+ }
 152+
 153+ public function isMobileDeviceProvider() {
 154+ return array(
 155+ array( false, 'Nothing set' ),
 156+ array( true, 'HTTP_X_DEVICE = webkit', 'webkit' ),
 157+ );
 158+ }
 159+
 160+ /**
 161+ * @dataProvider isFauxMobileDeviceProvider
 162+ */
 163+ public function testIsFauxMobileDevice( $isFauxDevice, $msg, $useformat=null ) {
 164+ global $wgRequest, $wgExtMobileFrontend;
 165+ $isFauxMobileDevice = self::getMethod( 'isFauxMobileDevice' );
 166+
 167+ $testMethod = ( $isFauxDevice ) ? 'assertTrue' : 'assertFalse';
 168+
 169+ ExtMobileFrontend::$useFormat = $useformat;
 170+ $this->$testMethod( $isFauxMobileDevice->invokeArgs( $wgExtMobileFrontend, array() ), $msg );
 171+ }
 172+
 173+ public function isFauxMobileDeviceProvider() {
 174+ return array(
 175+ array( false, 'Nothing set' ),
 176+ array( true, 'useformat=mobile', 'mobile' ),
 177+ array( true, 'useformat=mobile-wap', 'mobile-wap' ),
 178+ array( false, 'useformat=yourmom', 'yourmom' ),
 179+ );
 180+ }
 181+
 182+ /**
 183+ * @dataProvider shouldDisplayMobileViewProvider
 184+ */
 185+ public function testShouldDisplayMobileView( $shouldDisplay, $xDevice=null, $requestVal=array(), $msg=null ) {
 186+ global $wgRequest, $wgExtMobileFrontend;
 187+ $shouldDisplayMobileView = self::getMethod( 'shouldDisplayMobileView' );
 188+
 189+ $testMethod = ( $shouldDisplay ) ? 'assertTrue' : 'assertFalse';
 190+
 191+ if ( count( $requestVal )) {
 192+ foreach ( $requestVal as $key => $val ) {
 193+ if ( $key == 'useformat' ) {
 194+ ExtMobileFrontend::$useFormat = $val;
 195+ } else {
 196+ $wgRequest->setVal( $key, $val );
 197+ }
 198+ }
 199+ }
 200+
 201+ if ( !is_null( $xDevice )) {
 202+ $_SERVER[ 'HTTP_X_DEVICE' ] = $xDevice;
 203+ }
 204+
 205+ $this->$testMethod( $shouldDisplayMobileView->invokeArgs( $wgExtMobileFrontend, array() ), $msg );
 206+
 207+ // clean up
 208+ if ( count( $requestVal )) {
 209+ foreach ( $requestVal as $key => $val ) {
 210+ if ( $key == 'useformat' ) {
 211+ continue;
 212+ } else {
 213+ $wgRequest->unsetVal( $key );
 214+ }
 215+ }
 216+ }
 217+ }
 218+
 219+ public function shouldDisplayMobileViewProvider() {
 220+ return array(
 221+ array( false, null, array() ),
 222+ array( true, 'webkit', array() ),
 223+ array( false, 'webkit', array( 'action' => 'edit' ) ),
 224+ array( false, 'webkit', array( 'mobileaction' => 'view_normal_site' ) ),
 225+ array( true, null, array( 'useformat' => 'mobile-wap' ) ),
 226+ array( false, null, array( 'useformat' => 'mobile-wap', 'action' => 'edit' ) ),
 227+ array( false, null, array( 'useformat' => 'mobile-wap', 'action' => 'history' ) ),
 228+ array( false, null, array( 'useformat' => 'mobile-wap', 'mobileaction' => 'view_normal_site' ) ),
 229+ array( true, null, array( 'useformat' => 'mobile' ) ),
 230+ array( false, null, array( 'useformat' => 'mobile', 'action' => 'edit' ) ),
 231+ array( false, null, array( 'useformat' => 'mobile', 'action' => 'history' ) ),
 232+ array( false, null, array( 'useformat' => 'mobile', 'mobileaction' => 'view_normal_site' ) ),
 233+ );
 234+ }
 235+
 236+ /**
 237+ * @dataProvider getXDeviceProvider
 238+ */
 239+ public function testGetXDevice( $xDevice=null ) {
 240+ global $wgExtMobileFrontend;
 241+ if ( is_null( $xDevice ) ) {
 242+ $assert = '';
 243+ if ( isset( $_SERVER[ 'HTTP_X_DEVICE' ] ) ) {
 244+ unset( $_SERVER[ 'HTTP_X_DEVICE' ] );
 245+ }
 246+ } else {
 247+ $_SERVER[ 'HTTP_X_DEVICE' ] = $xDevice;
 248+ $assert = $xDevice;
 249+ }
 250+ $this->assertEquals( $assert, $wgExtMobileFrontend->getXDevice() );
 251+ }
 252+
 253+ public function getXDeviceProvider() {
 254+ return array(
 255+ array( 'webkit' ),
 256+ array( null ),
 257+ );
 258+ }
 259+
 260+ /**
 261+ * @dataProvider getMobileActionProvider
 262+ */
 263+ public function testGetMobileAction( $mobileaction=null ) {
 264+ global $wgRequest, $wgExtMobileFrontend;
 265+
 266+ if ( is_null( $mobileaction )) {
 267+ $assert = '';
 268+ $wgRequest->unsetVal( 'mobileaction' );
 269+ } else {
 270+ $wgRequest->setVal( 'mobileaction', $mobileaction );
 271+ $assert = $mobileaction;
 272+ }
 273+
 274+ $this->assertEquals( $assert, $wgExtMobileFrontend->getMobileAction() );
 275+ }
 276+
 277+ public function getMobileActionProvider() {
 278+ return array(
 279+ array( null ),
 280+ array( 'view_normal_site' ),
 281+ );
 282+ }
 283+
 284+ /**
 285+ * @dataProvider getActionProvider
 286+ */
 287+ public function testGetAction( $action=null ) {
 288+ global $wgRequest, $wgExtMobileFrontend;
 289+
 290+ if ( is_null( $action )) {
 291+ $assert = '';
 292+ $wgRequest->unsetVal( 'action' );
 293+ } else {
 294+ $wgRequest->setVal( 'action', $action );
 295+ $assert = $action;
 296+ }
 297+
 298+ $this->assertEquals( $assert, $wgExtMobileFrontend->getAction() );
 299+ }
 300+
 301+ public function getActionProvider() {
 302+ return array(
 303+ array( null ),
 304+ array( 'edit' ),
 305+ );
 306+ }
106307 }
Index: trunk/extensions/MobileFrontend/MobileFrontend.body.php
——@@ -43,6 +43,21 @@
4444 public static $logoutHtml;
4545 public static $loginHtml;
4646 public static $zeroRatedBanner;
 47+
 48+ /**
 49+ * @var string xDevice header information
 50+ */
 51+ protected $xDevice;
 52+
 53+ /**
 54+ * @var string MediaWiki 'action'
 55+ */
 56+ protected $action;
 57+
 58+ /**
 59+ * @var string
 60+ */
 61+ protected $mobileAction;
4762
4863 /**
4964 * @var WmlContext
——@@ -127,8 +142,7 @@
128143 */
129144 public function testCanonicalRedirect( $request, $title, $output ) {
130145 global $wgUsePathInfo;
131 - $xDevice = isset( $_SERVER['HTTP_X_DEVICE'] ) ? $_SERVER['HTTP_X_DEVICE'] : '';
132 - if ( empty( $xDevice ) ) {
 146+ if ( !$this->shouldDisplayMobileView() ) {
133147 return true; // Let the redirect happen
134148 } else {
135149 if ( $title->getNamespace() == NS_SPECIAL ) {
——@@ -294,15 +308,14 @@
295309 */
296310 public function beforePageRedirect( $out, &$redirect, &$code ) {
297311 wfProfileIn( __METHOD__ );
 312+ $shouldDisplayMobileView = $this->shouldDisplayMobileView();
298313 if ( $out->getTitle()->isSpecial( 'Userlogin' ) ) {
299 - $xDevice = isset( $_SERVER['HTTP_X_DEVICE'] ) ? $_SERVER['HTTP_X_DEVICE'] : '';
300 - if ( $xDevice ) {
 314+ if ( $shouldDisplayMobileView ) {
301315 $forceHttps = true;
302316 $redirect = $this->getMobileUrl( $redirect, $forceHttps );
303317 }
304318 } else if ( $out->getTitle()->isSpecial( 'Randompage' ) ) {
305 - $xDevice = isset( $_SERVER['HTTP_X_DEVICE'] ) ? $_SERVER['HTTP_X_DEVICE'] : '';
306 - if ( $xDevice ) {
 319+ if ( $shouldDisplayMobileView ) {
307320 $redirect = $this->getMobileUrl( $redirect );
308321 }
309322 }
——@@ -325,22 +338,15 @@
326339 // Thus, globalized objects will not be available as expected in the function.
327340 // This is stated to be intended behavior, as per the following: [http://bugs.php.net/bug.php?id=40104]
328341
329 - $xDevice = isset( $_SERVER['HTTP_X_DEVICE'] ) ? $_SERVER['HTTP_X_DEVICE'] : '';
 342+ $xDevice = $this->getXDevice();
330343 self::$useFormat = $wgRequest->getText( 'useformat' );
331344 $this->wmlContext->setUseFormat( self::$useFormat );
332 - $mobileAction = $wgRequest->getText( 'mobileaction' );
333 - $action = $wgRequest->getText( 'action' );
 345+ $mobileAction = $this->getMobileAction();
334346
335 - if ( self::$useFormat !== 'mobile' && self::$useFormat !== 'mobile-wap' &&
336 - !$xDevice ) {
 347+ if ( !$this->shouldDisplayMobileView() ) {
337348 wfProfileOut( __METHOD__ );
338349 return true;
339350 }
340 - if ( $action === 'edit' || $action === 'history' ||
341 - $mobileAction === 'view_normal_site' ) {
342 - wfProfileOut( __METHOD__ );
343 - return true;
344 - }
345351
346352 $userAgent = $_SERVER['HTTP_USER_AGENT'];
347353 $acceptHeader = isset( $_SERVER["HTTP_ACCEPT"] ) ? $_SERVER["HTTP_ACCEPT"] : '';
——@@ -1271,6 +1277,7 @@
12721278 public function getMobileUrl( $url, $forceHttps = false ) {
12731279 $parsedUrl = wfParseUrl( $url );
12741280 $this->updateMobileUrlHost( $parsedUrl );
 1281+ $this->updateMobileUrlQueryString( $parsedUrl );
12751282 if ( $forceHttps ) {
12761283 $parsedUrl[ 'scheme' ] = 'https';
12771284 }
——@@ -1342,6 +1349,20 @@
13431350 $parsedUrl[ 'path' ] = $wgScriptPath . $templatePathSansToken . $pathSansScriptPath;
13441351 }
13451352
 1353+ protected function updateMobileUrlQueryString( &$parsedUrl ) {
 1354+ if ( !$this->isFauxMobileDevice() ) {
 1355+ return;
 1356+ }
 1357+
 1358+ if ( !isset( $parsedUrl[ 'query' ] )) {
 1359+ $parsedUrl[ 'query' ] = 'useformat=' . urlencode( self::$useFormat );
 1360+ } else {
 1361+ $query = wfCgiToArray( $parsedUrl[ 'query' ] );
 1362+ $query[ 'useformat' ] = urlencode( self::$useFormat );
 1363+ $parsedUrl[ 'query' ] = wfArrayToCgi( $query );
 1364+ }
 1365+ }
 1366+
13461367 /**
13471368 * Parse mobile URL template into its host and path components.
13481369 *
——@@ -1377,6 +1398,67 @@
13781399 }
13791400 }
13801401
 1402+ protected function isMobileDevice() {
 1403+ $xDevice = $this->getXDevice();
 1404+
 1405+ if ( empty( $xDevice )) {
 1406+ return false;
 1407+ }
 1408+
 1409+ return true;
 1410+ }
 1411+
 1412+ protected function isFauxMobileDevice() {
 1413+ if ( self::$useFormat !== 'mobile' && self::$useFormat !== 'mobile-wap') {
 1414+ return false;
 1415+ }
 1416+
 1417+ return true;
 1418+ }
 1419+
 1420+ protected function shouldDisplayMobileView() {
 1421+ if ( !$this->isMobileDevice() && !$this->isFauxMobileDevice() ) {
 1422+ return false;
 1423+ }
 1424+
 1425+ $action = $this->getAction();
 1426+ $mobileAction = $this->getMobileAction();
 1427+
 1428+ if ( $action === 'edit' || $action === 'history' ||
 1429+ $mobileAction === 'view_normal_site' ) {
 1430+ return false;
 1431+ }
 1432+
 1433+ return true;
 1434+ }
 1435+
 1436+ public function getXDevice() {
 1437+ if ( is_null( $this->xDevice ) ) {
 1438+ $this->xDevice = isset( $_SERVER['HTTP_X_DEVICE'] ) ?
 1439+ $_SERVER['HTTP_X_DEVICE'] : '';
 1440+ }
 1441+
 1442+ return $this->xDevice;
 1443+ }
 1444+
 1445+ public function getMobileAction() {
 1446+ global $wgRequest;
 1447+ if ( is_null( $this->mobileAction )) {
 1448+ $this->mobileAction = $wgRequest->getText( 'mobileaction' );
 1449+ }
 1450+
 1451+ return $this->mobileAction;
 1452+ }
 1453+
 1454+ public function getAction() {
 1455+ global $wgRequest;
 1456+ if ( is_null( $this->action )) {
 1457+ $this->action = $wgRequest->getText( 'action' );
 1458+ }
 1459+
 1460+ return $this->action;
 1461+ }
 1462+
13811463 public function getVersion() {
13821464 return __CLASS__ . ': $Id$';
13831465 }

Comments

#Comment by Randomfvideos (talk | contribs)   00:43, 4 March 2012

What is this about?

#Comment by Awjrichards (talk | contribs)   17:53, 6 March 2012

This is part of an ongoing effort to make the MobileFrontend extension usable to others besides the Wikimedia Foundation. This is a pretty early step, but it begins to reduce the exclusive reliance on using X-Device headers to determine whether or not to display the mobile view as well as lays some groundwork for continuing in that direction.

Status & tagging log