From: Aaron Schulz Date: Wed, 6 Mar 2019 18:51:43 +0000 (-0800) Subject: Make IndexPager query direction code more readable X-Git-Tag: 1.34.0-rc.0~2608^2 X-Git-Url: http://git.cyclocoop.org/%22%20.%20generer_url_aide%28?a=commitdiff_plain;h=46db79baed031be50b447cca4a0bca6629f67c4a;p=lhc%2Fweb%2Fwiklou.git Make IndexPager query direction code more readable Rename $descending variable in doQuery() to $order, along with the $descending argument to reallyDoQuery() and buildQueryInfo(). Use new IndexPager::QUERY_* constants for checking and inverting such values. Fix the comments so that they do not imply the opposite of what is true anymore. For compatibility, the constants are boolean values such that any subclass defining reallyDoQuery()/buildQueryInfo() can use that argument the same way as before. Change-Id: I912d3678c755c25463a2fadbec6888f3a87d4215 --- diff --git a/includes/pager/IndexPager.php b/includes/pager/IndexPager.php index ce1d2d076c..dc7dd269de 100644 --- a/includes/pager/IndexPager.php +++ b/includes/pager/IndexPager.php @@ -67,14 +67,16 @@ use Wikimedia\Rdbms\IDatabase; * @ingroup Pager */ abstract class IndexPager extends ContextSource implements Pager { - /** - * Constants for the $mDefaultDirection field. - * - * These are boolean for historical reasons and should stay boolean for backwards-compatibility. - */ + /** Backwards-compatible constant for $mDefaultDirection field (do not change) */ const DIR_ASCENDING = false; + /** Backwards-compatible constant for $mDefaultDirection field (do not change) */ const DIR_DESCENDING = true; + /** Backwards-compatible constant for reallyDoQuery() (do not change) */ + const QUERY_ASCENDING = true; + /** Backwards-compatible constant for reallyDoQuery() (do not change) */ + const QUERY_DESCENDING = false; + /** @var WebRequest */ public $mRequest; /** @var int[] List of default entry limit options to be presented to clients */ @@ -208,11 +210,13 @@ abstract class IndexPager extends ContextSource implements Pager { public function doQuery() { # Use the child class name for profiling $fname = __METHOD__ . ' (' . static::class . ')'; + /** @noinspection PhpUnusedLocalVariableInspection */ $section = Profiler::instance()->scopedProfileIn( $fname ); - $descending = $this->mIsBackwards - ? ( $this->mDefaultDirection === self::DIR_DESCENDING ) - : ( $this->mDefaultDirection === self::DIR_ASCENDING ); + $defaultOrder = ( $this->mDefaultDirection === self::DIR_ASCENDING ) + ? self::QUERY_ASCENDING + : self::QUERY_DESCENDING; + $order = $this->mIsBackwards ? self::oppositeOrder( $defaultOrder ) : $defaultOrder; # Plus an extra row so that we can tell the "next" link should be shown $queryLimit = $this->mLimit + 1; @@ -225,14 +229,15 @@ abstract class IndexPager extends ContextSource implements Pager { // direction see if we get a row. $oldIncludeOffset = $this->mIncludeOffset; $this->mIncludeOffset = !$this->mIncludeOffset; - $isFirst = !$this->reallyDoQuery( $this->mOffset, 1, !$descending )->numRows(); + $oppositeOrder = self::oppositeOrder( $order ); + $isFirst = !$this->reallyDoQuery( $this->mOffset, 1, $oppositeOrder )->numRows(); $this->mIncludeOffset = $oldIncludeOffset; } $this->mResult = $this->reallyDoQuery( $this->mOffset, $queryLimit, - $descending + $order ); $this->extractResultInfo( $isFirst, $queryLimit, $this->mResult ); @@ -242,6 +247,16 @@ abstract class IndexPager extends ContextSource implements Pager { $this->mResult->rewind(); // Paranoia } + /** + * @param bool $order One of the IndexPager::QUERY_* class constants + * @return bool The opposite query order as an IndexPager::QUERY_ constant + */ + final protected static function oppositeOrder( $order ) { + return ( $order === self::QUERY_ASCENDING ) + ? self::QUERY_DESCENDING + : self::QUERY_ASCENDING; + } + /** * @return IResultWrapper The result wrapper. */ @@ -364,17 +379,18 @@ abstract class IndexPager extends ContextSource implements Pager { } /** - * Do a query with specified parameters, rather than using the object - * context + * Do a query with specified parameters, rather than using the object context + * + * @note For b/c, query direction is true for ascending and false for descending * * @param string $offset Index offset, inclusive * @param int $limit Exact query limit - * @param bool $descending Query direction, false for ascending, true for descending + * @param bool $order IndexPager::QUERY_ASCENDING or IndexPager::QUERY_DESCENDING * @return IResultWrapper */ - public function reallyDoQuery( $offset, $limit, $descending ) { + public function reallyDoQuery( $offset, $limit, $order ) { list( $tables, $fields, $conds, $fname, $options, $join_conds ) = - $this->buildQueryInfo( $offset, $limit, $descending ); + $this->buildQueryInfo( $offset, $limit, $order ); return $this->mDb->select( $tables, $fields, $conds, $fname, $options, $join_conds ); } @@ -382,12 +398,14 @@ abstract class IndexPager extends ContextSource implements Pager { /** * Build variables to use by the database wrapper. * + * @note For b/c, query direction is true for ascending and false for descending + * * @param string $offset Index offset, inclusive * @param int $limit Exact query limit - * @param bool $descending Query direction, false for ascending, true for descending + * @param bool $order IndexPager::QUERY_ASCENDING or IndexPager::QUERY_DESCENDING * @return array */ - protected function buildQueryInfo( $offset, $limit, $descending ) { + protected function buildQueryInfo( $offset, $limit, $order ) { $fname = __METHOD__ . ' (' . $this->getSqlComment() . ')'; $info = $this->getQueryInfo(); $tables = $info['tables']; @@ -396,7 +414,7 @@ abstract class IndexPager extends ContextSource implements Pager { $options = $info['options'] ?? []; $join_conds = $info['join_conds'] ?? []; $sortColumns = array_merge( [ $this->mIndexField ], $this->mExtraSortFields ); - if ( $descending ) { + if ( $order === self::QUERY_ASCENDING ) { $options['ORDER BY'] = $sortColumns; $operator = $this->mIncludeOffset ? '>=' : '>'; } else { diff --git a/includes/pager/RangeChronologicalPager.php b/includes/pager/RangeChronologicalPager.php index bf36983530..7fe364feb0 100644 --- a/includes/pager/RangeChronologicalPager.php +++ b/includes/pager/RangeChronologicalPager.php @@ -93,14 +93,14 @@ abstract class RangeChronologicalPager extends ReverseChronologicalPager { * * @param string $offset Index offset, inclusive * @param int $limit Exact query limit - * @param bool $descending Query direction, false for ascending, true for descending + * @param bool $order IndexPager::QUERY_ASCENDING or IndexPager::QUERY_DESCENDING * @return array */ - protected function buildQueryInfo( $offset, $limit, $descending ) { + protected function buildQueryInfo( $offset, $limit, $order ) { list( $tables, $fields, $conds, $fname, $options, $join_conds ) = parent::buildQueryInfo( $offset, $limit, - $descending + $order ); if ( $this->rangeConds ) { diff --git a/includes/specials/pagers/ActiveUsersPager.php b/includes/specials/pagers/ActiveUsersPager.php index 3e1a869f65..5dbdba08f1 100644 --- a/includes/specials/pagers/ActiveUsersPager.php +++ b/includes/specials/pagers/ActiveUsersPager.php @@ -161,11 +161,11 @@ class ActiveUsersPager extends UsersPager { ]; } - protected function buildQueryInfo( $offset, $limit, $descending ) { + protected function buildQueryInfo( $offset, $limit, $order ) { $fname = __METHOD__ . ' (' . $this->getSqlComment() . ')'; $sortColumns = array_merge( [ $this->mIndexField ], $this->mExtraSortFields ); - if ( $descending ) { + if ( $order === self::QUERY_ASCENDING ) { $dir = 'ASC'; $orderBy = $sortColumns; $operator = $this->mIncludeOffset ? '>=' : '>'; diff --git a/includes/specials/pagers/AllMessagesTablePager.php b/includes/specials/pagers/AllMessagesTablePager.php index f045333130..8120417b46 100644 --- a/includes/specials/pagers/AllMessagesTablePager.php +++ b/includes/specials/pagers/AllMessagesTablePager.php @@ -167,24 +167,25 @@ class AllMessagesTablePager extends TablePager { } /** - * This function normally does a database query to get the results; we need + * This function normally does a database query to get the results; we need * to make a pretend result using a FakeResultWrapper. * @param string $offset * @param int $limit - * @param bool $descending + * @param bool $order * @return FakeResultWrapper */ - function reallyDoQuery( $offset, $limit, $descending ) { + function reallyDoQuery( $offset, $limit, $order ) { + $asc = ( $order === self::QUERY_ASCENDING ); $result = new FakeResultWrapper( [] ); - $messageNames = $this->getAllMessages( $descending ); + $messageNames = $this->getAllMessages( $order ); $statuses = self::getCustomisedStatuses( $messageNames, $this->langcode, $this->foreign ); $count = 0; foreach ( $messageNames as $key ) { $customised = isset( $statuses['pages'][$key] ); if ( $customised !== $this->custom && - ( $descending && ( $key < $offset || !$offset ) || !$descending && $key > $offset ) && + ( $asc && ( $key < $offset || !$offset ) || !$asc && $key > $offset ) && ( ( $this->prefix && preg_match( $this->prefix, $key ) ) || $this->prefix === false ) ) { $actual = wfMessage( $key )->inLanguage( $this->lang )->plain(); diff --git a/includes/specials/pagers/ContribsPager.php b/includes/specials/pagers/ContribsPager.php index 1220e8617a..382ba2fd2b 100644 --- a/includes/specials/pagers/ContribsPager.php +++ b/includes/specials/pagers/ContribsPager.php @@ -160,14 +160,14 @@ class ContribsPager extends RangeChronologicalPager { * * @param string $offset Index offset, inclusive * @param int $limit Exact query limit - * @param bool $descending Query direction, false for ascending, true for descending + * @param bool $order IndexPager::QUERY_ASCENDING or IndexPager::QUERY_DESCENDING * @return IResultWrapper */ - function reallyDoQuery( $offset, $limit, $descending ) { + function reallyDoQuery( $offset, $limit, $order ) { list( $tables, $fields, $conds, $fname, $options, $join_conds ) = $this->buildQueryInfo( $offset, $limit, - $descending + $order ); /* @@ -193,7 +193,7 @@ class ContribsPager extends RangeChronologicalPager { ) ]; Hooks::run( 'ContribsPager::reallyDoQuery', - [ &$data, $this, $offset, $limit, $descending ] + [ &$data, $this, $offset, $limit, $order ] ); $result = []; @@ -207,7 +207,7 @@ class ContribsPager extends RangeChronologicalPager { } // sort results - if ( $descending ) { + if ( $order === self::QUERY_ASCENDING ) { ksort( $result ); } else { krsort( $result ); diff --git a/includes/specials/pagers/DeletedContribsPager.php b/includes/specials/pagers/DeletedContribsPager.php index fa0fbf3520..56b799bb7c 100644 --- a/includes/specials/pagers/DeletedContribsPager.php +++ b/includes/specials/pagers/DeletedContribsPager.php @@ -115,17 +115,17 @@ class DeletedContribsPager extends IndexPager { * * @param string $offset Index offset, inclusive * @param int $limit Exact query limit - * @param bool $descending Query direction, false for ascending, true for descending + * @param bool $order IndexPager::QUERY_ASCENDING or IndexPager::QUERY_DESCENDING * @return IResultWrapper */ - function reallyDoQuery( $offset, $limit, $descending ) { - $data = [ parent::reallyDoQuery( $offset, $limit, $descending ) ]; + function reallyDoQuery( $offset, $limit, $order ) { + $data = [ parent::reallyDoQuery( $offset, $limit, $order ) ]; // This hook will allow extensions to add in additional queries, nearly // identical to ContribsPager::reallyDoQuery. Hooks::run( 'DeletedContribsPager::reallyDoQuery', - [ &$data, $this, $offset, $limit, $descending ] + [ &$data, $this, $offset, $limit, $order ] ); $result = []; @@ -139,7 +139,7 @@ class DeletedContribsPager extends IndexPager { } // sort results - if ( $descending ) { + if ( $order === self::QUERY_ASCENDING ) { ksort( $result ); } else { krsort( $result ); diff --git a/includes/specials/pagers/ImageListPager.php b/includes/specials/pagers/ImageListPager.php index 1794362461..486b0ec910 100644 --- a/includes/specials/pagers/ImageListPager.php +++ b/includes/specials/pagers/ImageListPager.php @@ -321,15 +321,15 @@ class ImageListPager extends TablePager { * is descending, so I renamed it to $asc here. * @param int $offset * @param int $limit - * @param bool $asc - * @return array + * @param bool $order IndexPager::QUERY_ASCENDING or IndexPager::QUERY_DESCENDING + * @return FakeResultWrapper * @throws MWException */ - function reallyDoQuery( $offset, $limit, $asc ) { + function reallyDoQuery( $offset, $limit, $order ) { $prevTableName = $this->mTableName; $this->mTableName = 'image'; list( $tables, $fields, $conds, $fname, $options, $join_conds ) = - $this->buildQueryInfo( $offset, $limit, $asc ); + $this->buildQueryInfo( $offset, $limit, $order ); $imageRes = $this->mDb->select( $tables, $fields, $conds, $fname, $options, $join_conds ); $this->mTableName = $prevTableName; @@ -347,13 +347,13 @@ class ImageListPager extends TablePager { $this->mIndexField = 'oi_' . substr( $this->mIndexField, 4 ); list( $tables, $fields, $conds, $fname, $options, $join_conds ) = - $this->buildQueryInfo( $offset, $limit, $asc ); + $this->buildQueryInfo( $offset, $limit, $order ); $oldimageRes = $this->mDb->select( $tables, $fields, $conds, $fname, $options, $join_conds ); $this->mTableName = $prevTableName; $this->mIndexField = $oldIndex; - return $this->combineResult( $imageRes, $oldimageRes, $limit, $asc ); + return $this->combineResult( $imageRes, $oldimageRes, $limit, $order ); } /**