Matthias Mullie has uploaded a new change for review.
https://gerrit.wikimedia.org/r/214342
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 Hooks.php
M includes/Formatter/Contributions.php
M includes/Formatter/FeedItemFormatter.php
M includes/Formatter/IRCLineUrlFormatter.php
M includes/Formatter/RecentChanges.php
M includes/Formatter/RevisionFormatter.php
6 files changed, 51 insertions(+), 78 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Flow
refs/changes/42/214342/1
diff --git a/Hooks.php b/Hooks.php
index 245bca3..31f53fc 100644
--- a/Hooks.php
+++ b/Hooks.php
@@ -391,7 +391,7 @@
/** @var Flow\Formatter\RecentChanges $formatter */
$formatter = Container::get( 'formatter.recentchanges'
);
- $links = $formatter->getLogTextLinks( $row,
$changesList, $block, $links );
+ $logTextLinks = $formatter->getLogTextLinks( $row,
$changesList, $block, $links );
} catch ( Exception $e ) {
wfDebugLog( 'Flow', __METHOD__ . ': Exception
formatting rc logtext ' . $rc->getAttribute( 'rc_id' ) . ' ' . $e );
MWExceptionHandler::logException( $e );
@@ -400,6 +400,11 @@
}
restore_error_handler();
+ if ( $logTextLinks === false ) {
+ return false;
+ }
+
+ $links = $logTextLinks;
return true;
}
@@ -623,6 +628,7 @@
$formatter = Container::get( 'formatter.contributions'
);
$line = $formatter->format( $row, $pager );
} catch ( Exception $e ) {
+ wfDebugLog( 'Flow', __METHOD__ . ': Failed formatting
contribution ' . json_encode( $row ) . ': ' . $e->getMessage() );
MWExceptionHandler::logException( $e );
$line = false;
}
@@ -677,9 +683,15 @@
}
set_error_handler( new Flow\RecoverableErrorHandler, -1 );
- /** @var Flow\Formatter\Contributions $formatter */
- $formatter = Container::get( 'formatter.contributions.feeditem'
);
- $result = $formatter->format( $row, $ctx );
+ try {
+ /** @var Flow\Formatter\FeedItemFormatter $formatter */
+ $formatter = Container::get(
'formatter.contributions.feeditem' );
+ $result = $formatter->format( $row, $ctx );
+ } catch ( Exception $e ) {
+ wfDebugLog( 'Flow', __METHOD__ . ': Failed formatting
contribution ' . json_encode( $row ) . ': ' . $e->getMessage() );
+ MWExceptionHandler::logException( $e );
+ return false;
+ }
restore_error_handler();
if ( $result instanceof FeedItem ) {
@@ -908,8 +920,8 @@
$formatter = Container::get( 'formatter.irclineurl' );
$result = $formatter->format( $rc );
} catch ( Exception $e ) {
- wfDebugLog( 'Flow', __METHOD__ . ': Failed formatting
rc ' . $rc->getAttribute( 'rc_id' )
- . ': ' . $e->getMessage() );
+ $result = null;
+ wfDebugLog( 'Flow', __METHOD__ . ': Failed formatting
rc ' . $rc->getAttribute( 'rc_id' ) . ': ' . $e->getMessage() );
MWExceptionHandler::logException( $e );
}
restore_error_handler();
diff --git a/includes/Formatter/Contributions.php
b/includes/Formatter/Contributions.php
index bd98466..b38cab1 100644
--- a/includes/Formatter/Contributions.php
+++ b/includes/Formatter/Contributions.php
@@ -19,34 +19,13 @@
* @param FormatterRow $row With properties workflow, revision,
previous_revision
* @param IContextSource $ctx
* @return string|false HTML for contributions entry, or false on
failure
- */
- public function format( FormatterRow $row, IContextSource $ctx ) {
- try {
- if ( !$this->permissions->isAllowed( $row->revision,
'contributions' ) ) {
- return false;
- }
- if ( $row->revision instanceof PostRevision &&
- !$this->permissions->isAllowed( $row->rootPost,
'contributions' ) ) {
- return false;
- }
- return $this->formatHtml( $row, $ctx );
- } catch ( FlowException $e ) {
- \MWExceptionHandler::logException( $e );
- return false;
- }
- }
-
- /**
- * @param FormatterRow $row
- * @param IContextSource $ctx
- * @return string
* @throws FlowException
*/
- protected function formatHtml( FormatterRow $row, IContextSource $ctx )
{
+ public function format( FormatterRow $row, IContextSource $ctx ) {
$this->serializer->setIncludeHistoryProperties( true );
- $data = $this->serializer->formatApi( $row, $ctx );
+ $data = $this->serializer->formatApi( $row, $ctx,
'contributions' );
if ( !$data ) {
- throw new FlowException( 'Could not format data for row
' . $row->revision->getRevisionId()->getAlphadecimal() );
+ return false;
}
$charDiff = ChangesList::showCharacterDifference(
diff --git a/includes/Formatter/FeedItemFormatter.php
b/includes/Formatter/FeedItemFormatter.php
index f92fc93..f182d35 100644
--- a/includes/Formatter/FeedItemFormatter.php
+++ b/includes/Formatter/FeedItemFormatter.php
@@ -17,29 +17,13 @@
* @param FormatterRow $row With properties workflow, revision,
previous_revision
* @param IContextSource $ctx
* @return FeedItem|false The requested format, or false on failure
+ * @throws FlowException
*/
public function format( FormatterRow $row, IContextSource $ctx ) {
- try {
- if ( !$this->permissions->isAllowed( $row->revision,
'history' ) ) {
- return false;
- }
- if ( $row->revision instanceof PostRevision &&
- !$this->permissions->isAllowed( $row->rootPost,
'history' ) ) {
- return false;
- }
-
- return $this->createFeedItem( $row, $ctx );
- } catch ( Exception $e ) {
- \MWExceptionHandler::logException( $e );
- return false;
- }
- }
-
- protected function createFeedItem( FormatterRow $row, IContextSource
$ctx ) {
$this->serializer->setIncludeHistoryProperties( true );
- $data = $this->serializer->formatApi( $row, $ctx );
+ $data = $this->serializer->formatApi( $row, $ctx,
'contributions' );
if ( !$data ) {
- throw new FlowException( 'Could not format data for row
' . $row->revision->getRevisionId()->getAlphadecimal() );
+ return false;
}
$preferredLinks = array(
diff --git a/includes/Formatter/IRCLineUrlFormatter.php
b/includes/Formatter/IRCLineUrlFormatter.php
index f8824c6..edae039 100644
--- a/includes/Formatter/IRCLineUrlFormatter.php
+++ b/includes/Formatter/IRCLineUrlFormatter.php
@@ -36,12 +36,21 @@
/**
* Allows us to set the rc_comment field
*/
+ /**
+ * @param array $feed
+ * @param RecentChange $rc
+ * @param null|string $actionComment
+ * @return string|null Text for IRC line, or null on failure
+ */
public function getLine( array $feed, RecentChange $rc, $actionComment
) {
$ctx = \RequestContext::getMain();
- $rc->mAttribs['rc_comment'] = $this->formatDescription(
- $this->serializeRcRevision( $rc, $ctx ),
- $ctx
- );
+
+ $serialized = $this->serializeRcRevision( $rc, $ctx );
+ if ( !$serialized ) {
+ return null;
+ }
+
+ $rc->mAttribs['rc_comment'] = $this->formatDescription(
$serialized, $ctx );
/** @var RCFeedFormatter $formatter */
$formatter = new $feed['original_formatter']();
@@ -58,12 +67,7 @@
$rcRow = $query->getResult( null, $rc );
$this->serializer->setIncludeHistoryProperties( true );
- $data = $this->serializer->formatApi( $rcRow, $ctx );
- if ( !$data ) {
- throw new FlowException( 'Could not format data for row
' . $rcRow->revision->getRevisionId()->getAlphadecimal() );
- }
-
- return $data;
+ return $this->serializer->formatApi( $rcRow, $ctx,
'recentchanges' );
}
/**
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: newchange
Gerrit-Change-Id: Icb26b999f16e591fccadb9dce2913e55d50bb7a2
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
