From e03ae008f004f396a2bf7a2036946af03c93e6ab Mon Sep 17 00:00:00 2001
From: =?utf8?q?Bartosz=20Dziewo=C5=84ski?= <matma.rex@gmail.com>
Date: Mon, 26 Aug 2019 21:56:04 +0200
Subject: [PATCH] EditPage: Don't set 'hookaborted' error if the hook set a
 better error

When the 'EditFilterMergedContent' hook set an erroneous status but
did not abort execution (hook function does not return false), we
always added a 'hookaborted' error to the status, regardless of what
was already set.

This mistake was not visible in the edit form, because error message
formatting was incorrectly done before that (which would actually emit
a different error if none was set).

However, the additional error code propagated to the API, where it was
emitted when using 'errorformat=html' (or any other format except the
default 'bc', which can only produce one error code/message).

This affects various extensions, including EventLogging, UploadWizard,
AbuseFilter (after I5424de38) and SpamBlacklist (after Id36aa6bd).

----

You can test this behavior with these two simple hooks:

// A: Set error status and error message
$wgHooks['EditFilterMergedContent'][] = function ( $context, $content, $status ) {
  $status->fatal( 'test-message' );
};

// B: Set error status but no error message
$wgHooks['EditFilterMergedContent'][] = function ( $context, $content, $status ) {
  $status->setOK( false );
};

Adding either of them to your LocalSettings will cause all edits to
fail as follows:

Before this patch:
         | Error message in the UI   | Error code in the API     |
| Hook A | test-message              | test-message, hookaborted |
| Hook B | internalerror_info        | hookaborted               |

After this patch:
         | Error message in the UI   | Error code in the API     |
| Hook A | test-message              | test-message              |
| Hook B | hookaborted               | hookaborted               |

Bug: T231253
Change-Id: I106dbd3cbdbf7082b1d1f1c1106ece6b19c22a86
---
 includes/EditPage.php | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/includes/EditPage.php b/includes/EditPage.php
index d0a5080d0b..2d53e01569 100644
--- a/includes/EditPage.php
+++ b/includes/EditPage.php
@@ -1785,8 +1785,11 @@ class EditPage {
 		} elseif ( !$status->isOK() ) {
 			# ...or the hook could be expecting us to produce an error
 			// FIXME this sucks, we should just use the Status object throughout
+			if ( !$status->getErrors() ) {
+				// Provide a fallback error message if none was set
+				$status->fatal( 'hookaborted' );
+			}
 			$this->hookError = $this->formatStatusErrors( $status );
-			$status->fatal( 'hookaborted' );
 			$status->value = self::AS_HOOK_ERROR_EXPECTED;
 			return false;
 		}
-- 
2.20.1