Matthias Mullie 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: Ibc45d1a81e023cd0942374fdc5e4fc6a978f936f
---
M Hooks.php
M includes/Formatter/Contributions.php
M includes/Formatter/FeedItemFormatter.php
M includes/Formatter/IRCLineUrlFormatter.php
4 files changed, 38 insertions(+), 59 deletions(-)
Approvals:
Sbisson: Looks good to me, approved
jenkins-bot: Verified
diff --git a/Hooks.php b/Hooks.php
index 74aa4ba..e375381 100644
--- a/Hooks.php
+++ b/Hooks.php
@@ -431,7 +431,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 );
@@ -440,6 +440,11 @@
}
restore_error_handler();
+ if ($logTextLinks === false) {
+ return false;
+ }
+
+ $links = $logTextLinks;
return true;
}
@@ -663,6 +668,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;
}
@@ -717,9 +723,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 ) {
@@ -948,8 +960,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' );
}
/**
--
To view, visit https://gerrit.wikimedia.org/r/210902
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibc45d1a81e023cd0942374fdc5e4fc6a978f936f
Gerrit-PatchSet: 4
Gerrit-Project: mediawiki/extensions/Flow
Gerrit-Branch: master
Gerrit-Owner: Matthias Mullie <[email protected]>
Gerrit-Reviewer: Catrope <[email protected]>
Gerrit-Reviewer: Matthias Mullie <[email protected]>
Gerrit-Reviewer: Sbisson <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
