pager: Fix navigations when date range is set

The main idea here is to make ReverseChronologicalPager not mess up
mOffset which IndexPager uses, store the offset in another variable
and add separate query conditions.
Also, make RangeChronologicalPager more cooperative with its parent
class, avoid unnecessary inconsistency.

Bug: T228431
Change-Id: Icf76c946770aacee5b038522c066fca33ed1546f

7 files changed, 128 insertions, 142 deletions

diff --git a/includes/actions/pagers/HistoryPager.php b/includes/actions/pagers/HistoryPager.php
index 9b7a9c43c2b0..56b34999b669 100644
--- a/includes/actions/pagers/HistoryPager.php
+++ b/includes/actions/pagers/HistoryPager.php
@@ -675,15 +675,6 @@ class HistoryPager extends ReverseChronologicalPager {
 	}
 
 	/**
-	 * @inheritDoc
-	 */
-	public function getDefaultQuery() {
-		parent::getDefaultQuery();
-		unset( $this->mDefaultQuery['date-range-to'] );
-		return $this->mDefaultQuery;
-	}
-
-	/**
 	 * Set the "prevent clickjacking" flag; for example if a write operation
 	 * if possible from the generated HTML.
 	 * @param bool $flag
diff --git a/includes/pager/RangeChronologicalPager.php b/includes/pager/RangeChronologicalPager.php
index c20e5d695ee6..4c135d037305 100644
--- a/includes/pager/RangeChronologicalPager.php
+++ b/includes/pager/RangeChronologicalPager.php
@@ -27,8 +27,8 @@ use Wikimedia\Timestamp\TimestampException;
  */
 abstract class RangeChronologicalPager extends ReverseChronologicalPager {
 
-	/** @var string[] */
-	protected $rangeConds = [];
+	/** @var string */
+	protected $startOffset;
 
 	/**
 	 * Set and return a date range condition using timestamps provided by the user.
@@ -38,73 +38,46 @@ abstract class RangeChronologicalPager extends ReverseChronologicalPager {
 	 *
 	 * @stable to override
 	 *
-	 * @param string $startStamp Timestamp of the beginning of the date range (or empty)
-	 * @param string $endStamp Timestamp of the end of the date range (or empty)
+	 * @param string $startTime Timestamp of the beginning of the date range (or empty)
+	 * @param string $endTime Timestamp of the end of the date range (or empty)
 	 * @return array|null Database conditions to satisfy the specified date range
 	 *     or null if dates are invalid
 	 */
-	public function getDateRangeCond( $startStamp, $endStamp ) {
-		$this->rangeConds = [];
+	public function getDateRangeCond( $startTime, $endTime ) {
+		// Construct the conds array for compatibility with callers (if any)
+		$rangeConds = [];
 
+		// This is a chronological pager, so the first column should be some kind of timestamp
+		$timestampField = is_array( $this->mIndexField ) ? $this->mIndexField[0] : $this->mIndexField;
 		try {
-			// This is a chronological pager, so the first column should be some kind of timestamp
-			$timestampField = is_array( $this->mIndexField ) ? $this->mIndexField[0] : $this->mIndexField;
-			if ( $startStamp !== '' ) {
-				$startTimestamp = MWTimestamp::getInstance( $startStamp );
-				$startOffset = $this->mDb->timestamp( $startTimestamp->getTimestamp() );
-				$this->rangeConds[] = $timestampField . '>=' . $this->mDb->addQuotes( $startOffset );
+			if ( $startTime !== '' ) {
+				$startTimestamp = MWTimestamp::getInstance( $startTime );
+				$this->startOffset = $this->mDb->timestamp( $startTimestamp->getTimestamp() );
+				$rangeConds[] = $this->mDb->buildComparison( '>=', [ $timestampField => $this->startOffset ] );
 			}
 
-			if ( $endStamp !== '' ) {
-				$endTimestamp = MWTimestamp::getInstance( $endStamp );
-				$endOffset = $this->mDb->timestamp( $endTimestamp->getTimestamp() );
-				$this->rangeConds[] = $timestampField . '<=' . $this->mDb->addQuotes( $endOffset );
+			if ( $endTime !== '' ) {
+				$endTimestamp = MWTimestamp::getInstance( $endTime );
+				// Turned to use '<' for consistency with the parent class,
+				// add one second for compatibility with existing use cases
+				$endTimestamp->timestamp = $endTimestamp->timestamp->modify( '+1 second' );
+				$this->endOffset = $this->mDb->timestamp( $endTimestamp->getTimestamp() );
+				$rangeConds[] = $this->mDb->buildComparison( '<', [ $timestampField => $this->endOffset ] );
 
 				// populate existing variables for compatibility with parent
 				$this->mYear = (int)$endTimestamp->format( 'Y' );
 				$this->mMonth = (int)$endTimestamp->format( 'm' );
 				$this->mDay = (int)$endTimestamp->format( 'd' );
-				$this->mOffset = $endOffset;
 			}
 		} catch ( TimestampException $ex ) {
 			return null;
 		}
 
-		return $this->rangeConds;
+		return $rangeConds;
 	}
 
 	/**
-	 * Takes ReverseChronologicalPager::getDateCond parameters and repurposes
-	 * them to work with timestamp-based getDateRangeCond.
-	 *
-	 * @stable to override
-	 *
-	 * @param int $year Year up to which we want revisions
-	 * @param int $month Month up to which we want revisions
-	 * @param int $day [optional] Day up to which we want revisions. Default is end of month.
-	 * @return string|null Timestamp or null if year and month are false/invalid
-	 */
-	public function getDateCond( $year, $month, $day = -1 ) {
-		// run through getDateRangeCond so rangeConds, mOffset, ... are set
-		$legacyTimestamp = self::getOffsetDate( $year, $month, $day );
-		// ReverseChronologicalPager uses strict inequality for the end date ('<'),
-		// but this class uses '<=' and expects extending classes to handle modifying the end date.
-		// Therefore, we need to subtract one second from the output of getOffsetDate to make it
-		// work with the '<=' inequality used in this class.
-		$legacyTimestamp->timestamp = $legacyTimestamp->timestamp->modify( '-1 second' );
-		$this->getDateRangeCond( '', $legacyTimestamp->getTimestamp( TS_MW ) );
-		return $this->mOffset;
-	}
-
-	/**
-	 * Build variables to use by the database wrapper.
-	 *
-	 * @stable to override
-	 *
-	 * @param string $offset Index offset, inclusive
-	 * @param int $limit Exact query limit
-	 * @param bool $order IndexPager::QUERY_ASCENDING or IndexPager::QUERY_DESCENDING
-	 * @return array
+	 * @inheritDoc
 	 */
 	protected function buildQueryInfo( $offset, $limit, $order ) {
 		[ $tables, $fields, $conds, $fname, $options, $join_conds ] = parent::buildQueryInfo(
@@ -112,9 +85,10 @@ abstract class RangeChronologicalPager extends ReverseChronologicalPager {
 			$limit,
 			$order
 		);
-
-		if ( $this->rangeConds ) {
-			$conds = array_merge( $conds, $this->rangeConds );
+		// End of the range has been added by ReverseChronologicalPager
+		if ( $this->startOffset ) {
+			$timestampField = is_array( $this->mIndexField ) ? $this->mIndexField[0] : $this->mIndexField;
+			$conds[] = $this->mDb->buildComparison( '>=', [ $timestampField => $this->startOffset ] );
 		}
 
 		return [ $tables, $fields, $conds, $fname, $options, $join_conds ];
diff --git a/includes/pager/ReverseChronologicalPager.php b/includes/pager/ReverseChronologicalPager.php
index ddf23b064df3..5a189bd57ab3 100644
--- a/includes/pager/ReverseChronologicalPager.php
+++ b/includes/pager/ReverseChronologicalPager.php
@@ -38,6 +38,8 @@ abstract class ReverseChronologicalPager extends IndexPager {
 	public $mDay;
 	/** @var string */
 	private $lastHeaderDate;
+	/** @var string */
+	protected $endOffset;
 
 	/**
 	 * @param string $date
@@ -169,7 +171,7 @@ abstract class ReverseChronologicalPager extends IndexPager {
 	}
 
 	/**
-	 * Set and return the mOffset timestamp such that we can get all revisions with
+	 * Set and return the offset timestamp such that we can get all revisions with
 	 * a timestamp up to the specified parameters.
 	 *
 	 * @stable to override
@@ -185,7 +187,7 @@ abstract class ReverseChronologicalPager extends IndexPager {
 		$day = (int)$day;
 
 		// Basic validity checks for year and month
-		// If year and month are invalid, don't update the mOffset
+		// If year and month are invalid, don't update the offset
 		if ( $year <= 0 && ( $month <= 0 || $month >= 13 ) ) {
 			return null;
 		}
@@ -200,17 +202,18 @@ abstract class ReverseChronologicalPager extends IndexPager {
 			$this->mYear = (int)$selectedDate->format( 'Y' );
 			$this->mMonth = (int)$selectedDate->format( 'm' );
 			$this->mDay = (int)$selectedDate->format( 'd' );
-			$this->mOffset = $this->mDb->timestamp( $timestamp->getTimestamp() );
+			// Don't mess with mOffset which IndexPager uses
+			$this->endOffset = $this->mDb->timestamp( $timestamp->getTimestamp() );
 		} catch ( TimestampException $e ) {
 			// Invalid user provided timestamp (T149257)
 			return null;
 		}
 
-		return $this->mOffset;
+		return $this->endOffset;
 	}
 
 	/**
-	 * Core logic of determining the mOffset timestamp such that we can get all items with
+	 * Core logic of determining the offset timestamp such that we can get all items with
 	 * a timestamp up to the specified parameters. Given parameters for a day up to which to get
 	 * items, this function finds the timestamp of the day just after the end of the range for use
 	 * in an database strict inequality filter.
@@ -286,4 +289,21 @@ abstract class ReverseChronologicalPager extends IndexPager {
 
 		return MWTimestamp::getInstance( "{$ymd}000000" );
 	}
+
+	/**
+	 * @inheritDoc
+	 */
+	protected function buildQueryInfo( $offset, $limit, $order ) {
+		[ $tables, $fields, $conds, $fname, $options, $join_conds ] = parent::buildQueryInfo(
+			$offset,
+			$limit,
+			$order
+		);
+		if ( $this->endOffset ) {
+			$timestampField = is_array( $this->mIndexField ) ? $this->mIndexField[0] : $this->mIndexField;
+			$conds[] = $this->mDb->buildComparison( '<', [ $timestampField => $this->endOffset ] );
+		}
+
+		return [ $tables, $fields, $conds, $fname, $options, $join_conds ];
+	}
 }
diff --git a/includes/specials/SpecialContributions.php b/includes/specials/SpecialContributions.php
index c7a5c23ed0d9..304e90d10f1f 100644
--- a/includes/specials/SpecialContributions.php
+++ b/includes/specials/SpecialContributions.php
@@ -196,15 +196,10 @@ class SpecialContributions extends IncludableSpecialPage {
 			$this->opts['bot'] = '1';
 		}
 
-		$skip = $request->getText( 'offset' ) || $request->getText( 'dir' ) == 'prev';
-		# Offset overrides year/month selection
-		if ( !$skip ) {
-			$this->opts['year'] = $request->getIntOrNull( 'year' );
-			$this->opts['month'] = $request->getIntOrNull( 'month' );
-
-			$this->opts['start'] = $request->getVal( 'start' );
-			$this->opts['end'] = $request->getVal( 'end' );
-		}
+		$this->opts['year'] = $request->getIntOrNull( 'year' );
+		$this->opts['month'] = $request->getIntOrNull( 'month' );
+		$this->opts['start'] = $request->getVal( 'start' );
+		$this->opts['end'] = $request->getVal( 'end' );
 
 		$id = 0;
 		if ( ExternalUserNames::isExternal( $target ) ) {
diff --git a/includes/specials/SpecialLog.php b/includes/specials/SpecialLog.php
index 6a2fb46b6322..62f129a54445 100644
--- a/includes/specials/SpecialLog.php
+++ b/includes/specials/SpecialLog.php
@@ -114,12 +114,6 @@ class SpecialLog extends SpecialPage {
 			}
 		}
 
-		# Don't let the user get stuck with a certain date
-		if ( $opts->getValue( 'offset' ) || $opts->getValue( 'dir' ) == 'prev' ) {
-			$opts->setValue( 'year', '' );
-			$opts->setValue( 'month', '' );
-		}
-
 		// If the user doesn't have the right permission to view the specific
 		// log type, throw a PermissionsError
 		// If the log type is invalid, just show all public logs
diff --git a/tests/phpunit/includes/pager/RangeChronologicalPagerTest.php b/tests/phpunit/includes/pager/RangeChronologicalPagerTest.php
index 0ac895d54525..58c5fb30d09b 100644
--- a/tests/phpunit/includes/pager/RangeChronologicalPagerTest.php
+++ b/tests/phpunit/includes/pager/RangeChronologicalPagerTest.php
@@ -7,7 +7,7 @@
  *
  * @author Geoffrey Mon <geofbot@gmail.com>
  */
-class RangeChronologicalPagerTest extends MediaWikiLangTestCase {
+class RangeChronologicalPagerTest extends MediaWikiIntegrationTestCase {
 
 	/**
 	 * @covers RangeChronologicalPager::getDateCond
@@ -26,14 +26,14 @@ class RangeChronologicalPagerTest extends MediaWikiLangTestCase {
 	 */
 	public function getDateCondProvider() {
 		return [
-			[ 2016, 12, 5, '20161205235959' ],
-			[ 2016, 12, 31, '20161231235959' ],
-			[ 2016, 12, 1337, '20161231235959' ],
-			[ 2016, 1337, 1337, '20161231235959' ],
-			[ 2016, 1337, -1, '20161231235959' ],
-			[ 2016, 12, 32, '20161231235959' ],
-			[ 2016, 12, -1, '20161231235959' ],
-			[ 2016, -1, -1, '20161231235959' ],
+			[ 2016, 12, 5, '20161206000000' ],
+			[ 2016, 12, 31, '20170101000000' ],
+			[ 2016, 12, 1337, '20170101000000' ],
+			[ 2016, 1337, 1337, '20170101000000' ],
+			[ 2016, 1337, -1, '20170101000000' ],
+			[ 2016, 12, 32, '20170101000000' ],
+			[ 2016, 12, -1, '20170101000000' ],
+			[ 2016, -1, -1, '20170101000000' ],
 		];
 	}
 
@@ -55,24 +55,24 @@ class RangeChronologicalPagerTest extends MediaWikiLangTestCase {
 		return [
 			[
 				'20161201000000',
-				'20161203000000',
+				'20161202235959',
 				[
-					'>=' . $db->addQuotes( $db->timestamp( '20161201000000' ) ),
-					'<=' . $db->addQuotes( $db->timestamp( '20161203000000' ) ),
+					$db->buildComparison( '>=', [ '' => $db->timestamp( '20161201000000' ) ] ),
+					$db->buildComparison( '<', [ '' => $db->timestamp( '20161203000000' ) ] ),
 				],
 			],
 			[
 				'',
-				'20161203000000',
+				'20161202235959',
 				[
-					'<=' . $db->addQuotes( $db->timestamp( '20161203000000' ) ),
+					$db->buildComparison( '<', [ '' => $db->timestamp( '20161203000000' ) ] ),
 				],
 			],
 			[
 				'20161201000000',
 				'',
 				[
-					'>=' . $db->addQuotes( $db->timestamp( '20161201000000' ) ),
+					$db->buildComparison( '>=', [ '' => $db->timestamp( '20161201000000' ) ] ),
 				],
 			],
 			[ '', '', [] ],
diff --git a/tests/phpunit/includes/pager/ReverseChronologicalPagerTest.php b/tests/phpunit/includes/pager/ReverseChronologicalPagerTest.php
index 6959bfc9d818..c61b60a4b078 100644
--- a/tests/phpunit/includes/pager/ReverseChronologicalPagerTest.php
+++ b/tests/phpunit/includes/pager/ReverseChronologicalPagerTest.php
@@ -1,5 +1,7 @@
 <?php
 
+use Wikimedia\TestingAccessWrapper;
+
 /**
  * Test class for ReverseChronologicalPagerTest methods.
  *
@@ -7,70 +9,80 @@
  *
  * @author Geoffrey Mon <geofbot@gmail.com>
  */
-class ReverseChronologicalPagerTest extends MediaWikiLangTestCase {
+class ReverseChronologicalPagerTest extends MediaWikiIntegrationTestCase {
 
 	/**
 	 * @covers ReverseChronologicalPager::getDateCond
+	 * @dataProvider provideGetDateCond
 	 */
-	public function testGetDateCond() {
+	public function testGetDateCond( $params, $expected ) {
 		$pager = $this->getMockForAbstractClass( ReverseChronologicalPager::class );
-		$timestamp = MWTimestamp::getInstance();
+		$pagerWrapper = TestingAccessWrapper::newFromObject( $pager );
 		$db = wfGetDB( DB_PRIMARY );
 
-		$currYear = $timestamp->format( 'Y' );
-		$currMonth = $timestamp->format( 'n' );
-
-		// Test that getDateCond sets and returns mOffset
-		$this->assertEquals( $pager->getDateCond( 2006, 6 ), $pager->mOffset );
-
-		// Test year and month
-		$pager->getDateCond( 2006, 6 );
-		$this->assertEquals( $pager->mOffset, $db->timestamp( '20060701000000' ) );
-
-		// Test year, month, and day
-		$pager->getDateCond( 2006, 6, 5 );
-		$this->assertEquals( $pager->mOffset, $db->timestamp( '20060606000000' ) );
-
-		// Test month overflow into the next year
-		$pager->getDateCond( 2006, 12 );
-		$this->assertEquals( $pager->mOffset, $db->timestamp( '20070101000000' ) );
-
-		// Test day overflow to the next month
-		$pager->getDateCond( 2006, 6, 30 );
-		$this->assertEquals( $pager->mOffset, $db->timestamp( '20060701000000' ) );
+		$pager->getDateCond( ...$params );
+		$this->assertEquals( $pagerWrapper->endOffset, $db->timestamp( $expected ) );
+	}
 
-		// Test invalid month (should use end of year)
-		$pager->getDateCond( 2006, -1 );
-		$this->assertEquals( $pager->mOffset, $db->timestamp( '20070101000000' ) );
+	/**
+	 * Data provider in description => [ [ param1, ... ], expected output ] format
+	 */
+	public function provideGetDateCond() {
+		yield 'Test year and month' => [
+			[ 2006, 6 ], '20060701000000'
+		];
+		yield 'Test year, month, and day' => [
+			[ 2006, 6, 5 ], '20060606000000'
+		];
+		yield 'Test month overflow into the next year' => [
+			[ 2006, 12 ], '20070101000000'
+		];
+		yield 'Test day overflow to the next month' => [
+			[ 2006, 6, 30 ], '20060701000000'
+		];
+		yield 'Test invalid month (should use end of year)' => [
+			[ 2006, -1 ], '20070101000000'
+		];
+		yield 'Test invalid day (should use end of month)' => [
+			[ 2006, 6, 1337 ], '20060701000000'
+		];
+		yield 'Test last day of year' => [
+			[ 2006, 12, 31 ], '20070101000000'
+		];
+		yield 'Test invalid day that overflows to next year' => [
+			[ 2006, 12, 32 ], '20070101000000'
+		];
+		yield '3-digit year, T287621' => [
+			[ 720, 1, 5 ], '07200106000000'
+		];
+		yield 'Y2K38 bug' => [
+			[ 2042, 1, 5 ], '20320101000000'
+		];
+	}
 
-		// Test invalid day (should use end of month)
-		$pager->getDateCond( 2006, 6, 1337 );
-		$this->assertEquals( $pager->mOffset, $db->timestamp( '20060701000000' ) );
+	/**
+	 * @covers ReverseChronologicalPager::getDateCond
+	 */
+	public function testGetDateCondSpecial() {
+		$pager = $this->getMockForAbstractClass( ReverseChronologicalPager::class );
+		$pagerWrapper = TestingAccessWrapper::newFromObject( $pager );
+		$timestamp = MWTimestamp::getInstance();
+		$db = wfGetDB( DB_PRIMARY );
 
-		// Test last day of year
-		$pager->getDateCond( 2006, 12, 31 );
-		$this->assertEquals( $pager->mOffset, $db->timestamp( '20070101000000' ) );
+		$currYear = $timestamp->format( 'Y' );
+		$currMonth = $timestamp->format( 'n' );
 
-		// Test invalid day that overflows to next year
-		$pager->getDateCond( 2006, 12, 32 );
-		$this->assertEquals( $pager->mOffset, $db->timestamp( '20070101000000' ) );
+		// Test that getDateCond sets and returns offset
+		$this->assertEquals( $pager->getDateCond( 2006, 6 ), $pagerWrapper->endOffset );
 
 		// Test month past current month (should use previous year)
 		if ( $currMonth < 5 ) {
 			$pager->getDateCond( -1, 5 );
-			$this->assertEquals( $pager->mOffset, $db->timestamp( $currYear - 1 . '0601000000' ) );
+			$this->assertEquals( $pagerWrapper->endOffset, $db->timestamp( $currYear - 1 . '0601000000' ) );
 		}
 		if ( $currMonth < 12 ) {
 			$pager->getDateCond( -1, 12 );
-			$this->assertEquals( $pager->mOffset, $db->timestamp( $currYear . '0101000000' ) );
+			$this->assertEquals( $pagerWrapper->endOffset, $db->timestamp( $currYear . '0101000000' ) );
 		}
-
-		// 3-digit year, T287621
-		$pager->getDateCond( 720, 1, 5 );
-		$this->assertEquals( $pager->mOffset, $db->timestamp( '07200106000000' ) );
-
-		// Y2K38 bug
-		$pager->getDateCond( 2042, 1, 5 );
-		$this->assertEquals( $pager->mOffset, $db->timestamp( '20320101000000' ) );
 	}
 }