EditPage: Don't set 'hookaborted' error if the hook set a better error
authorBartosz Dziewoński <matma.rex@gmail.com>
Mon, 26 Aug 2019 19:56:04 +0000 (21:56 +0200)
committerBartosz Dziewoński <matma.rex@gmail.com>
Mon, 26 Aug 2019 21:25:09 +0000 (23:25 +0200)
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

index d0a5080..2d53e01 100644 (file)
@@ -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;
                }