Show warnings in HTMLForm and warnings as warnings on Login/Signup form

This commit changes the way how HTMLForm handles a Status object
when executed from a request. It now handles, beside the errors,
also the warnings of a Status object and prints them out, wrapped
in a warning box.

The LoginSignupPage uses this feature to show informative warnings
actually as warnings and not as more disturbing error messages.
Error messages should be reserved for errors and only for erros. An
AuthenticationProvider, which returns an UI AuthenticationResponse
can choose, if the given message is an error or a warning message.

This commit also addds a new function to Status, which allows a
developer to split the object into two new Status objects, where one only
contains the errors and the other only the warnings of the origin
Status object (splitByErrorType). StatusValue also has a new function,
splitByErrorType(), to support this.

Bug: T139179
Change-Id: I9a27911613e62b5c4cb86bea40696cb37c4f49c2

diff --git a/includes/Status.php b/includes/Status.php
index e578873..a6348b7 100644 (file)
--- a/includes/Status.php
+++ b/includes/Status.php
@@ -105,6 +105,26 @@ class Status {
                return new self( $sv );
        }
 
+       /**
+        * Splits this Status object into two new Status objects, one which contains only
+        * the error messages, and one that contains the warnings, only. The returned array is
+        * defined as:
+        * array(
+        *      0 => object(Status) # the Status with error messages, only
+        *      1 => object(Status) # The Status with warning messages, only
+        * )
+        *
+        * @return array
+        */
+       public function splitByErrorType() {
+               list( $errorsOnlyStatusValue, $warningsOnlyStatusValue ) = $this->sv->splitByErrorType();
+               $errorsOnlyStatus = new Status( $errorsOnlyStatusValue );
+               $warningsOnlyStatus = new Status( $warningsOnlyStatusValue );
+               $errorsOnlyStatus->cleanCallback = $warningsOnlyStatus->cleanCallback = $this->cleanCallback;
+
+               return [ $errorsOnlyStatus, $warningsOnlyStatus ];
+       }
+
        /**
         * Change operation result
         *
@@ -314,6 +334,7 @@ class Status {
 
        /**
         * Return the message for a single error.
+        *
         * @param mixed $error With an array & two values keyed by
         * 'message' and 'params', use those keys-value pairs.
         * Otherwise, if its an array, just use the first value as the

diff --git a/includes/auth/AuthenticationResponse.php b/includes/auth/AuthenticationResponse.php
index 0339e45..6684fb9 100644 (file)
--- a/includes/auth/AuthenticationResponse.php
+++ b/includes/auth/AuthenticationResponse.php
@@ -81,6 +81,9 @@ class AuthenticationResponse {
        /** @var Message|null I18n message to display in case of UI or FAIL */
        public $message = null;
 
+       /** @var string Whether the $message is an error or warning message, for styling reasons */
+       public $messageType = 'warning';
+
        /**
         * @var string|null Local user name from authentication.
         * May be null if the authentication passed but no local user is known.
@@ -144,6 +147,7 @@ class AuthenticationResponse {
                $ret = new AuthenticationResponse;
                $ret->status = AuthenticationResponse::FAIL;
                $ret->message = $msg;
+               $ret->messageType = 'error';
                return $ret;
        }
 
@@ -172,18 +176,23 @@ class AuthenticationResponse {
        /**
         * @param AuthenticationRequest[] $reqs AuthenticationRequests needed to continue
         * @param Message $msg
+        * @param string $msgtype
         * @return AuthenticationResponse
         * @see AuthenticationResponse::UI
         */
-       public static function newUI( array $reqs, Message $msg ) {
+       public static function newUI( array $reqs, Message $msg, $msgtype = 'warning' ) {
                if ( !$reqs ) {
                        throw new \InvalidArgumentException( '$reqs may not be empty' );
                }
+               if ( $msgtype !== 'warning' && $msgtype !== 'error' ) {
+                       throw new \InvalidArgumentException( $msgtype . ' is not a valid message type.' );
+               }
 
                $ret = new AuthenticationResponse;
                $ret->status = AuthenticationResponse::UI;
                $ret->neededRequests = $reqs;
                $ret->message = $msg;
+               $ret->messageType = $msgtype;
                return $ret;
        }
 

diff --git a/includes/auth/ConfirmLinkSecondaryAuthenticationProvider.php b/includes/auth/ConfirmLinkSecondaryAuthenticationProvider.php
index beb11f4..7f121cd 100644 (file)
--- a/includes/auth/ConfirmLinkSecondaryAuthenticationProvider.php
+++ b/includes/auth/ConfirmLinkSecondaryAuthenticationProvider.php
@@ -64,7 +64,8 @@ class ConfirmLinkSecondaryAuthenticationProvider extends AbstractSecondaryAuthen
                $req = new ConfirmLinkAuthenticationRequest( $maybeLink );
                return AuthenticationResponse::newUI(
                        [ $req ],
-                       wfMessage( 'authprovider-confirmlink-message' )
+                       wfMessage( 'authprovider-confirmlink-message' ),
+                       'warning'
                );
        }
 
