jenkins-bot has submitted this change and it was merged. Change subject: Don't log exception when missing permissions, just ignore it ......................................................................
Don't log exception when missing permissions, just ignore it formatApi() fails on a couple of occasions, one of which being when a user has insufficient permissions. That's an "acceptable" error: we shouldn't log it, just ignore that row. Some places already counter this by first checking permissions, then passing it off to formatApi() - if that one fails, it's not a permission issue & we should log the failure. Let's just throw an exception right away if it's a real error case, and return false if it's a "this is no error but we can't show you the data" case. Makes dealing with this simpler. One potential regression: this formatApi function is not just called from RC, Contribs, ... formatters, but also in other places (everywhere...), so adding a new exception in there might break those other places. The first check was already being logged if it was being hit, and I couldn't find it in the logs; so pretty sure we won't see it happen. Change-Id: Icb26b999f16e591fccadb9dce2913e55d50bb7a2 --- M includes/Formatter/RecentChanges.php M includes/Formatter/RevisionFormatter.php 2 files changed, 13 insertions(+), 19 deletions(-) Approvals: Catrope: Looks good to me, approved jenkins-bot: Verified diff --git a/includes/Formatter/RecentChanges.php b/includes/Formatter/RecentChanges.php index 1c0d684..ad87b81 100644 --- a/includes/Formatter/RecentChanges.php +++ b/includes/Formatter/RecentChanges.php @@ -24,16 +24,12 @@ * @throws FlowException */ public function format( RecentChangesRow $row, IContextSource $ctx, $linkOnly = false ) { - if ( !$this->permissions->isAllowed( $row->revision, 'recentchanges' ) ) { - return false; - } - $this->serializer->setIncludeHistoryProperties( true ); $this->serializer->setIncludeContent( false ); $data = $this->serializer->formatApi( $row, $ctx, 'recentchanges' ); if ( !$data ) { - throw new FlowException( 'Could not format data for row ' . $row->revision->getRevisionId()->getAlphadecimal() ); + return false; } if ( $linkOnly ) { @@ -159,19 +155,19 @@ * @param IContextSource $ctx * @param array $block * @param array $links - * @return array + * @return array|false Links array, or false on failure * @throws FlowException * @throws \Flow\Exception\InvalidInputException */ public function getLogTextLinks( RecentChangesRow $row, IContextSource $ctx, array $block, array $links = array() ) { - $old = unserialize( $block[count( $block ) - 1]->mAttribs['rc_params'] ); - $oldId = $old ? UUID::create( $old['flow-workflow-change']['revision'] ) : $row->revision->getRevisionId(); - $data = $this->serializer->formatApi( $row, $ctx, 'recentchanges' ); if ( !$data ) { - throw new FlowException( 'Could not format data for row ' . $row->revision->getRevisionId()->getAlphadecimal() ); + return false; } + $old = unserialize( $block[count( $block ) - 1]->mAttribs['rc_params'] ); + $oldId = $old ? UUID::create( $old['flow-workflow-change']['revision'] ) : $row->revision->getRevisionId(); + if ( isset( $data['links']['topic'] ) ) { // add highlight details to anchor /** @var Anchor $anchor */ diff --git a/includes/Formatter/RevisionFormatter.php b/includes/Formatter/RevisionFormatter.php index 10d5ea2..6723f83 100644 --- a/includes/Formatter/RevisionFormatter.php +++ b/includes/Formatter/RevisionFormatter.php @@ -165,12 +165,12 @@ */ public function formatApi( FormatterRow $row, IContextSource $ctx, $action = 'view' ) { $user = $ctx->getUser(); + // @todo the only permissions currently checked in this class are prev-revision // mostly permissions is used for the actions, figure out how permissions should // fit into this class either used more or not at all. if ( $user->getName() !== $this->permissions->getUser()->getName() ) { - wfDebugLog( 'Flow', __METHOD__ . ': Formatting for wrong user' ); - return false; + throw new FlowException( 'Formatting for wrong user' ); } if ( !$this->permissions->isAllowed( $row->revision, $action ) ) { @@ -251,13 +251,11 @@ 'watchable', ) ); - if ( - $row->summary && - $this->permissions->isAllowed( $row->summary->revision, 'view' ) - ) { - $res['summary'] = array( - 'revision' => $this->formatApi( $row->summary, $ctx ) - ); + if ( $row->summary ) { + $summary = $this->formatApi( $row->summary, $ctx, $action ); + if ( $summary ) { + $res['summary'] = $summary; + } } // Only non-anon users can watch/unwatch a flow topic -- To view, visit https://gerrit.wikimedia.org/r/214342 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: Icb26b999f16e591fccadb9dce2913e55d50bb7a2 Gerrit-PatchSet: 5 Gerrit-Project: mediawiki/extensions/Flow Gerrit-Branch: master Gerrit-Owner: Matthias Mullie <mmul...@wikimedia.org> Gerrit-Reviewer: Catrope <roan.katt...@gmail.com> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits