From 12c7ad7504b6d28c71bda4b56ef2c656971db084 Mon Sep 17 00:00:00 2001 From: thiemowmde Date: Tue, 12 Dec 2023 14:05:44 +0100 Subject: [PATCH] Get rid of "guarded " terminology This patch only moves existing code around without changing any behavior. What I basically did was merging the old "guardedReferences" method into "references", and then splitting the resulting code in other ways. Now we see a few other concepts emerging. But the idea something would be "guarded" (how?) is gone. The most critical detail in this patch are the new method names, and how the code is split. The names should tell a story, and the methods should do exactly what the name says. Suggestions? Bug: T353266 Change-Id: I8b7921ce24487e9657e4193ea6a2e3e7d7b0b1c3 --- src/Cite.php | 101 +++++++++++++------------ tests/phpunit/integration/CiteTest.php | 1 - 2 files changed, 53 insertions(+), 49 deletions(-) diff --git a/src/Cite.php b/src/Cite.php index 523d134e4..21ca4a081 100644 --- a/src/Cite.php +++ b/src/Cite.php @@ -271,6 +271,7 @@ class Cite { $status->merge( $this->validateRef( $text, ...array_values( $arguments ) ) ); if ( !$status->isGood() && $this->inReferencesGroup !== null ) { + // We know we are in the middle of a tag and can't display errors in place foreach ( $status->getErrors() as $error ) { $this->mReferencesErrors[] = [ $error['message'], ...$error['params'] ]; } @@ -365,71 +366,75 @@ class Cite { } $status = $this->parseArguments( $argv, [ 'group', 'responsive' ] ); - $this->inReferencesGroup = $status->getValue()['group'] ?? self::DEFAULT_GROUP; - $ret = $this->guardedReferences( $parser, $text, $status ); + $arguments = $status->getValue(); + + $this->inReferencesGroup = $arguments['group'] ?? self::DEFAULT_GROUP; + + $status->merge( $this->parseReferencesTagContent( $parser, $text ) ); + if ( !$status->isGood() ) { + $error = $status->getErrors()[0]; + $ret = $this->errorReporter->halfParsed( $parser, $error['message'], ...$error['params'] ); + } else { + $responsive = $arguments['responsive']; + $ret = $this->formatReferences( $parser, $this->inReferencesGroup, $responsive ); + // Append errors collected while {@see parseReferencesTagContent} processed tags + // in + $ret .= $this->formatReferencesErrors( $parser ); + } + $this->inReferencesGroup = null; return $ret; } /** - * Must only be called from references(). Use that to prevent recursion. - * * @param Parser $parser * @param ?string $text Raw, untrimmed wikitext content of the tag, if any - * @param StatusValue $status with the arguments as given in * - * @return string HTML + * @return StatusValue */ - private function guardedReferences( - Parser $parser, - ?string $text, - StatusValue $status - ): string { - $responsive = $status->getValue()['responsive']; - - if ( $text !== null && trim( $text ) !== '' ) { - if ( preg_match( '{' . preg_quote( Parser::MARKER_PREFIX ) . '-(?i:references)-}', $text ) ) { - return $this->errorReporter->halfParsed( $parser, 'cite_error_included_references' ); - } - - // Detect whether we were sent already rendered s. Mostly a side effect of using - // {{#tag:references}}. The following assumes that the parsed s sent within the - // block were the most recent calls to . This assumption is true for - // all known use cases, but not strictly enforced by the parser. It is possible that - // some unusual combination of #tag, and conditional parser functions could - // be created that would lead to malformed references here. - preg_match_all( '{' . preg_quote( Parser::MARKER_PREFIX ) . '-(?i:ref)-}', $text, $matches ); - $count = count( $matches[0] ); - - // Undo effects of calling while unaware of being contained in - foreach ( $this->referenceStack->rollbackRefs( $count ) as $call ) { - // Rerun call with the context now being known - $this->guardedRef( $parser, ...$call ); - } - - // Parse the content to process any unparsed tags - $parser->recursiveTagParse( $text ); + private function parseReferencesTagContent( Parser $parser, ?string $text ): StatusValue { + // Nothing to parse in an empty tag + if ( $text === null || trim( $text ) === '' ) { + return StatusValue::newGood(); } - if ( !$status->isGood() ) { - // Bail out with an error. - $error = $status->getErrors()[0]; - return $this->errorReporter->halfParsed( $parser, $error['message'], ...$error['params'] ); + if ( preg_match( '{' . preg_quote( Parser::MARKER_PREFIX ) . '-(?i:references)-}', $text ) ) { + return StatusValue::newFatal( 'cite_error_included_references' ); } - $s = $this->formatReferences( $parser, $this->inReferencesGroup, $responsive ); + // Detect whether we were sent already rendered s. Mostly a side effect of using + // {{#tag:references}}. The following assumes that the parsed s sent within the + // block were the most recent calls to . This assumption is true for + // all known use cases, but not strictly enforced by the parser. It is possible that + // some unusual combination of #tag, and conditional parser functions could + // be created that would lead to malformed references here. + preg_match_all( '{' . preg_quote( Parser::MARKER_PREFIX ) . '-(?i:ref)-}', $text, $matches ); + $count = count( $matches[0] ); - // Append errors generated while processing - if ( $this->mReferencesErrors ) { - $html = []; - foreach ( $this->mReferencesErrors as $msg ) { - $html[] = $this->errorReporter->halfParsed( $parser, ...$msg ); + // Undo effects of calling while unaware of being contained in + foreach ( $this->referenceStack->rollbackRefs( $count ) as $call ) { + // Rerun call with the context now being known + $this->guardedRef( $parser, ...$call ); + } + + // Parse the content to process any unparsed tags, but drop the resulting + // HTML + $parser->recursiveTagParse( $text ); + + return StatusValue::newGood(); + } + + private function formatReferencesErrors( Parser $parser ): string { + $html = ''; + foreach ( $this->mReferencesErrors as $msg ) { + if ( $html ) { + $html .= "
\n"; } - $s .= "\n" . implode( "
\n", $html ); - $this->mReferencesErrors = []; + $html .= $this->errorReporter->halfParsed( $parser, ...$msg ); } - return $s; + $this->mReferencesErrors = []; + return $html ? "\n$html" : ''; } /** diff --git a/tests/phpunit/integration/CiteTest.php b/tests/phpunit/integration/CiteTest.php index 448544ecd..de049e0bf 100644 --- a/tests/phpunit/integration/CiteTest.php +++ b/tests/phpunit/integration/CiteTest.php @@ -373,7 +373,6 @@ class CiteTest extends \MediaWikiIntegrationTestCase { /** * @covers ::references - * @covers ::guardedReferences * @dataProvider provideGuardedReferences */ public function testGuardedReferences(