[MediaWiki-commits] [Gerrit] Don't log exception when missing permissions, just ignore it - change (mediawiki...Flow)
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
*
[MediaWiki-commits] [Gerrit] Don't log exception when missing permissions, just ignore it - change (mediawiki...Flow)
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 (
-
[MediaWiki-commits] [Gerrit] Don't log exception when missing permissions, just ignore it - change (mediawiki...Flow)
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 @@
[MediaWiki-commits] [Gerrit] Don't log exception when missing permissions, just ignore it - change (mediawiki...Flow)
Matthias Mullie has uploaded a new change for review.
https://gerrit.wikimedia.org/r/210902
Change subject: Don't log exception when missing permissions, just ignore it
..
Don't log exception when missing permissions, just ignore it
Similar to format()
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.
format() already counters 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.
I've just made getLogTextLinks do the same...
Change-Id: Ibc45d1a81e023cd0942374fdc5e4fc6a978f936f
---
M Hooks.php
M includes/Formatter/RecentChanges.php
2 files changed, 11 insertions(+), 2 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Flow
refs/changes/02/210902/1
diff --git a/Hooks.php b/Hooks.php
index 245bca3..1c420aa 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;
}
diff --git a/includes/Formatter/RecentChanges.php
b/includes/Formatter/RecentChanges.php
index 1c0d684..64a62ad 100644
--- a/includes/Formatter/RecentChanges.php
+++ b/includes/Formatter/RecentChanges.php
@@ -159,11 +159,15 @@
* @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() ) {
+ if ( !$this-permissions-isAllowed( $row-revision,
'recentchanges' ) ) {
+ return false;
+ }
+
$old = unserialize( $block[count( $block ) -
1]-mAttribs['rc_params'] );
$oldId = $old ? UUID::create(
$old['flow-workflow-change']['revision'] ) : $row-revision-getRevisionId();
--
To view, visit https://gerrit.wikimedia.org/r/210902
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibc45d1a81e023cd0942374fdc5e4fc6a978f936f
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
