From 4834340ec010bf0dc42116fbb6b1593962d2e4b0 Mon Sep 17 00:00:00 2001 From: "C. Scott Ananian" Date: Fri, 15 Oct 2021 15:42:40 -0400 Subject: [PATCH] Deprecate ParserOutput::addWarning() in favor of ::addWarningMsg() Encourage localization and factor out common code by taking a message key as the first argument to ::addWarningMsg() instead of a wikitext string. This also plays nicer with Parsoid by separating out the localization code from the parse. Bug: T293515 Change-Id: I6a7c04c67ac586ab00d4edcbb3d09485a7794e23 --- RELEASE-NOTES-1.38 | 1 + includes/parser/CoreParserFunctions.php | 9 ++++---- includes/parser/PPFrame_Hash.php | 21 +++++++++++-------- includes/parser/Parser.php | 16 ++++++++------ includes/parser/ParserOutput.php | 17 +++++++++++++++ .../includes/parser/ParserOutputTest.php | 16 +++++++++----- 6 files changed, 55 insertions(+), 25 deletions(-) diff --git a/RELEASE-NOTES-1.38 b/RELEASE-NOTES-1.38 index 5b6e2a1aee8..6526572d737 100644 --- a/RELEASE-NOTES-1.38 +++ b/RELEASE-NOTES-1.38 @@ -230,6 +230,7 @@ because of Phabricator reports. - ::getFlag() - use ::getOutputFlag() - ::setFlag() - use ::setOutputFlag() - ::getAllFlags() - this method is now marked @internal + - ::addWarning() - use ::addWarningMsg() * The following methods were deprecated; use ::setPreventClickjacking(..) instead: - OutputPage::preventClickjacking() diff --git a/includes/parser/CoreParserFunctions.php b/includes/parser/CoreParserFunctions.php index 98f8b6d7f58..b9d8f1bf1c8 100644 --- a/includes/parser/CoreParserFunctions.php +++ b/includes/parser/CoreParserFunctions.php @@ -501,11 +501,10 @@ class CoreParserFunctions { return ''; } } else { - $parser->getOutput()->addWarning( - wfMessage( 'restricted-displaytitle', - // Message should be parsed, but this param should only be escaped. - wfEscapeWikiText( $text ) - )->text() + $parser->getOutput()->addWarningMsg( + 'restricted-displaytitle', + // Message should be parsed, but this param should only be escaped. + Message::plaintextParam( $text ) ); $parser->addTrackingCategory( 'restricted-displaytitle-ignored' ); } diff --git a/includes/parser/PPFrame_Hash.php b/includes/parser/PPFrame_Hash.php index 7d36eeee84a..2301cdf3f7e 100644 --- a/includes/parser/PPFrame_Hash.php +++ b/includes/parser/PPFrame_Hash.php @@ -121,10 +121,12 @@ class PPFrame_Hash implements PPFrame { // Numbered parameter $index = $bits['index'] - $indexOffset; if ( isset( $namedArgs[$index] ) || isset( $numberedArgs[$index] ) ) { - $this->parser->getOutput()->addWarning( wfMessage( 'duplicate-args-warning', - wfEscapeWikiText( $this->title ), - wfEscapeWikiText( $title ), - wfEscapeWikiText( $index ) )->text() ); + $this->parser->getOutput()->addWarningMsg( + 'duplicate-args-warning', + Message::plaintextParam( (string)$this->title ), + Message::plaintextParam( (string)$title ), + Message::numParam( $index ) + ); $this->parser->addTrackingCategory( 'duplicate-args-category' ); } $numberedArgs[$index] = $bits['value']; @@ -133,11 +135,12 @@ class PPFrame_Hash implements PPFrame { // Named parameter $name = trim( $this->expand( $bits['name'], PPFrame::STRIP_COMMENTS ) ); if ( isset( $namedArgs[$name] ) || isset( $numberedArgs[$name] ) ) { - $this->parser->getOutput()->addWarning( wfMessage( 'duplicate-args-warning', - wfEscapeWikiText( $this->title ), - wfEscapeWikiText( $title ), - // @phan-suppress-next-line SecurityCheck-DoubleEscaped taint track for named args - wfEscapeWikiText( $name ) )->text() ); + $this->parser->getOutput()->addWarningMsg( + 'duplicate-args-warning', + Message::plaintextParam( (string)$this->title ), + Message::plaintextParam( (string)$title ), + Message::plaintextParam( (string)$name ) + ); $this->parser->addTrackingCategory( 'duplicate-args-category' ); } $namedArgs[$name] = $bits['value']; diff --git a/includes/parser/Parser.php b/includes/parser/Parser.php index fc2926f6852..1f20b91bc23 100644 --- a/includes/parser/Parser.php +++ b/includes/parser/Parser.php @@ -2950,9 +2950,11 @@ class Parser { # does no harm if $current and $max are present but are unnecessary for the message # Not doing ->inLanguage( $this->mOptions->getUserLangObj() ), since this is shown # only during preview, and that would split the parser cache unnecessarily. - $warning = wfMessage( "$limitationType-warning" )->numParams( $current, $max ) - ->text(); - $this->mOutput->addWarning( $warning ); + $this->mOutput->addWarningMsg( + "$limitationType-warning", + Message::numParam( $current ), + Message::numParam( $max ) + ); $this->addTrackingCategory( "$limitationType-category" ); } @@ -3237,8 +3239,10 @@ class Parser { . wfMessage( 'parser-template-loop-warning', $titleText )->inContentLanguage()->text() . ''; $this->addTrackingCategory( 'template-loop-category' ); - $this->mOutput->addWarning( wfMessage( 'template-loop-warning', - wfEscapeWikiText( $titleText ) )->text() ); + $this->mOutput->addWarningMsg( + 'template-loop-warning', + Message::plaintextParam( $titleText ) + ); $this->logger->debug( __METHOD__ . ": template loop broken at '$titleText'" ); } } @@ -3283,7 +3287,7 @@ class Parser { // T91154: {{=}} is deprecated when it doesn't expand to `=`; // use {{Template:=}} if you must. $this->addTrackingCategory( 'template-equals-category' ); - $this->mOutput->addWarning( wfMessage( 'template-equals-warning' )->text() ); + $this->mOutput->addWarningMsg( 'template-equals-warning' ); } # Replace raw HTML by a placeholder diff --git a/includes/parser/ParserOutput.php b/includes/parser/ParserOutput.php index aca9676dbc7..908d41329c8 100644 --- a/includes/parser/ParserOutput.php +++ b/includes/parser/ParserOutput.php @@ -838,10 +838,27 @@ class ParserOutput extends CacheTime { $this->mLanguageLinks[] = $t; } + /** + * @deprecated since 1.38; use ::addWarningMsg() instead + */ public function addWarning( $s ) { $this->mWarnings[$s] = 1; } + /** + * Add a warning to the output for this page. + * @param string $msg The localization message key for the warning + * @param mixed ...$args Optional arguments for the message + * @since 1.38 + */ + public function addWarningMsg( string $msg, ...$args ): void { + $s = wfMessage( $msg, ...$args ) + // some callers set the title here? + ->inContentLanguage() // because this ends up in cache + ->text(); + $this->mWarnings[$s] = 1; + } + public function addOutputHook( $hook, $data = false ) { $this->mOutputHooks[] = [ $hook, $data ]; } diff --git a/tests/phpunit/includes/parser/ParserOutputTest.php b/tests/phpunit/includes/parser/ParserOutputTest.php index b3008c7845b..070e2fe2797 100644 --- a/tests/phpunit/includes/parser/ParserOutputTest.php +++ b/tests/phpunit/includes/parser/ParserOutputTest.php @@ -816,8 +816,8 @@ EOF // flags & co $a = new ParserOutput(); - $a->addWarning( 'Oops' ); - $a->addWarning( 'Whoops' ); + $a->addWarningMsg( 'duplicate-args-warning', 'A', 'B', 'C' ); + $a->addWarningMsg( 'template-loop-warning', 'D' ); $a->setFlag( 'foo' ); $a->setFlag( 'bar' ); @@ -827,8 +827,9 @@ EOF $b = new ParserOutput(); - $b->addWarning( 'Yikes' ); - $b->addWarning( 'Whoops' ); + $b->addWarningMsg( 'template-equals-warning' ); + $b->addWarningMsg( 'template-loop-warning', 'D' ); + $b->addWarning( 'Old School' ); // test the deprecated ::addWarning() $b->setFlag( 'zoo' ); $b->setFlag( 'bar' ); @@ -837,7 +838,12 @@ EOF $b->recordOption( 'Bar' ); yield 'flags' => [ $a, $b, [ - 'getWarnings' => [ 'Oops', 'Whoops', 'Yikes' ], + 'getWarnings' => [ + wfMessage( 'duplicate-args-warning', 'A', 'B', 'C' )->text(), + wfMessage( 'template-loop-warning', 'D' )->text(), + wfMessage( 'template-equals-warning' )->text(), + 'Old School', + ], '$mFlags' => [ 'foo' => true, 'bar' => true, 'zoo' => true ], 'getUsedOptions' => [ 'Foo', 'Bar', 'Zoo' ], ] ];