@@ -150,7 +151,8 @@ class ConfirmLinkSecondaryAuthenticationProvider extends AbstractSecondaryAuthen
                                        'linkOk', wfMessage( 'ok' ), wfMessage( 'authprovider-confirmlink-ok-help' )
                                )
                        ],
-                       $combinedStatus->getMessage( 'authprovider-confirmlink-failed' )
+                       $combinedStatus->getMessage( 'authprovider-confirmlink-failed' ),
+                       'error'
                );
        }
 }

diff --git a/includes/auth/ResetPasswordSecondaryAuthenticationProvider.php b/includes/auth/ResetPasswordSecondaryAuthenticationProvider.php
index dd97830..f11a12c 100644 (file)
--- a/includes/auth/ResetPasswordSecondaryAuthenticationProvider.php
+++ b/includes/auth/ResetPasswordSecondaryAuthenticationProvider.php
@@ -112,17 +112,17 @@ class ResetPasswordSecondaryAuthenticationProvider extends AbstractSecondaryAuth
 
                $req = AuthenticationRequest::getRequestByClass( $reqs, get_class( $needReq ) );
                if ( !$req || !array_key_exists( 'retype', $req->getFieldInfo() ) ) {
-                       return AuthenticationResponse::newUI( $needReqs, $data->msg );
+                       return AuthenticationResponse::newUI( $needReqs, $data->msg, 'warning' );
                }
 
                if ( $req->password !== $req->retype ) {
-                       return AuthenticationResponse::newUI( $needReqs, new \Message( 'badretype' ) );
+                       return AuthenticationResponse::newUI( $needReqs, new \Message( 'badretype' ), 'error' );
                }
 
                $req->username = $user->getName();
                $status = $this->manager->allowsAuthenticationDataChange( $req );
                if ( !$status->isGood() ) {
-                       return AuthenticationResponse::newUI( $needReqs, $status->getMessage() );
+                       return AuthenticationResponse::newUI( $needReqs, $status->getMessage(), 'error' );
                }
                $this->manager->changeAuthenticationData( $req );
 

