From cf4f985f225f843acab6c3b56ce78cc12ffc2fc6 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Mon, 22 Oct 2018 12:13:01 -0400 Subject: [PATCH] ApiComparePages: Don't error with no prev/next rev Prior to I700edfa76, torelative=prev on the first revision of a page would "diff" from an empty revision, and torelative=next on the latest revision would diff to that same latest revision. People were depending on that behavior, so restore it. Bug: T203433 Change-Id: Ie81b58c196998a8047322740fe1d1fa44eff8526 --- includes/api/ApiComparePages.php | 40 +++++++++- includes/api/i18n/en.json | 2 + includes/api/i18n/qqq.json | 2 + .../includes/api/ApiComparePagesTest.php | 74 ++++++++++++++----- 4 files changed, 97 insertions(+), 21 deletions(-) diff --git a/includes/api/ApiComparePages.php b/includes/api/ApiComparePages.php index 76b7bce67b..3a92b7c465 100644 --- a/includes/api/ApiComparePages.php +++ b/includes/api/ApiComparePages.php @@ -64,16 +64,48 @@ class ApiComparePages extends ApiBase { switch ( $params['torelative'] ) { case 'prev': // Swap 'from' and 'to' - list( $toRev, $toRelRev2, $toValsRev ) = [ $fromRev, $fromRelRev, $fromValsRev ]; - $fromRev = $this->revisionStore->getPreviousRevision( $fromRelRev ); + list( $toRev, $toRelRev, $toValsRev ) = [ $fromRev, $fromRelRev, $fromValsRev ]; + $fromRev = $this->revisionStore->getPreviousRevision( $toRelRev ); $fromRelRev = $fromRev; $fromValsRev = $fromRev; + if ( !$fromRev ) { + $title = Title::newFromLinkTarget( $toRelRev->getPageAsLinkTarget() ); + $this->addWarning( [ + 'apiwarn-compare-no-prev', + wfEscapeWikiText( $title->getPrefixedText() ), + $toRelRev->getId() + ] ); + + // (T203433) Create an empty dummy revision as the "previous". + // The main slot has to exist, the rest will be handled by DifferenceEngine. + $fromRev = $this->revisionStore->newMutableRevisionFromArray( [ + 'title' => $title ?: Title::makeTitle( NS_SPECIAL, 'Badtitle/' . __METHOD__ ) + ] ); + $fromRev->setContent( + SlotRecord::MAIN, + $toRelRev->getContent( SlotRecord::MAIN, RevisionRecord::RAW ) + ->getContentHandler() + ->makeEmptyContent() + ); + } break; case 'next': $toRev = $this->revisionStore->getNextRevision( $fromRelRev ); $toRelRev = $toRev; $toValsRev = $toRev; + if ( !$toRev ) { + $title = Title::newFromLinkTarget( $fromRelRev->getPageAsLinkTarget() ); + $this->addWarning( [ + 'apiwarn-compare-no-next', + wfEscapeWikiText( $title->getPrefixedText() ), + $fromRelRev->getId() + ] ); + + // (T203433) The web UI treats "next" as "cur" in this case. + // Avoid repeating metadata by making a MutableRevisionRecord with no changes. + $toRev = MutableRevisionRecord::newFromParentRevision( $fromRelRev ); + } break; case 'cur': @@ -93,10 +125,12 @@ class ApiComparePages extends ApiBase { list( $toRev, $toRelRev, $toValsRev ) = $this->getDiffRevision( 'to', $params ); } - // Handle missing from or to revisions + // Handle missing from or to revisions (should never happen) + // @codeCoverageIgnoreStart if ( !$fromRev || !$toRev ) { $this->dieWithError( 'apierror-baddiff' ); } + // @codeCoverageIgnoreEnd // Handle revdel if ( !$fromRev->audienceCan( diff --git a/includes/api/i18n/en.json b/includes/api/i18n/en.json index 25bf3f74a7..88b5d68bce 100644 --- a/includes/api/i18n/en.json +++ b/includes/api/i18n/en.json @@ -1887,6 +1887,8 @@ "apiwarn-badurlparam": "Could not parse $1urlparam for $2. Using only width and height.", "apiwarn-badutf8": "The value passed for $1 contains invalid or non-normalized data. Textual data should be valid, NFC-normalized Unicode without C0 control characters other than HT (\\t), LF (\\n), and CR (\\r).", "apiwarn-checktoken-percentencoding": "Check that symbols such as \"+\" in the token are properly percent-encoded in the URL.", + "apiwarn-compare-no-next": "Revision $2 is the latest revision of $1, there is no revision for torelative=next to compare to.", + "apiwarn-compare-no-prev": "Revision $2 is the earliest revision of $1, there is no revision for torelative=prev to compare to.", "apiwarn-compare-nocontentmodel": "No content model could be determined, assuming $1.", "apiwarn-deprecation-deletedrevs": "list=deletedrevs has been deprecated. Please use prop=deletedrevisions or list=alldeletedrevisions instead.", "apiwarn-deprecation-httpsexpected": "HTTP used when HTTPS was expected.", diff --git a/includes/api/i18n/qqq.json b/includes/api/i18n/qqq.json index d27933051a..24077816ca 100644 --- a/includes/api/i18n/qqq.json +++ b/includes/api/i18n/qqq.json @@ -1774,6 +1774,8 @@ "apiwarn-badurlparam": "{{doc-apierror}}\n\nParameters:\n* $1 - Module parameter prefix, e.g. \"bl\".\n* $2 - Image title.", "apiwarn-badutf8": "{{doc-apierror}}\n\nParameters:\n* $1 - Parameter name.\n{{doc-important|Do not translate \"\\t\", \"\\n\", and \"\\r\"}}", "apiwarn-checktoken-percentencoding": "{{doc-apierror}}", + "apiwarn-compare-no-next": "{{doc-apierror}}\n\nParameters:\n* $1 - Title of the page.\n* $2 - Revision ID.", + "apiwarn-compare-no-prev": "{{doc-apierror}}\n\nParameters:\n* $1 - Title of the page.\n* $2 - Revision ID.", "apiwarn-compare-nocontentmodel": "{{doc-apierror}}\n\nParameters:\n* $1 - Content model being assumed.", "apiwarn-deprecation-deletedrevs": "{{doc-apierror}}", "apiwarn-deprecation-httpsexpected": "{{doc-apierror}}", diff --git a/tests/phpunit/includes/api/ApiComparePagesTest.php b/tests/phpunit/includes/api/ApiComparePagesTest.php index 3709c2195a..e7be4672b5 100644 --- a/tests/phpunit/includes/api/ApiComparePagesTest.php +++ b/tests/phpunit/includes/api/ApiComparePagesTest.php @@ -553,6 +553,62 @@ class ApiComparePagesTest extends ApiTestCase { ] ], ], + 'Relative diff, no prev' => [ + [ + 'fromrev' => '{{REPL:revA1}}', + 'torelative' => 'prev', + 'prop' => 'ids|rel|diff|title|user|comment', + ], + [ + 'warnings' => [ + [ + 'code' => 'compare-no-prev', + 'module' => 'compare', + ], + ], + 'compare' => [ + 'toid' => '{{REPL:pageA}}', + 'torevid' => '{{REPL:revA1}}', + 'tons' => 0, + 'totitle' => 'ApiComparePagesTest A', + 'touser' => '{{REPL:creator}}', + 'touserid' => '{{REPL:creatorid}}', + 'tocomment' => 'Test for ApiComparePagesTest: A 1', + 'toparsedcomment' => 'Test for ApiComparePagesTest: A 1', + 'next' => '{{REPL:revA2}}', + 'body' => 'Line 1:' . "\n" + . 'Line 1:' . "\n" + . '−
 
