jenkins-bot has submitted this change and it was merged.
Change subject: Move desktop->mobile diff redirect logic from to
Special:MobileDiff
......................................................................
Move desktop->mobile diff redirect logic from to Special:MobileDiff
* somewhat generecizes desktop -> mobile redirect method in
MobileContext for potential similar cases in the future
* simplifies unit tests
* Move MFMockRevision out of its own file into SpecialMobileDiffTest.php
Change-Id: Ide0b9d13e2504c186c91640ce29466a0c3ef2d3e
---
M MobileFrontend.php
M includes/MobileContext.php
M includes/specials/SpecialMobileDiff.php
M tests/MobileContextTest.php
D tests/mocks/MFMockRevision.php
M tests/specials/SpecialMobileDiffTest.php
6 files changed, 136 insertions(+), 139 deletions(-)
Approvals:
Jdlrobson: Verified; Looks good to me, approved
jenkins-bot: Verified
diff --git a/MobileFrontend.php b/MobileFrontend.php
index d6f451e..423b241 100644
--- a/MobileFrontend.php
+++ b/MobileFrontend.php
@@ -139,8 +139,6 @@
*/
function efExtMobileFrontendUnitTests( &$files ) {
$dir = dirname( __FILE__ ) . '/tests';
- // mocks
- $files[] = "$dir/mocks/MFMockRevision.php";
$files[] = "$dir/ApiParseExtenderTest.php";
$files[] = "$dir/MobileContextTest.php";
diff --git a/includes/MobileContext.php b/includes/MobileContext.php
index 81285a6..a5c9759 100644
--- a/includes/MobileContext.php
+++ b/includes/MobileContext.php
@@ -237,56 +237,17 @@
/**
* If a page has an equivalent but different mobile page redirect to it
- *
*/
private function redirectMobileEnabledPages() {
- $req = $this->getRequest();
- $rev2 = $req->getText( 'diff' );
- $rev1 = $req->getText( 'oldid' );
- // redirect requests to the diff page to mobile view
- if ( !$rev2 ) {
- if ( $rev1 ) {
- $rev2 = $rev1;
- $rev1 = '';
- } else {
- return;
- }
+ $redirectUrl = null;
+ if ( $this->getRequest()->getText( 'diff' ) ||
+ $this->getRequest()->getText( 'oldid' ) ) {
+ $redirectUrl =
SpecialMobileDiff::getMobileUrlFromDesktop();
}
- if ( $rev1 ) {
- $rev = $this->getRevision( $rev1 );
- if ( $rev ) {
- // the diff parameter could be the string prev
or next - deal with these cases
- if ( $rev2 === 'prev' ) {
- $prev = $rev->getPrevious();
- // yes this is confusing - this is how
it works arrgghh
- $rev2 = $rev1;
- $rev1 = $prev ? $prev->getId() : '';
- } else if ( $rev2 === 'next' ) {
- $next = $rev->getNext();
- $rev2 = $next ? $next->getId() : '';
- } else {
- $rev2 = $this->getRevision( $rev2 );
- $rev2 = $rev2 ? $rev2->getId() : '';
- }
- } else {
- $rev2 = '';
- }
+ if ( $redirectUrl ) {
+ $this->getOutput()->redirect( $redirectUrl );
}
-
- if ( $rev2 ) {
- $subpage = $rev1 ? $rev1 . '...' . $rev2 : $rev2;
- $title = SpecialPage::getTitleFor( 'MobileDiff',
$subpage );
- $this->getOutput()->redirect( $this->getMobileUrl(
$title->getFullURL() ) );
- }
- }
-
- /**
- * @param int $revisioniId
- * @return Revision
- */
- protected function getRevision( $revisioniId ) {
- return Revision::newFromId( $revisioniId );
}
/**
diff --git a/includes/specials/SpecialMobileDiff.php
b/includes/specials/SpecialMobileDiff.php
index 7a87e23..7193239 100644
--- a/includes/specials/SpecialMobileDiff.php
+++ b/includes/specials/SpecialMobileDiff.php
@@ -13,7 +13,7 @@
parent::__construct( 'MobileDiff' );
}
- public function getRevision( $id ) {
+ public static function getRevision( $id ) {
return Revision::newFromId( $id );
}
@@ -37,12 +37,12 @@
$id = intval( $revids[1] );
$prevId = intval( $revids[0] );
if ( $id && $prevId ) {
- $rev = $this->getRevision( $id );
+ $rev = static::getRevision( $id );
// deal with identical ids
if ( $id === $prevId ) {
$rev = null;
} else if ( $rev ) {
- $prev = $this->getRevision( $prevId );
+ $prev = static::getRevision( $prevId );
if ( !$prev ) {
$rev = null;
}
@@ -53,7 +53,7 @@
} else if ( count( $revids ) === 1 ) {
$id = intval( $revids[0] );
if ( $id ) {
- $rev = $this->getRevision( $id );
+ $rev = static::getRevision( $id );
if ( $rev ) {
$prev = $rev->getPrevious();
}
@@ -231,4 +231,47 @@
return $this->getLanguage()->commaList( $userMembers );
}
+
+ public static function getMobileUrlFromDesktop() {
+ $req = MobileContext::singleton()->getRequest();
+ $rev2 = $req->getText( 'diff' );
+ $rev1 = $req->getText( 'oldid' );
+ // redirect requests to the diff page to mobile view
+ if ( !$rev2 ) {
+ if ( $rev1 ) {
+ $rev2 = $rev1;
+ $rev1 = '';
+ } else {
+ return false;
+ }
+ }
+
+ if ( $rev1 ) {
+ $rev = static::getRevision( $rev1 );
+ if ( $rev ) {
+ // the diff parameter could be the string prev
or next - deal with these cases
+ if ( $rev2 === 'prev' ) {
+ $prev = $rev->getPrevious();
+ // yes this is confusing - this is how
it works arrgghh
+ $rev2 = $rev1;
+ $rev1 = $prev ? $prev->getId() : '';
+ } else if ( $rev2 === 'next' ) {
+ $next = $rev->getNext();
+ $rev2 = $next ? $next->getId() : '';
+ } else {
+ $rev2 = static::getRevision( $rev2 );
+ $rev2 = $rev2 ? $rev2->getId() : '';
+ }
+ } else {
+ $rev2 = '';
+ }
+ }
+
+ if ( $rev2 ) {
+ $subpage = $rev1 ? $rev1 . '...' . $rev2 : $rev2;
+ $title = SpecialPage::getTitleFor( 'MobileDiff',
$subpage );
+ return $title->getLocalURL();
+ }
+ return false;
+ }
}
diff --git a/tests/MobileContextTest.php b/tests/MobileContextTest.php
index da1505c..2b051d2 100644
--- a/tests/MobileContextTest.php
+++ b/tests/MobileContextTest.php
@@ -451,52 +451,4 @@
);
}
- /**
- * @dataProvider getMobileRedirection
- */
- public function testDiffRedirection( array $query, $isMobile, $expected
) {
- $req = new FauxRequest( $query );
- if ( $isMobile ) {
- $req->setHeader( 'X-Device', 'test' );
- }
- if ( $expected ) {
- $expected = wfExpandUrl( Title::newFromText( $expected
)->getFullURL(), PROTO_HTTP );
- }
- $context = new RequestContext();
- $context->setRequest( $req );
- $mobileContext = new MFMockMobileContext( $context );
- $mobileContext->shouldDisplayMobileView();
- $this->assertEquals( $expected,
$context->getOutput()->mRedirect );
- }
-
- public function getMobileRedirection() {
- return array(
- array( array(), true, '' ),
- // simple case
- array( array(), false, '' ),
- // this makes no sense but this is the url for newly
created pages (oldid but no diff)
- array( array( 'oldid' => 5 ), true,
'Special:MobileDiff/5' ),
- array( array( 'diff' => 123 ), true,
'Special:MobileDiff/123' ),
- // some more complicated cases...
- array( array( 'oldid' => 90, 'diff' => 100 ), true,
'Special:MobileDiff/90...100' ),
- array( array( 'oldid' => 123, 'diff' => 'next' ), true,
'Special:MobileDiff/123...124' ),
- array( array( 'oldid' => 123, 'diff' => 'prev' ), true,
'Special:MobileDiff/122...123' ),
- // bad id given (revisions older than 200 do not exist
in our MockRevision)
- array( array( 'diff' => 208, 'oldid' => 50 ), true, ''
),
- array( array( 'diff' => 50, 'oldid' => 208 ), true, ''
),
- array( array( 'diff' => 'prev', 'oldid' => 201 ), true,
'' ),
- // weird edge case comparing identical things
- array( array( 'oldid' => 101, 'diff' => 101 ), true,
'Special:MobileDiff/101...101' ),
- );
- }
-}
-
-class MFMockMobileContext extends MobileContext {
- public function __construct( IContextSource $context ) {
- parent::__construct( $context );
- }
-
- protected function getRevision( $revisionId ) {
- return MFMockRevision::newFromId( $revisionId );
- }
}
diff --git a/tests/mocks/MFMockRevision.php b/tests/mocks/MFMockRevision.php
deleted file mode 100644
index 4a37a3a..0000000
--- a/tests/mocks/MFMockRevision.php
+++ /dev/null
@@ -1,39 +0,0 @@
-<?php
-class MFMockRevision extends Revision {
- private $id;
-
- public function getId() {
- return $this->id;
- }
-
- public function __construct( $revisionId ) {
- if ( $revisionId > 200 ) {
- throw new MWException( 'Unknown revision ID' );
- }
- $this->id = $revisionId;
- }
-
- public static function newFromId( $revisionId, $flags = 0 ) {
- if ( $revisionId <= 200 ) {
- return new MFMockRevision( $revisionId );
- }
- }
-
- public function getTitle() {
- return new Title( "page_$this->id" );
- }
-
- public function getPrevious() {
- return new MFMockRevision( $this->id - 1 );
- }
-
- /**
- * Get next revision for this title
- *
- * @return Revision or null
- */
- public function getNext() {
- return new MFMockRevision( $this->id + 1 );
- }
-}
-?>
diff --git a/tests/specials/SpecialMobileDiffTest.php
b/tests/specials/SpecialMobileDiffTest.php
index fe8e11e..6f16c6d 100644
--- a/tests/specials/SpecialMobileDiffTest.php
+++ b/tests/specials/SpecialMobileDiffTest.php
@@ -4,7 +4,16 @@
* @group MobileFrontend
*/
class SpecialMobileDiffTest extends MediaWikiTestCase {
+ /** Keeps track of request variables that should be unset on teardown
**/
+ private $unsetReqVals = array();
+ public function tearDown() {
+ foreach ( $this->unsetReqVals as $v ) {
+ MobileContext::singleton()->getRequest()->unsetVal( $v
);
+ }
+ MobileContext::setInstance( null ); // refresh MobileContext
instance
+ parent::tearDown();
+ }
/**
* @dataProvider providerTestNames
*/
@@ -36,13 +45,86 @@
array( 'prev...next', false ),
);
}
+
+ /**
+ * @dataProvider redirectFromDesktopDiffProvider
+ */
+ public function testRedirectFromDesktopDiff( array $query, $expected ) {
+ foreach ( $query as $k => $v ) {
+ MobileContext::singleton()->getRequest()->setVal( $k,
$v );
+ $this->unsetReqVals[] = $k;
+ }
+ if ( $expected ) {
+ $expected = Title::newFromText( $expected
)->getLocalURL();
+ }
+ $redirectUrl = MockSpecialMobileDiff::getMobileUrlFromDesktop();
+ $this->assertEquals( $expected, $redirectUrl );
+ }
+
+ public function redirectFromDesktopDiffProvider() {
+ return array(
+ array( array(), false ),
+ // this makes no sense but this is the url for newly
created pages (oldid but no diff)
+ array( array( 'oldid' => 5 ), 'Special:MobileDiff/5' ),
+ array( array( 'diff' => 123 ), 'Special:MobileDiff/123'
),
+ // some more complicated cases...
+ array( array( 'oldid' => 90, 'diff' => 100 ),
'Special:MobileDiff/90...100' ),
+ array( array( 'oldid' => 123, 'diff' => 'next' ),
'Special:MobileDiff/123...124' ),
+ array( array( 'oldid' => 123, 'diff' => 'prev' ),
'Special:MobileDiff/122...123' ),
+ // bad id given (revisions older than 200 do not exist
in our MockRevision)
+ array( array( 'diff' => 208, 'oldid' => 50 ), '' ),
+ array( array( 'diff' => 50, 'oldid' => 208 ), '' ),
+ array( array( 'diff' => 'prev', 'oldid' => 201 ), '' ),
+ // weird edge case comparing identical things
+ array( array( 'oldid' => 101, 'diff' => 101 ),
'Special:MobileDiff/101...101' ),
+ );
+ }
}
class MockSpecialMobileDiff extends SpecialMobileDiff {
- public function getRevision( $id ) {
+ public static function getRevision( $id ) {
return MFMockRevision::newFromId( $id );
}
public function executeBadQuery() {
return false;
}
}
+
+class MFMockRevision extends Revision {
+ private $id;
+
+ public function getId() {
+ return $this->id;
+ }
+
+ public function __construct( $revisionId ) {
+ if ( $revisionId > 200 ) {
+ throw new MWException( 'Unknown revision ID' );
+ }
+ $this->id = $revisionId;
+ }
+
+ public static function newFromId( $revisionId, $flags = 0 ) {
+ if ( $revisionId <= 200 ) {
+ return new MFMockRevision( $revisionId );
+ }
+ }
+
+ public function getTitle() {
+ return new Title( "page_$this->id" );
+ }
+
+ public function getPrevious() {
+ return new MFMockRevision( $this->id - 1 );
+ }
+
+ /**
+ * Get next revision for this title
+ *
+ * @return Revision or null
+ */
+ public function getNext() {
+ return new MFMockRevision( $this->id + 1 );
+ }
+}
+
--
To view, visit https://gerrit.wikimedia.org/r/65163
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ide0b9d13e2504c186c91640ce29466a0c3ef2d3e
Gerrit-PatchSet: 3
Gerrit-Project: mediawiki/extensions/MobileFrontend
Gerrit-Branch: master
Gerrit-Owner: awjrichards <[email protected]>
Gerrit-Reviewer: Jdlrobson <[email protected]>
Gerrit-Reviewer: awjrichards <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
