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 ce1d2d0..dc7dd26 100644 (file)
--- 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 bf36983..7fe364f 100644 (file)
--- 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 3e1a869..5dbdba0 100644 (file)
--- 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 f045333..8120417 100644 (file)
--- 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 1220e86..382ba2f 100644 (file)
--- 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 fa0fbf3..56b799b 100644 (file)
--- 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 1794362..486b0ec 100644 (file)
--- 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 );
        }
 
        /**