+
A 1
' . "\n", + ], + ], + ], + 'Relative diff, no next' => [ + [ + 'fromrev' => '{{REPL:revA4}}', + 'torelative' => 'next', + 'prop' => 'ids|rel|diff|title|user|comment', + ], + [ + 'warnings' => [ + [ + 'code' => 'compare-no-next', + 'module' => 'compare', + ], + ], + 'compare' => [ + 'fromid' => '{{REPL:pageA}}', + 'fromrevid' => '{{REPL:revA4}}', + 'fromns' => 0, + 'fromtitle' => 'ApiComparePagesTest A', + 'fromuser' => '{{REPL:creator}}', + 'fromuserid' => '{{REPL:creatorid}}', + 'fromcomment' => 'Test for ApiComparePagesTest: A 4', + 'fromparsedcomment' => 'Test for ApiComparePagesTest: A 4', + 'prev' => '{{REPL:revA3}}', + 'body' => '', + ], + ], + ], 'Diff for specific slots' => [ // @todo Use a page with multiple slots here [ @@ -874,24 +930,6 @@ class ApiComparePagesTest extends ApiTestCase { [], 'missingcontent' ], - 'Error, Relative diff, no prev' => [ - [ - 'fromrev' => '{{REPL:revA1}}', - 'torelative' => 'prev', - 'prop' => 'ids', - ], - [], - 'baddiff' - ], - 'Error, Relative diff, no next' => [ - [ - 'fromrev' => '{{REPL:revA4}}', - 'torelative' => 'next', - 'prop' => 'ids', - ], - [], - 'baddiff' - ], 'Error, section diff with no revision' => [ [ 'fromtitle' => 'ApiComparePagesTest F', -- 2.20.1