diff --git a/includes/htmlform/HTMLForm.php b/includes/htmlform/HTMLForm.php
index 3c88594..567e692 100644 (file)
--- a/includes/htmlform/HTMLForm.php
+++ b/includes/htmlform/HTMLForm.php
@@ -1014,7 +1014,8 @@ class HTMLForm extends ContextSource {
                $this->getOutput()->addModuleStyles( 'mediawiki.htmlform.styles' );
 
                $html = ''
-                       . $this->getErrors( $submitResult )
+                       . $this->getErrorsOrWarnings( $submitResult, 'error' )
+                       . $this->getErrorsOrWarnings( $submitResult, 'warning' )
                        . $this->getHeaderText()
                        . $this->getBody()
                        . $this->getHiddenFields()
@@ -1230,23 +1231,46 @@ class HTMLForm extends ContextSource {
         *
         * @param string|array|Status $errors
         *
+        * @deprecated since 1.28, use getErrorsOrWarnings() instead
+        *
         * @return string
         */
        public function getErrors( $errors ) {
-               if ( $errors instanceof Status ) {
-                       if ( $errors->isOK() ) {
-                               $errorstr = '';
+               wfDeprecated( __METHOD__ );
+               return $this->getErrorsOrWarnings( $errors, 'error' );
+       }
+
+       /**
+        * Returns a formatted list of errors or warnings from the given elements.
+        *
+        * @param string|array|Status $elements The set of errors/warnings to process.
+        * @param string $elementsType Should warnings or errors be returned.  This is meant
+        *      for Status objects, all other valid types are always considered as errors.
+        * @return string
+        */
+       public function getErrorsOrWarnings( $elements, $elementsType ) {
+               if ( !in_array( $elementsType, [ 'error', 'warning' ], true ) ) {
+                       throw new DomainException( $elementsType . ' is not a valid type.' );
+               }
+               $elementstr = false;
+               if ( $elements instanceof Status ) {
+                       list( $errorStatus, $warningStatus ) = $elements->splitByErrorType();
+                       $status = $elementsType === 'error' ? $errorStatus : $warningStatus;
+                       if ( $status->isGood() ) {
+                               $elementstr = '';
                        } else {
-                               $errorstr = $this->getOutput()->parse( $errors->getWikiText() );
+                               $elementstr = $this->getOutput()->parse(
+                                       $status->getWikiText()
+                               );
                        }
-               } elseif ( is_array( $errors ) ) {
-                       $errorstr = $this->formatErrors( $errors );
-               } else {
-                       $errorstr = $errors;
+               } elseif ( is_array( $elements ) && $elementsType === 'error' ) {
+                       $elementstr = $this->formatErrors( $elements );
+               } elseif ( $elementsType === 'error' ) {
+                       $elementstr = $elements;
                }
 
-               return $errorstr
-                       ? Html::rawElement( 'div', [ 'class' => 'error' ], $errorstr )
+               return $elementstr
+                       ? Html::rawElement( 'div', [ 'class' => $elementsType ], $elementstr )
                        : '';
        }
 

diff --git a/includes/htmlform/OOUIHTMLForm.php b/includes/htmlform/OOUIHTMLForm.php
index 0b22727..bbd3473 100644 (file)
--- a/includes/htmlform/OOUIHTMLForm.php
+++ b/includes/htmlform/OOUIHTMLForm.php
@@ -26,6 +26,7 @@
  */
 class OOUIHTMLForm extends HTMLForm {
        private $oouiErrors;
+       private $oouiWarnings;
 
        public function __construct( $descriptor, $context = null, $messagePrefix = '' ) {
                parent::__construct( $descriptor, $context, $messagePrefix );
@@ -185,28 +186,34 @@ class OOUIHTMLForm extends HTMLForm {
        }
 
        /**
-        * @param string|array|Status $err
+        * @param string|array|Status $elements
+        * @param string $elementsType
         * @return string
         */
-       function getErrors( $err ) {
-               if ( !$err ) {
+       function getErrorsOrWarnings( $elements, $elementsType ) {
+               if ( !in_array( $elementsType, [ 'error', 'warning' ] ) ) {
+                       throw new DomainException( $elementsType . ' is not a valid type.' );
+               }
+               if ( !$elements ) {
                        $errors = [];
-               } elseif ( $err instanceof Status ) {
-                       if ( $err->isOK() ) {
+               } elseif ( $elements instanceof Status ) {
+                       if ( $elements->isGood() ) {
                                $errors = [];
                        } else {
-                               $errors = $err->getErrorsByType( 'error' );
+                               $errors = $elements->getErrorsByType( $elementsType );
                                foreach ( $errors as &$error ) {
                                        // Input:  array( 'message' => 'foo', 'errors' => array( 'a', 'b', 'c' ) )
                                        // Output: array( 'foo', 'a', 'b', 'c' )
                                        $error = array_merge( [ $error['message'] ], $error['params'] );
                                }
                        }
-               } else {
-                       $errors = $err;
+               } elseif ( $elementsType === 'errors' )  {
+                       $errors = $elements;
                        if ( !is_array( $errors ) ) {
                                $errors = [ $errors ];
                        }
+               } else {
+                       $errors = [];
                }
 
                foreach ( $errors as &$error ) {
@@ -215,7 +222,11 @@ class OOUIHTMLForm extends HTMLForm {
                }
 
                // Used in getBody()
-               $this->oouiErrors = $errors;
+               if ( $elementsType === 'error' ) {
+                       $this->oouiErrors = $errors;
+               } else {
+                       $this->oouiWarnings = $errors;
+               }
                return '';
        }
 
@@ -236,7 +247,10 @@ class OOUIHTMLForm extends HTMLForm {
                        if ( $this->oouiErrors ) {
                                $classes[] = 'mw-htmlform-ooui-header-errors';
                        }
-                       if ( $this->mHeader || $this->oouiErrors ) {
+                       if ( $this->oouiWarnings ) {
+                               $classes[] = 'mw-htmlform-ooui-header-warnings';
+                       }
+                       if ( $this->mHeader || $this->oouiErrors || $this->oouiWarnings ) {
                                // if there's no header, don't create an (empty) LabelWidget, simply use a placeholder
                                if ( $this->mHeader ) {
                                        $element = new OOUI\LabelWidget( [ 'label' => new OOUI\HtmlSnippet( $this->mHeader ) ] );
@@ -249,6 +263,7 @@ class OOUIHTMLForm extends HTMLForm {
                                                [
                                                        'align' => 'top',
                                                        'errors' => $this->oouiErrors,
+                                                       'notices' => $this->oouiWarnings,
                                                        'classes' => $classes,
                                                ]
                                        )

diff --git a/includes/libs/StatusValue.php b/includes/libs/StatusValue.php
index 1d23f9d..f45dc41 100644 (file)
--- a/includes/libs/StatusValue.php
+++ b/includes/libs/StatusValue.php
@@ -79,6 +79,34 @@ class StatusValue {
                return $result;
        }
 
+       /**
+        * Splits this StatusValue object into two new StatusValue objects, one which contains only
+        * the error messages, and one that contains the warnings, only. The returned array is
+        * defined as:
+        * array(
+        *      0 => object(StatusValue) # the StatusValue with error messages, only
+        *      1 => object(StatusValue) # The StatusValue with warning messages, only
+        * )
+        *
+        * @return array
+        */
+       public function splitByErrorType() {
+               $errorsOnlyStatusValue = clone $this;
+               $warningsOnlyStatusValue = clone $this;
+               $warningsOnlyStatusValue->ok = true;
+
+               $errorsOnlyStatusValue->errors = $warningsOnlyStatusValue->errors = [];
+               foreach ( $this->errors as $item ) {
+                       if ( $item['type'] === 'warning' ) {
+                               $warningsOnlyStatusValue->errors[] = $item;
+                       } else {
+                               $errorsOnlyStatusValue->errors[] = $item;
+                       }
+               };
+
+               return [ $errorsOnlyStatusValue, $warningsOnlyStatusValue ];
+       }
+
        /**
         * Returns whether the operation completed and didn't have any error or
         * warnings

diff --git a/includes/specialpage/LoginSignupSpecialPage.php b/includes/specialpage/LoginSignupSpecialPage.php
index c3d43df..9e93970 100644 (file)
--- a/includes/specialpage/LoginSignupSpecialPage.php
+++ b/includes/specialpage/LoginSignupSpecialPage.php
@@ -354,7 +354,7 @@ abstract class LoginSignupSpecialPage extends AuthManagerSpecialPage {
                                $this->authAction = $this->isSignup() ? AuthManager::ACTION_CREATE_CONTINUE
                                        : AuthManager::ACTION_LOGIN_CONTINUE;
                                $this->authRequests = $response->neededRequests;
-                               $this->mainLoginForm( $response->neededRequests, $response->message, 'warning' );
+                               $this->mainLoginForm( $response->neededRequests, $response->message, $response->messageType );
                                break;
                        default:
                                throw new LogicException( 'invalid AuthenticationResponse' );
@@ -494,7 +494,13 @@ abstract class LoginSignupSpecialPage extends AuthManagerSpecialPage {
 
                $form = $this->getAuthForm( $requests, $this->authAction, $msg, $msgtype );
                $form->prepareForm();
-               $formHtml = $form->getHTML( $msg ? Status::newFatal( $msg ) : false );
+               $submitStatus = Status::newGood();
+               if ( $msg && $msgtype === 'warning' ) {
+                       $submitStatus->warning( $msg );
+               } elseif ( $msg && $msgtype === 'error' ) {
+                       $submitStatus->fatal( $msg );
+               }
+               $formHtml = $form->getHTML( $submitStatus );
 
                $out->addHTML( $this->getPageHtml( $formHtml ) );
        }

diff --git a/resources/src/mediawiki.less/mediawiki.ui/variables.less b/resources/src/mediawiki.less/mediawiki.ui/variables.less
index 507109a..b6f6568 100644 (file)
--- a/resources/src/mediawiki.less/mediawiki.ui/variables.less
+++ b/resources/src/mediawiki.less/mediawiki.ui/variables.less
@@ -51,6 +51,7 @@
 @colorButtonTextActive: @colorGray7;
 @colorDisabledText: @colorGray12;
 @colorErrorText: #c00;
+@colorWarningText: #705000;
 
 // UI colors
 @colorFieldBorder: @colorGray12;

diff --git a/resources/src/mediawiki.ui/components/forms.less b/resources/src/mediawiki.ui/components/forms.less
index cc96a5c..aedec5b 100644 (file)
--- a/resources/src/mediawiki.ui/components/forms.less
+++ b/resources/src/mediawiki.ui/components/forms.less
@@ -103,6 +103,7 @@
        //
        // Styleguide 5.2.
        .error,
+       .warning,
        .errorbox,
        .warningbox,
        .successbox {
@@ -118,7 +119,13 @@
                color: @colorErrorText;
                border: 1px solid #fac5c5;
                background-color: #fae3e3;
-               text-shadow: 0 1px #fae3e3;
+       }
+
+       // Colours taken from those for .warningbox in shared.css
+       .warning {
+               color: @colorWarningText;
+               border: 1px solid #fde29b;
+               background-color: #fdf1d1;
        }
 
        // This specifies styling for individual field validation error messages.

diff --git a/tests/phpunit/includes/StatusTest.php b/tests/phpunit/includes/StatusTest.php
index 474a481..ebc2d10 100644 (file)
--- a/tests/phpunit/includes/StatusTest.php
+++ b/tests/phpunit/includes/StatusTest.php
@@ -645,4 +645,66 @@ class StatusTest extends MediaWikiLangTestCase {
                ];
        }
 
+       /**
+        * @dataProvider provideErrorsWarningsOnly
+        * @covers Status::getErrorsOnlyStatus
+        * @covers Status::getWarningsOnlyStatus
+        */
+       public function testGetErrorsWarningsOnlyStatus( $errorText, $warningText, $type, $errorResult,
+               $warningResult
+       ) {
+               $status = Status::newGood();
+               if ( $errorText ) {
+                       $status->fatal( $errorText );
+               }
+               if ( $warningText ) {
+                       $status->warning( $warningText );
+               }
+               $testStatus = $status->splitByErrorType()[$type];
+               $this->assertEquals( $errorResult, $testStatus->getErrorsByType( 'error' ) );
+               $this->assertEquals( $warningResult, $testStatus->getErrorsByType( 'warning' ) );
+       }
+
+       public static function provideErrorsWarningsOnly() {
+               return [
+                       [
+                               'Just an error',
+                               'Just a warning',
+                               0,
+                               [
+                                       0 => [
+                                               'type' => 'error',
+                                               'message' => 'Just an error',
+                                               'params' => []
+                                       ],
+                               ],
+                               [],
+                       ], [
+                               'Just an error',
+                               'Just a warning',
+                               1,
+                               [],
+                               [
+                                       0 => [
+                                               'type' => 'warning',
+                                               'message' => 'Just a warning',
+                                               'params' => []
+                                       ],
+                               ],
+                       ], [
+                               null,
+                               null,
+                               1,
+                               [],
+                               [],
+                       ], [
+                               null,
+                               null,
+                               0,
+                               [],
+                               [],
+                       ]
+               ];
+       }
+
 }

diff --git a/tests/phpunit/includes/auth/AuthenticationResponseTest.php b/tests/phpunit/includes/auth/AuthenticationResponseTest.php
index 477b161..194b49e 100644 (file)
--- a/tests/phpunit/includes/auth/AuthenticationResponseTest.php
+++ b/tests/phpunit/includes/auth/AuthenticationResponseTest.php
@@ -16,6 +16,7 @@ class AuthenticationResponseTest extends \MediaWikiTestCase {
        public function testConstructors( $constructor, $args, $expect ) {
                if ( is_array( $expect ) ) {
                        $res = new AuthenticationResponse();
+                       $res->messageType = 'warning';
                        foreach ( $expect as $field => $value ) {
                                $res->$field = $value;
                        }
@@ -51,6 +52,7 @@ class AuthenticationResponseTest extends \MediaWikiTestCase {
                        [ 'newFail', [ $msg ], [
                                'status' => AuthenticationResponse::FAIL,
                                'message' => $msg,
+                               'messageType' => 'error',
                        ] ],
 
                        [ 'newRestart', [ $msg ], [
@@ -66,6 +68,21 @@ class AuthenticationResponseTest extends \MediaWikiTestCase {
                                'status' => AuthenticationResponse::UI,
                                'neededRequests' => [ $req ],
                                'message' => $msg,
+                               'messageType' => 'warning',
+                       ] ],
+
+                       [ 'newUI', [ [ $req ], $msg, 'warning' ], [
+                               'status' => AuthenticationResponse::UI,
+                               'neededRequests' => [ $req ],
+                               'message' => $msg,
+                               'messageType' => 'warning',
+                       ] ],
+
+                       [ 'newUI', [ [ $req ], $msg, 'error' ], [
+                               'status' => AuthenticationResponse::UI,
+                               'neededRequests' => [ $req ],
+                               'message' => $msg,
+                               'messageType' => 'error',
                        ] ],
                        [ 'newUI', [ [], $msg ],
                                new \InvalidArgumentException( '$reqs may not be empty' )