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
