Matthias Mullie has uploaded a new change for review. https://gerrit.wikimedia.org/r/167785
Change subject: Hygiene: fix feedback from scrutinizer #4 ...................................................................... Hygiene: fix feedback from scrutinizer #4 I ran scrutinizer-ci.com against flow - these are some of the things it came up with. https://scrutinizer-ci.com/g/matthiasmullie/mediawiki-extensions-Flow/inspections/d37be56d-1a25-4c3c-8f2e-f47fc8f3c403/issues/ Change-Id: Icecee81bded92f2b74627a0fbd9d0e245aceccd3 --- M includes/Data/Index.php M includes/Data/Mapper/CachingObjectMapper.php M includes/Data/ObjectLocator.php M includes/Data/ObjectStorage.php M includes/Data/Pager/Pager.php M includes/Data/RecentChanges/RecentChanges.php M includes/Data/Storage/BasicDbStorage.php M includes/Data/Storage/BoardHistoryStorage.php M includes/Data/Storage/PostRevisionStorage.php M includes/Data/Storage/RevisionStorage.php M includes/Data/Utils/RawSql.php M includes/Data/Utils/ResultDuplicator.php M includes/Formatter/AbstractFormatter.php M includes/Formatter/Contributions.php M includes/Formatter/ContributionsQuery.php M includes/Formatter/IRCLineUrlFormatter.php M includes/Formatter/RecentChanges.php M includes/Formatter/RecentChangesQuery.php M includes/Formatter/RevisionFormatter.php 19 files changed, 176 insertions(+), 29 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Flow refs/changes/85/167785/1 diff --git a/includes/Data/Index.php b/includes/Data/Index.php index 88ef47e..ebf7f6c 100644 --- a/includes/Data/Index.php +++ b/includes/Data/Index.php @@ -61,7 +61,7 @@ * are broken by evaluating the second term and so on. * * @todo choose a default sort instead of false? - * @return array|false Sorting order of either 'ASC', 'DESC' or false + * @return array|false Columns to sort on */ function getSort(); diff --git a/includes/Data/Mapper/CachingObjectMapper.php b/includes/Data/Mapper/CachingObjectMapper.php index 99a6b96..7abac0c 100644 --- a/includes/Data/Mapper/CachingObjectMapper.php +++ b/includes/Data/Mapper/CachingObjectMapper.php @@ -26,6 +26,11 @@ protected $fromStorageRow; /** + * @var string[] + */ + protected $primaryKey; + + /** * @var MultiDimArray */ protected $loaded; diff --git a/includes/Data/ObjectLocator.php b/includes/Data/ObjectLocator.php index edb13f3..5067a52 100644 --- a/includes/Data/ObjectLocator.php +++ b/includes/Data/ObjectLocator.php @@ -2,9 +2,10 @@ namespace Flow\Data; -use FormatJson; +use Flow\Exception\FlowException; use Flow\Exception\NoIndexException; use Flow\Model\UUID; +use FormatJson; /** * Denormalized indexes that are query-only. The indexes used here must @@ -33,6 +34,12 @@ */ protected $lifecycleHandlers; + /** + * @param ObjectMapper $mapper + * @param ObjectStorage $storage + * @param Index[] $indexes + * @param LifecycleHandler[] $lifecycleHandlers + */ public function __construct( ObjectMapper $mapper, ObjectStorage $storage, array $indexes = array(), array $lifecycleHandlers = array() ) { $this->mapper = $mapper; $this->storage = $storage; @@ -50,6 +57,10 @@ } public function getIterator() { + if ( !method_exists( $this->storage, 'getIterator' ) ) { + throw new FlowException( 'Storage object of class "' . get_class( $this->storage ) . '" has no iterator.' ); + } + return $this->storage->getIterator(); } @@ -246,7 +257,7 @@ } public function clear() { - // nop, we dont store anything + // nop, we don't store anything } /** diff --git a/includes/Data/ObjectStorage.php b/includes/Data/ObjectStorage.php index 4e02806..fdf6266 100644 --- a/includes/Data/ObjectStorage.php +++ b/includes/Data/ObjectStorage.php @@ -23,7 +23,7 @@ * * @param array $queries list of queries to perform * @param array $options Options to use for all queries - * @return array + * @return array[] Array of results for every query */ function findMulti( array $queries, array $options = array() ); diff --git a/includes/Data/Pager/Pager.php b/includes/Data/Pager/Pager.php index 66c9d13..fddec20 100644 --- a/includes/Data/Pager/Pager.php +++ b/includes/Data/Pager/Pager.php @@ -41,6 +41,11 @@ */ protected $options; + /** + * @var string + */ + protected $offsetKey; + public function __construct( ObjectManager $storage, array $query, array $options ) { // not sure i like this $this->storage = $storage; diff --git a/includes/Data/RecentChanges/RecentChanges.php b/includes/Data/RecentChanges/RecentChanges.php index 03018e6..5954311 100644 --- a/includes/Data/RecentChanges/RecentChanges.php +++ b/includes/Data/RecentChanges/RecentChanges.php @@ -36,6 +36,11 @@ protected $usernames; /** + * @var RecentChangeFactory + */ + protected $rcFactory; + + /** * @param FlowActions $actions * @param UserNameBatch $usernames * @param RecentChangeFactory $rcFactory Creates mw RecentChange instances diff --git a/includes/Data/Storage/BasicDbStorage.php b/includes/Data/Storage/BasicDbStorage.php index 4f13b40..34828c2 100644 --- a/includes/Data/Storage/BasicDbStorage.php +++ b/includes/Data/Storage/BasicDbStorage.php @@ -18,6 +18,22 @@ * Doesn't support auto-increment pk yet */ class BasicDbStorage extends DbStorage { + /** + * @var string + */ + protected $table; + + /** + * @var string[] + */ + protected $primaryKey; + + /** + * @param DbFactory $dbFactory + * @param string $table + * @param string[] $primaryKey + * @throws DataModelException + */ public function __construct( DbFactory $dbFactory, $table, array $primaryKey ) { if ( !$primaryKey ) { throw new DataModelException( 'PK required', 'process-data' ); @@ -69,7 +85,7 @@ $pk = ObjectManager::splitFromRow( $old, $this->primaryKey ); if ( $pk === null ) { $missing = array_diff( $this->primaryKey, array_keys( $old ) ); - throw new DataPersistenceException( 'Row has null primary key: ' . implode( $missing ), 'process-data' ); + throw new DataPersistenceException( 'Row has null primary key: ' . implode( ', ', $missing ), 'process-data' ); } $updates = ObjectManager::calcUpdates( $old, $new ); if ( !$updates ) { @@ -98,7 +114,7 @@ $pk = ObjectManager::splitFromRow( $row, $this->primaryKey ); if ( $pk === null ) { $missing = array_diff( $this->primaryKey, array_keys( $row ) ); - throw new DataPersistenceException( 'Row has null primary key: ' . implode( $missing ), 'process-data' ); + throw new DataPersistenceException( 'Row has null primary key: ' . implode( ', ', $missing ), 'process-data' ); } $pk = $this->preprocessSqlArray( $pk ); diff --git a/includes/Data/Storage/BoardHistoryStorage.php b/includes/Data/Storage/BoardHistoryStorage.php index 2b4219a..19b03dd 100644 --- a/includes/Data/Storage/BoardHistoryStorage.php +++ b/includes/Data/Storage/BoardHistoryStorage.php @@ -26,9 +26,19 @@ $merged = $this->findHeaderHistory( $queries, $options ) + $this->findTopicListHistory( $queries, $options ) + $this->findTopicSummaryHistory( $queries, $options ); - // newest items at the begining of the list + // newest items at the beginning of the list krsort( $merged ); - return RevisionStorage::mergeExternalContent( array( $merged ) ); + + // Merge data from external store & get rid of failures + $res = array( $merged ); + $res = RevisionStorage::mergeExternalContent( $res ); + foreach ( $res as $i => $result ) { + if ( $result ) { + $res[$i] = array_filter( $result, array( $this, 'validate' ) ); + } + } + + return $res; } protected function findHeaderHistory( array $queries, array $options = array() ) { @@ -105,6 +115,17 @@ return $retval; } + /** + * When retrieving revisions from DB, RevisionStorage::mergeExternalContent + * will be called to fetch the content. This could fail, resulting in the + * content being a 'false' value. + * + * {@inheritDoc} + */ + public function validate( array $row ) { + return !isset( $row['rev_content'] ) || $row['rev_content'] !== false; + } + public function getPrimaryKeyColumns() { return array( 'topic_list_id' ); } @@ -124,5 +145,4 @@ public function getIterator() { throw new DataModelException( 'Not Implemented', 'process-data' ); } - } diff --git a/includes/Data/Storage/PostRevisionStorage.php b/includes/Data/Storage/PostRevisionStorage.php index a156d30..5d12b2c 100644 --- a/includes/Data/Storage/PostRevisionStorage.php +++ b/includes/Data/Storage/PostRevisionStorage.php @@ -11,7 +11,17 @@ * SQL storage and query for PostRevision instances */ class PostRevisionStorage extends RevisionStorage { + /** + * @var TreeRepository + */ + protected $treeRepo; + /** + * @param DbFactory $dbFactory + * @param array|false List of external store servers available for insert + * or false to disable. See $wgFlowExternalStore. + * @param TreeRepository $treeRepo + */ public function __construct( DbFactory $dbFactory, $externalStore, TreeRepository $treeRepo ) { parent::__construct( $dbFactory, $externalStore ); $this->treeRepo = $treeRepo; @@ -60,7 +70,7 @@ } if ( !$res ) { - return false; + return array(); } return $rows; @@ -98,15 +108,15 @@ ); if ( !$res ) { - return false; + return array(); } return $changes; } - // this doesnt delete the whole post, it just deletes the revision. + // this doesn't delete the whole post, it just deletes the revision. // The post will *always* exist in the tree structure, its just a tree - // and we arn't going to re-parent its children; + // and we aren't going to re-parent its children; protected function removeRelated( array $row ) { return $this->dbFactory->getDB( DB_MASTER )->delete( $this->joinTable(), diff --git a/includes/Data/Storage/RevisionStorage.php b/includes/Data/Storage/RevisionStorage.php index 7d5a142..fb6dfda 100644 --- a/includes/Data/Storage/RevisionStorage.php +++ b/includes/Data/Storage/RevisionStorage.php @@ -97,7 +97,7 @@ /** * @param DbFactory $dbFactory - * @param array|false List of externel store servers available for insert + * @param array|false List of external store servers available for insert * or false to disable. See $wgFlowExternalStore. */ public function __construct( DbFactory $dbFactory, $externalStore ) { @@ -459,7 +459,7 @@ return false; } } - return $this->updateRelated( $changeSet, $old ); + return (bool) $this->updateRelated( $changeSet, $old ); } diff --git a/includes/Data/Utils/RawSql.php b/includes/Data/Utils/RawSql.php index 0cd9af9..5b6a2af 100644 --- a/includes/Data/Utils/RawSql.php +++ b/includes/Data/Utils/RawSql.php @@ -8,11 +8,13 @@ * plain strings. */ class RawSql { - function __construct( $sql ) { + protected $sql; + + public function __construct( $sql ) { $this->sql = $sql; } - function getSQL( $db ) { + public function getSQL( $db ) { if ( is_callable( $this->sql ) ) { return call_user_func( $this->sql, $db ); } diff --git a/includes/Data/Utils/ResultDuplicator.php b/includes/Data/Utils/ResultDuplicator.php index b8121d3..6f3bacf 100644 --- a/includes/Data/Utils/ResultDuplicator.php +++ b/includes/Data/Utils/ResultDuplicator.php @@ -13,13 +13,39 @@ // // Maintains merge ordering class ResultDuplicator { - // Maps from the query array to its position in the query array + /** + * Maps from the query array to its position in the query array + * + * @var array + */ protected $queryKeys; + + /** + * @var int + */ + protected $dimensions; + + /** + * @var MultiDimArray + */ + protected $desiredOrder; + + /** + * @var MultiDimArray + */ protected $queryMap; - protected $queries = array(); + + /** + * @var MultiDimArray + */ protected $result; /** + * @var array + */ + protected $queries = array(); + + /** * @param array $queryKeys * @param int $dimensions */ diff --git a/includes/Formatter/AbstractFormatter.php b/includes/Formatter/AbstractFormatter.php index ba6abcd..8d74217 100644 --- a/includes/Formatter/AbstractFormatter.php +++ b/includes/Formatter/AbstractFormatter.php @@ -107,7 +107,7 @@ $request = array_keys( $links ); } elseif ( !$request ) { // empty array was passed - return array(); + return ''; } $have = array_combine( $request, $request ); diff --git a/includes/Formatter/Contributions.php b/includes/Formatter/Contributions.php index 2dddbe7..7fe3866 100644 --- a/includes/Formatter/Contributions.php +++ b/includes/Formatter/Contributions.php @@ -42,6 +42,9 @@ protected function formatHtml( FormatterRow $row, IContextSource $ctx ) { $this->serializer->setIncludeHistoryProperties( true ); $data = $this->serializer->formatApi( $row, $ctx ); + if ( !$data ) { + throw new FlowException( 'Could not format data for row ' . $row->revision->getRevisionId()->getAlphadecimal() ); + } $charDiff = ChangesList::showCharacterDifference( $data['size']['old'], diff --git a/includes/Formatter/ContributionsQuery.php b/includes/Formatter/ContributionsQuery.php index 9e8431b..c3d9c70 100644 --- a/includes/Formatter/ContributionsQuery.php +++ b/includes/Formatter/ContributionsQuery.php @@ -22,7 +22,7 @@ protected $cache; /** - * @var DBFactory + * @var DbFactory */ protected $dbFactory; @@ -89,6 +89,8 @@ * @return array Query conditions */ protected function buildConditions( ContribsPager $pager, $offset, $descending ) { + $conditions = array(); + // Work out user condition if ( $pager->contribs == 'newbie' ) { list( $minUserId, $excludeUserIds ) = $this->getNewbieConditionInfo( $pager ); @@ -234,8 +236,14 @@ } // get content in external storage - $revisions = RevisionStorage::mergeExternalContent( array( $revisions ) ); - $revisions = reset( $revisions ); + $res = array( $revisions ); + $res = RevisionStorage::mergeExternalContent( $res ); + foreach ( $res as $i => $result ) { + if ( $result ) { + $res[$i] = array_filter( $result, array( $this, 'validate' ) ); + } + } + $revisions = reset( $res ); // we have all required data to build revision $mapper = $this->storage->getStorage( $revisionClass )->getMapper(); @@ -291,6 +299,17 @@ return array( $minUserId, $excludeUserIds ); } + + /** + * When retrieving revisions from DB, self::mergeExternalContent will be + * called to fetch the content. This could fail, resulting in the content + * being a 'false' value. + * + * {@inheritDoc} + */ + public function validate( array $row ) { + return !isset( $row['rev_content'] ) || $row['rev_content'] !== false; + } } class ContributionsRow extends FormatterRow { diff --git a/includes/Formatter/IRCLineUrlFormatter.php b/includes/Formatter/IRCLineUrlFormatter.php index d81c7b1..6fe8058 100644 --- a/includes/Formatter/IRCLineUrlFormatter.php +++ b/includes/Formatter/IRCLineUrlFormatter.php @@ -23,13 +23,18 @@ public function getLine( array $feed, RecentChange $rc, $actionComment ) { /** @var RecentChangesQuery $query */ $query = Container::get( 'query.recentchanges' ); - //throw new \Exception($rc->mAttribs['rc_params']); $query->loadMetadataBatch( array( (object)$rc->mAttribs ) ); $rcRow = $query->getResult( null, $rc ); + $ctx = \RequestContext::getMain(); $data = $this->serializer->formatApi( $rcRow, $ctx ); + if ( !$data ) { + throw new FlowException( 'Could not format data for row ' . $rcRow->revision->getRevisionId()->getAlphadecimal() ); + } $this->serializer->setIncludeHistoryProperties( true ); + $rc->mAttribs['rc_comment'] = $this->formatDescription( $data, $ctx ); + /** @var RCFeedFormatter $formatter */ $formatter = new $feed['original_formatter'](); return $formatter->getLine( $feed, $rc, $actionComment ); diff --git a/includes/Formatter/RecentChanges.php b/includes/Formatter/RecentChanges.php index 73a5107..853471e 100644 --- a/includes/Formatter/RecentChanges.php +++ b/includes/Formatter/RecentChanges.php @@ -29,6 +29,10 @@ $this->serializer->setIncludeHistoryProperties( true ); $data = $this->serializer->formatApi( $row, $ctx ); + if ( !$data ) { + throw new FlowException( 'Could not format data for row ' . $row->revision->getRevisionId()->getAlphadecimal() ); + } + // @todo where should this go? $data['size'] = array( 'old' => $row->recentChange->getAttribute( 'rc_old_len' ), diff --git a/includes/Formatter/RecentChangesQuery.php b/includes/Formatter/RecentChangesQuery.php index b3dc4d1..1edc7f4 100644 --- a/includes/Formatter/RecentChangesQuery.php +++ b/includes/Formatter/RecentChangesQuery.php @@ -3,7 +3,7 @@ namespace Flow\Formatter; use Flow\Data\ManagerGroup; -use Flow\Data\RecentChanges\RecentChanges; +use Flow\Data\RecentChanges\RecentChanges as RecentChangesHandler; // not to be confused with RecentChanges in Flow\Formatter namespace use Flow\Exception\FlowException; use Flow\FlowActions; use Flow\Model\UUID; @@ -48,7 +48,7 @@ public function loadMetadataBatch( $rows, $isWatchlist = false ) { $needed = array(); foreach ( $rows as $row ) { - if ( !isset( $row->rc_source ) || $row->rc_source !== RecentChanges::SRC_FLOW ) { + if ( !isset( $row->rc_source ) || $row->rc_source !== RecentChangesHandler::SRC_FLOW ) { continue; } if ( !isset( $row->rc_params ) ) { diff --git a/includes/Formatter/RevisionFormatter.php b/includes/Formatter/RevisionFormatter.php index 4200f6a..8d7e4c6 100644 --- a/includes/Formatter/RevisionFormatter.php +++ b/includes/Formatter/RevisionFormatter.php @@ -89,6 +89,16 @@ protected $userLinks = array(); /** + * @var UserNameBatch + */ + protected $usernames; + + /** + * @var GenderCache + */ + protected $genderCache; + + /** * @param RevisionActionPermissions $permissions * @param Templating $templating * @param UserNameBatch $usernames @@ -421,8 +431,11 @@ if ( // thanks extension must be available !class_exists( 'ThanksHooks' ) || - // anon's can't thank + // anons can't thank $user->isAnon() || + // can only thank for PostRevisions + // (other revision objects have mo getCreator* methods) + !$revision instanceof PostRevision || // can't thank an anon user $revision->getCreatorIp() || // can't thank self @@ -436,11 +449,13 @@ case 'reply': if ( !$postId ) { throw new FlowException( "$type called without \$postId" ); + } elseif ( !$revision instanceof PostRevision ) { + throw new FlowException( "$type called without PostRevision object" ); } /* - * If the post being replied is at or exceeds the max threading - * depth, the reply link should point to parent. + * If the post being replied to is at or exceeds the max + * threading depth, the reply link should point to parent. */ $replyToId = $postId; $replyToRevision = $revision; @@ -854,6 +869,7 @@ if ( !$revision instanceof PostSummary ) { throw new FlowException( 'Expected PostSummary but received ' . get_class( $revision ) ); } + /** @var PostRevision $post */ $post = $revision->getCollection()->getPost()->getLastRevision(); if ( $post->isTopicTitle() ) { return Message::plaintextParam( $this->templating->getContent( $post, 'wikitext' ) ); -- To view, visit https://gerrit.wikimedia.org/r/167785 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Icecee81bded92f2b74627a0fbd9d0e245aceccd3 Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/Flow Gerrit-Branch: master Gerrit-Owner: Matthias Mullie <mmul...@wikimedia.org> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
