jenkins-bot has submitted this change and it was merged.
Change subject: Wikibase parsing of diff=... parameter was different from core
......................................................................
Wikibase parsing of diff=... parameter was different from core
Core logik is basically:
* Check if the diff parameter does exists.
* If yes, check if it is equal to 'prev'.
* If it's equal to 'next'.
* Convert everything else to a number.
* If that conversion returns 0 compare to the current revision.
* Else try to use the number to find a revision.
* That may fail (e.g. for diff=-1).
In other words: In core every non-numeric value including 'cur',
'curr', 'c', '0' and even the empty string and null
(...?diff&oldid=123 will work) does exactly the same as 'cur'.
Bug: 61749
Change-Id: Id7aab6ec65a23128fffe4c9e366213603a7e59c6
---
M repo/includes/ContentRetriever.php
M repo/includes/actions/ViewEntityAction.php
M repo/tests/phpunit/includes/ContentRetrieverTest.php
3 files changed, 158 insertions(+), 69 deletions(-)
Approvals:
Tobias Gritschacher: Looks good to me, approved
WikidataJenkins: Verified
jenkins-bot: Verified
diff --git a/repo/includes/ContentRetriever.php
b/repo/includes/ContentRetriever.php
index 10f0eb0..5ca5495 100644
--- a/repo/includes/ContentRetriever.php
+++ b/repo/includes/ContentRetriever.php
@@ -5,7 +5,6 @@
use Article;
use Content;
use Revision;
-use Title;
use WebRequest;
/**
@@ -18,6 +17,7 @@
*
* @licence GNU GPL v2+
* @author Katie Filbert < aude.w...@gmail.com >
+ * @author Thiemo Mättig < thiemo.maet...@wikimedia.de >
*/
class ContentRetriever {
@@ -30,20 +30,18 @@
* @todo split out the get revision id stuff, add tests and see if
* any core code can be shared here
*
- * @param Article $article
- * @param Title $title
* @param WebRequest $request
+ * @param Article $article
*
* @return Content|null
*/
- public function getContentForRequest( Article $article, Title $title,
WebRequest $request ) {
- $queryValues = $request->getQueryValues();
- $oldId = $article->getOldID();
+ public function getContentForRequest( WebRequest $request, Article
$article ) {
+ $revision = $article->getRevisionFetched();
- if ( array_key_exists( 'diff', $queryValues ) ) {
- $revision = $this->getDiffRevision( $oldId,
$queryValues['diff'] );
- } else {
- $revision = Revision::newFromTitle( $title, $oldId );
+ if ( $request->getCheck( 'diff' ) ) {
+ $oldId = $revision->getId();
+ $diffValue = $request->getVal( 'diff' );
+ $revision = $this->resolveDiffRevision( $oldId,
$diffValue, $article );
}
return $revision !== null ?
@@ -56,53 +54,39 @@
* @since 0.5
*
* @param int $oldId
- * @param string|int $diffValue
+ * @param int|string $diffValue
+ * @param Article $article
*
* @return Revision|null
*/
- public function getDiffRevision( $oldId, $diffValue ) {
- if ( $this->isSpecialDiffParam( $diffValue ) ) {
- return $this->resolveDiffRevision( $oldId, $diffValue );
+ protected function resolveDiffRevision( $oldId, $diffValue, Article
$article )
+ {
+ $newid = $this->resolveNewid( $oldId, $diffValue, $article );
+
+ if ( !$newid ) {
+ $newid = $article->getTitle()->getLatestRevID();
}
- if ( is_numeric( $diffValue ) ) {
- $revId = (int)$diffValue;
- return Revision::newFromId( $revId );
- }
-
- // uses default handling for revision not found
- // @todo error handling could be improved!
- return null;
+ return Revision::newFromId( $newid );
}
/**
- * @param mixed $diffValue
+ * Get the revision ID specified in the diff parameter or prev/next
revision of oldid
*
- * @return boolean
- */
- protected function isSpecialDiffParam( $diffValue ) {
- return in_array( $diffValue, array( 'prev', 'next', 'cur', '0'
), true );
- }
-
- /**
- * For non-revision ids in the diff request param, get the correct
revision
+ * @since 0.5
*
* @param int $oldId
- * @param string|int $diffValue
+ * @param int|string $diffValue
+ * @param Article $article
*
- * @return Revision
+ * @return Bool|int
*/
- protected function resolveDiffRevision( $oldId, $diffValue ) {
- $oldIdRev = Revision::newFromId( $oldId );
-
- if ( $diffValue === 0 || $diffValue === 'cur' ) {
- $curId = $oldIdRev->getTitle()->getLatestRevID();
- return Revision::newFromId( $curId );
- } elseif ( $diffValue === 'next' ) {
- return $oldIdRev->getNext();
- }
-
- return $oldIdRev;
+ protected function resolveNewid( $oldId, $diffValue, Article $article )
+ {
+ $contentHandler =
$article->getRevisionFetched()->getContentHandler();
+ $context = $article->getContext();
+ $differenceEngine = $contentHandler->createDifferenceEngine(
$context, $oldId, $diffValue );
+ return $differenceEngine->getNewid();
}
}
diff --git a/repo/includes/actions/ViewEntityAction.php
b/repo/includes/actions/ViewEntityAction.php
index da5d845..1e18d58 100644
--- a/repo/includes/actions/ViewEntityAction.php
+++ b/repo/includes/actions/ViewEntityAction.php
@@ -82,9 +82,8 @@
public function show() {
$contentRetriever = new ContentRetriever();
$content = $contentRetriever->getContentForRequest(
- $this->getArticle(),
- $this->getTitle(),
- $this->getRequest()
+ $this->getRequest(),
+ $this->getArticle()
);
if ( is_null( $content ) ) {
@@ -130,9 +129,8 @@
$contentRetriever = new ContentRetriever();
$content = $contentRetriever->getContentForRequest(
- $this->getArticle(),
- $this->getTitle(),
- $this->getRequest()
+ $this->getRequest(),
+ $this->getArticle()
);
if ( !( $content instanceof EntityContent ) ) {
@@ -141,7 +139,7 @@
return false;
}
- if ( $this->getContext()->getRequest()->getCheck( 'diff' ) ) {
+ if ( $this->getRequest()->getCheck( 'diff' ) ) {
// showing a diff
return false;
}
@@ -185,7 +183,7 @@
}
// Create and set the title.
- if ( $this->getContext()->getRequest()->getCheck( 'diff' ) ) {
+ if ( $this->getRequest()->getCheck( 'diff' ) ) {
// Escaping HTML characters in order to retain original
label that may contain HTML
// characters. This prevents having characters
evaluated or stripped via
// OutputPage::setPageTitle:
@@ -236,7 +234,7 @@
if ( $wgSend404Code ) {
// If there's no backing content, send a 404 Not Found
// for better machine handling of broken links.
- $this->getContext()->getRequest()->response()->header(
"HTTP/1.1 404 Not Found" );
+ $this->getRequest()->response()->header( "HTTP/1.1 404
Not Found" );
}
$hookResult = wfRunHooks( 'BeforeDisplayNoArticleText', array(
$this ) );
diff --git a/repo/tests/phpunit/includes/ContentRetrieverTest.php
b/repo/tests/phpunit/includes/ContentRetrieverTest.php
index 6d904b3..e49bd63 100644
--- a/repo/tests/phpunit/includes/ContentRetrieverTest.php
+++ b/repo/tests/phpunit/includes/ContentRetrieverTest.php
@@ -3,6 +3,7 @@
namespace Wikibase\Test;
use Revision;
+use Title;
use Wikibase\ContentRetriever;
use Wikibase\Item;
use Wikibase\ItemContent;
@@ -28,30 +29,125 @@
*/
class ContentRetrieverTest extends \MediaWikiTestCase {
+ /**
+ * @param int $oldId
+ * @param Revision $revision
+ * @param Title $title
+ *
+ * @return \PHPUnit_Framework_MockObject_MockObject
+ */
+ private function getArticleMock( $oldId, Revision $revision, Title
$title )
+ {
+ $context = $this->getContextMock( $title );
+ $page = $this->getPageMock( $revision );
+
+ $article = $this->getMockBuilder( 'Article' )
+ ->disableOriginalConstructor()
+ ->getMock();
+
+ $article->expects( $this->any() )
+ ->method( 'getContext' )
+ ->will( $this->returnValue( $context ) );
+
+ $article->expects( $this->any() )
+ ->method( 'getOldID' )
+ ->will( $this->returnValue( $oldId ) );
+
+ $article->expects( $this->any() )
+ ->method( 'getPage' )
+ ->will( $this->returnValue( $page ) );
+
+ $article->expects( $this->any() )
+ ->method( 'getRevisionFetched' )
+ ->will( $this->returnValue( $revision ) );
+
+ $article->expects( $this->any() )
+ ->method( 'getTitle' )
+ ->will( $this->returnValue( $title ) );
+
+ return $article;
+ }
+
+ /**
+ * @param Title $title
+ *
+ * @return \PHPUnit_Framework_MockObject_MockObject
+ */
+ private function getContextMock( Title $title )
+ {
+ $context = $this->getMockBuilder( 'IContextSource' )
+ ->disableOriginalConstructor()
+ ->getMock();
+
+ $context->expects( $this->any() )
+ ->method( 'getLanguage' )
+ ->will( $this->returnValue( $title->getPageLanguage() )
);
+
+ $context->expects( $this->any() )
+ ->method( 'getTitle' )
+ ->will( $this->returnValue( $title ) );
+
+ return $context;
+ }
+
+ /**
+ * @param Revision $revision
+ *
+ * @return \PHPUnit_Framework_MockObject_MockObject
+ */
+ private function getPageMock( Revision $revision )
+ {
+ $page = $this->getMockBuilder( 'WikiPage' )
+ ->disableOriginalConstructor()
+ ->getMock();
+
+ $page->expects( $this->any() )
+ ->method( 'getContentHandler' )
+ ->will( $this->returnValue(
$revision->getContentHandler() ) );
+
+ return $page;
+ }
+
+ /**
+ * @param array $queryParams
+ *
+ * @return \PHPUnit_Framework_MockObject_MockObject
+ */
+ private function getRequestMock( array $queryParams )
+ {
+ $request = $this->getMockBuilder( 'WebRequest' )
+ ->disableOriginalConstructor()
+ ->getMock();
+
+ $request->expects( $this->any() )
+ ->method( 'getCheck' )
+ ->will( $this->returnValue( isset( $queryParams['diff']
) ) );
+
+ $request->expects( $this->any() )
+ ->method( 'getQueryValues' )
+ ->will( $this->returnValue( $queryParams ) );
+
+ $request->expects( $this->any() )
+ ->method( 'getVal' )
+ ->will( $this->returnValue( isset( $queryParams['diff']
) ? $queryParams['diff'] : null ) );
+
+ return $request;
+ }
+
public function testGetContentForRequest() {
$cases = $this->getContentCases();
foreach( $cases as $case ) {
+ /** @var Title $title */
list( $expected, $oldId, $queryParams, $title, $message
) = $case;
- $article = $this->getMockBuilder( 'Article' )
- ->disableOriginalConstructor()
- ->getMock();
+ $revision = Revision::newFromId( $oldId );
- $article->expects( $this->any() )
- ->method( 'getOldID' )
- ->will( $this->returnValue( $oldId ) );
-
- $request = $this->getMockBuilder( 'WebRequest' )
- ->disableOriginalConstructor()
- ->getMock();
-
- $request->expects( $this->any() )
- ->method( 'getQueryValues' )
- ->will( $this->returnValue( $queryParams ) );
+ $request = $this->getRequestMock( $queryParams );
+ $article = $this->getArticleMock( $oldId, $revision,
$title );
$contentRetriever = new ContentRetriever();
- $content = $contentRetriever->getContentForRequest(
$article, $title, $request );
+ $content = $contentRetriever->getContentForRequest(
$request, $article );
$this->assertEquals( $expected, $content, $message );
}
@@ -77,10 +173,18 @@
$content->getEntity()->setDescription( 'en',
$description );
$page->doEditContent( $content, 'edit description' );
$revIds[] = $page->getLatest();
+
+ /** @var ItemContent $content */
+ $content = Revision::newFromId( $revIds[count( $revIds
) - 1] )->getContent();
}
+ /**
+ * @var ItemContent $content2
+ * @var ItemContent $content3
+ */
$content2 = Revision::newFromId( $revIds[1] )->getContent();
$content3 = Revision::newFromId( $revIds[2] )->getContent();
+ $this->assertNotEquals( $content2, $content3, 'Setup failed' );
return array(
array( $content3, $revIds[0], array( 'diff' => '0',
'oldid' => $revIds[0] ), $title, 'diff=0' ),
@@ -88,8 +192,11 @@
array( $content2, $revIds[0], array( 'diff' => 'next',
'oldid' => $revIds[0] ), $title, 'diff=next' ),
array( $content2, $revIds[1], array( 'diff' => 'prev',
'oldid' => $revIds[1] ), $title, 'diff=prev' ),
array( $content3, $revIds[1], array( 'diff' => 'cur',
'oldid' => $revIds[1] ), $title, 'diff=cur' ),
+ array( $content3, $revIds[1], array( 'diff' => 'c',
'oldid' => $revIds[1] ), $title, 'diff=c' ),
+ array( $content3, $revIds[1], array( 'diff' => '0',
'oldid' => $revIds[1] ), $title, 'diff=0' ),
+ array( $content3, $revIds[1], array( 'diff' => '',
'oldid' => $revIds[1] ), $title, 'diff=' ),
array( $content3, $revIds[2], array(), $title, 'no
query params' ),
- array( null, $revIds[1], array( 'diff' => 'kitten',
'oldid' => $revIds[0] ), $title, 'diff=kitten' )
+ array( null, $revIds[1], array( 'diff' => '-1', 'oldid'
=> $revIds[0] ), $title, 'diff=-1' )
);
}
--
To view, visit https://gerrit.wikimedia.org/r/115310
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Id7aab6ec65a23128fffe4c9e366213603a7e59c6
Gerrit-PatchSet: 8
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Thiemo Mättig (WMDE) <thiemo.maet...@wikimedia.de>
Gerrit-Reviewer: Addshore <addshorew...@gmail.com>
Gerrit-Reviewer: Aude <aude.w...@gmail.com>
Gerrit-Reviewer: Thiemo Mättig (WMDE) <thiemo.maet...@wikimedia.de>
Gerrit-Reviewer: Tobias Gritschacher <tobias.gritschac...@wikimedia.de>
Gerrit-Reviewer: WikidataJenkins <wikidata-servi...@wikimedia.de>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
