jenkins-bot has submitted this change and it was merged.
Change subject: (bug 49434) Support diff=0 in Wikibase
......................................................................
(bug 49434) Support diff=0 in Wikibase
This adds support for requests with diff=0.
Also, moves the revision calculating code and getContent()
out of ViewEntityAction and adds/improves tests.
Ultimately, much of this code should be in core,
with stuff factored out of Article and DifferenceEngine
and merged together with this code, in a new core class.
But best not to block fixing this bug on that for now.
Change-Id: I6d160aa13f12feb98f2e95128e7e7a07ee6831d6
---
M repo/Wikibase.classes.php
A repo/includes/ContentRetriever.php
M repo/includes/actions/ViewEntityAction.php
A repo/tests/phpunit/includes/ContentRetrieverTest.php
4 files changed, 234 insertions(+), 65 deletions(-)
Approvals:
Daniel Kinzler: Looks good to me, approved
jenkins-bot: Verified
diff --git a/repo/Wikibase.classes.php b/repo/Wikibase.classes.php
index 581d1f8..8f7d4a9 100644
--- a/repo/Wikibase.classes.php
+++ b/repo/Wikibase.classes.php
@@ -34,6 +34,7 @@
// includes
'Wikibase\ClaimSaver' => 'includes/ClaimSaver.php',
'Wikibase\ClaimSummaryBuilder' =>
'includes/ClaimSummaryBuilder.php',
+ 'Wikibase\ContentRetriever' => 'includes/ContentRetriever.php',
'Wikibase\DataTypeSelector' => 'includes/DataTypeSelector.php',
'Wikibase\EditEntity' => 'includes/EditEntity.php',
'Wikibase\EntityContentDiffView' =>
'includes/EntityContentDiffView.php',
diff --git a/repo/includes/ContentRetriever.php
b/repo/includes/ContentRetriever.php
new file mode 100644
index 0000000..4d565b9
--- /dev/null
+++ b/repo/includes/ContentRetriever.php
@@ -0,0 +1,107 @@
+<?php
+
+namespace Wikibase;
+
+use Article;
+use Revision;
+use Title;
+use WebRequest;
+use WikiPage;
+
+/**
+ * Fetches content for a given Title / Article and request (diff or not diff)
+ *
+ * @since 0.5
+ *
+ * @todo put/merge this into core, with revision id / content fetching stuff
+ * factored out of DifferenceEngine and Article classes. :)
+ *
+ * @licence GNU GPL v2+
+ * @author Katie Filbert < aude.w...@gmail.com >
+ */
+class ContentRetriever {
+
+ /**
+ * Returns the content to display on a page, given request params.
+ *
+ * If it is a diff request, then display the revision specified
+ * in the 'diff=' request param.
+ *
+ * @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
+ *
+ * @return Content|null
+ */
+ public function getContentForRequest( Article $article, Title $title,
WebRequest $request ) {
+ $queryValues = $request->getQueryValues();
+ $oldId = $article->getOldID();
+
+ if ( array_key_exists( 'diff', $queryValues ) ) {
+ $revision = $this->getDiffRevision( $oldId,
$queryValues['diff'] );
+ } else {
+ $revision = Revision::newFromTitle( $title, $oldId );
+ }
+
+ return $revision !== null ? $revision->getContent() : null;
+ }
+
+ /**
+ * Get the revision specified in the diff parameter or prev/next
revision of oldid
+ *
+ * @since 0.5
+ *
+ * @param int $oldId
+ * @param string|int $diffValue
+ *
+ * @return Revision|null
+ */
+ public function getDiffRevision( $oldId, $diffValue ) {
+ if ( $this->isSpecialDiffParam( $diffValue ) ) {
+ return $this->resolveDiffRevision( $oldId, $diffValue );
+ }
+
+ 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;
+ }
+
+ /**
+ * @param mixed $diffValue
+ *
+ * @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
+ *
+ * @param int $oldId
+ * @param string|int $diffValue
+ *
+ * @return Revision
+ */
+ 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;
+ }
+
+}
diff --git a/repo/includes/actions/ViewEntityAction.php
b/repo/includes/actions/ViewEntityAction.php
index 24720a1..eae6cf1 100644
--- a/repo/includes/actions/ViewEntityAction.php
+++ b/repo/includes/actions/ViewEntityAction.php
@@ -51,6 +51,23 @@
}
/**
+ * Get the revision specified in the diff parameter or prev/next
revision of oldid
+ *
+ * @since 0.4
+ * @deprecated since 0.5
+ * use ContentRetriever::getDiffRevision
+ *
+ * @param int $oldId
+ * @param string|int $diffValue
+ *
+ * @return Revision|null
+ */
+ public function getDiffRevision( $oldId, $diffValue ) {
+ $contentRetriever = new ContentRetriever();
+ return $contentRetriever->getDiffRevision( $oldId, $diffValue );
+ }
+
+ /**
* @see Action::getName()
*
* @since 0.1
@@ -59,69 +76,6 @@
*/
public function getName() {
return 'view';
- }
-
- /**
- * Returns the content of the page in the revision being viewed.
- *
- * @todo split out the get revision id stuff, add tests and see if
- * any core code can be shared here
- *
- * @return EntityContent|null
- */
- protected function getContent() {
- $title = $this->getTitle();
- $queryValues = $this->getRequest()->getQueryValues();
-
- if ( array_key_exists( 'diff', $queryValues ) ) {
- $oldId = $this->getArticle()->getOldID();
- $revision = $this->getDiffRevision( $oldId,
$queryValues['diff'] );
- } else {
- $revisionId = $this->getArticle()->getOldID();
- $revision = \Revision::newFromTitle( $title,
$revisionId );
- }
-
- return $revision !== null ? $revision->getContent() : null;
- }
-
- /**
- * Get the revision specified in the diff parameter or prev/next
revision of oldid
- *
- * @todo ViewEntityAction really isn't a great place for this
- *
- * @since 0.4
- *
- * @param int $oldId
- * @param string|int $diffValue
- *
- * @return \Revision|null
- */
- public function getDiffRevision( $oldId, $diffValue ) {
- $revision = null;
-
- if ( in_array( $diffValue, array( 'prev', 'next', 'cur' ) ) ) {
- $oldIdRev = \Revision::newFromId( $oldId );
-
- if ( $oldIdRev !== null ) {
- switch ( $diffValue ) {
- case 'next':
- $revision =
$oldIdRev->getNext();
- break;
- case 'cur':
- $curId =
$oldIdRev->getTitle()->getLatestRevID();
- $revision =
\Revision::newFromId( $curId );
- break;
- default:
- $revision = $oldIdRev;
- break;
- }
- }
- } else {
- $revId = (int)$diffValue;
- $revision = \Revision::newFromId( $revId );
- }
-
- return $revision;
}
/**
@@ -144,7 +98,12 @@
* Parent is doing $this->checkCanExecute( $this->getUser() )
*/
public function show() {
- $content = $this->getContent();
+ $contentRetriever = new ContentRetriever();
+ $content = $contentRetriever->getContentForRequest(
+ $this->getArticle(),
+ $this->getTitle(),
+ $this->getRequest()
+ );
if ( is_null( $content ) ) {
$this->displayMissingEntity();
@@ -199,7 +158,12 @@
return false;
}
- $content = $this->getContent();
+ $contentRetriever = new ContentRetriever();
+ $content = $contentRetriever->getContentForRequest(
+ $this->getArticle(),
+ $this->getTitle(),
+ $this->getRequest()
+ );
if ( !( $content instanceof EntityContent ) ) {
//XXX: HACK against evil tricks in
Article::getContentObject
diff --git a/repo/tests/phpunit/includes/ContentRetrieverTest.php
b/repo/tests/phpunit/includes/ContentRetrieverTest.php
new file mode 100644
index 0000000..bb159e7
--- /dev/null
+++ b/repo/tests/phpunit/includes/ContentRetrieverTest.php
@@ -0,0 +1,97 @@
+<?php
+namespace Wikibase\Test;
+
+use Article;
+use Revision;
+use WebRequest;
+use Wikibase\ContentRetriever;
+use Wikibase\DataModel\Entity\ItemId;
+use Wikibase\Item;
+use Wikibase\ItemContent;
+use WikiPage;
+
+/**
+ * @covers ContentRetriever
+ *
+ * @licence GNU GPL v2+
+ * @author Katie Filbert < aude.w...@gmail.com >
+ *
+ * @group Wikibase
+ *
+ * The database group has as a side effect that temporal database tables are
created. This makes
+ * it possible to test without poisoning a production database.
+ * @group Database
+ *
+ * Some of the tests takes more time, and needs therefor longer time before
they can be aborted
+ * as non-functional. The reason why tests are aborted is assumed to be set up
of temporal databases
+ * that hold the first tests in a pending state awaiting access to the
database.
+ * @group medium
+ */
+class ContentRetrieverTest extends \MediaWikiTestCase {
+
+ public function testGetContentForRequest() {
+ $cases = $this->getContentCases();
+
+ foreach( $cases as $case ) {
+ list( $expected, $oldId, $queryParams, $title, $message
) = $case;
+
+ $article = $this->getMockBuilder( 'Article' )
+ ->disableOriginalConstructor()
+ ->getMock();
+
+ $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 ) );
+
+ $contentRetriever = new ContentRetriever();
+ $content = $contentRetriever->getContentForRequest(
$article, $title, $request );
+
+ $this->assertEquals( $expected, $content, $message );
+ }
+ }
+
+ private function getContentCases() {
+ $item = Item::newEmpty();
+ $item->setId( new ItemId( 'Q848' ) );
+
+ $descriptions = array(
+ 'Largest city in Germany',
+ 'Capital of Germany',
+ 'Best city in Germany'
+ );
+
+ $content = new ItemContent( $item );
+ $title = $content->getTitle();
+ $page = WikiPage::factory( $title );
+
+ $revIds = array();
+
+ foreach( $descriptions as $description ) {
+ $content->getEntity()->setDescription( 'en',
$description );
+ $page->doEditContent( $content, 'edit description' );
+ $revIds[] = $page->getLatest();
+ }
+
+ $content2 = Revision::newFromId( $revIds[1] )->getContent();
+ $content3 = Revision::newFromId( $revIds[2] )->getContent();
+
+ return array(
+ array( $content3, $revIds[0], array( 'diff' => '0',
'oldid' => $revIds[0] ), $title, 'diff=0' ),
+ array( $content2, $revIds[0], array( 'diff' =>
$revIds[1], 'oldid' => $revIds[0] ), $title, 'rev id' ),
+ 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[2], array(), $title, 'no
query params' ),
+ array( null, $revIds[1], array( 'diff' => 'kitten',
'oldid' => $revIds[0] ), $title, 'diff=kitten' )
+ );
+ }
+
+}
--
To view, visit https://gerrit.wikimedia.org/r/92648
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I6d160aa13f12feb98f2e95128e7e7a07ee6831d6
Gerrit-PatchSet: 6
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Aude <aude.w...@gmail.com>
Gerrit-Reviewer: Addshore <addshorew...@gmail.com>
Gerrit-Reviewer: Daniel Kinzler <daniel.kinz...@wikimedia.de>
Gerrit-Reviewer: Jeroen De Dauw <jeroended...@gmail.com>
Gerrit-Reviewer: Tobias Gritschacher <tobias.gritschac...@wikimedia.de>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
