From dd6587c426a23196c019f7ca31e305f5d1de4282 Mon Sep 17 00:00:00 2001 From: Isabelle Hurbain-Palatin Date: Mon, 28 Oct 2024 18:44:08 +0100 Subject: [PATCH] Display Cite error messages in Parsoid Bug: T372709 Depends-On: Ieed7b5a18f5223c7b8a2918df88790d4dc305f9d Change-Id: Ifeb3b216e898bec1c3eb5917262820c5809fda45 --- src/ErrorReporter.php | 5 +- src/Parsoid/ErrorUtils.php | 61 +++++++++ src/Parsoid/RefGroup.php | 11 ++ src/Parsoid/References.php | 23 ++++ .../parser/citeParserTests-knownFailures.json | 8 +- tests/parser/citeParserTests.txt | 119 ++++++++++++------ tests/phpunit/integration/CiteParsoidTest.php | 19 ++- 7 files changed, 198 insertions(+), 48 deletions(-) create mode 100644 src/Parsoid/ErrorUtils.php diff --git a/src/ErrorReporter.php b/src/ErrorReporter.php index a06f662ac..a9f45f375 100644 --- a/src/ErrorReporter.php +++ b/src/ErrorReporter.php @@ -2,6 +2,7 @@ namespace Cite; +use Cite\Parsoid\ErrorUtils; use InvalidArgumentException; use MediaWiki\Html\Html; use MediaWiki\Language\Language; @@ -75,6 +76,9 @@ class ErrorReporter { if ( $type === 'error' ) { // Take care; this is a sideeffect that might not belong to this class. $parser->addTrackingCategory( 'cite-tracking-category-cite-error' ); + if ( ErrorUtils::isDiffingError( $key ) ) { + $parser->addTrackingCategory( 'cite-tracking-category-cite-diffing-error' ); + } } // Optional wrapper messages: cite_error, cite_warning @@ -125,5 +129,4 @@ class ErrorReporter { } return array_slice( $matches, 1 ); } - } diff --git a/src/Parsoid/ErrorUtils.php b/src/Parsoid/ErrorUtils.php new file mode 100644 index 000000000..a07d9fd43 --- /dev/null +++ b/src/Parsoid/ErrorUtils.php @@ -0,0 +1,61 @@ + true, + 'cite_error_ref_no_key' => true, + 'cite_error_ref_too_many_keys' => true, + 'cite_error_references_invalid_parameters' => true, + 'cite_error_ref_invalid_dir' => true, + 'cite_error_ref_follow_conflicts' => true, + 'cite_error_ref_no_input' => true, + 'cite_error_group_refs_without_references' => true, + ]; + return $diffingErrors[$catName] ?? false; + } + + /** + * Creates a document fragment containing the Parsoid rendering of an error message + */ + public static function renderParsoidError( + ParsoidExtensionAPI $extApi, + string $errorKey, + ?array $errorParams + ): DocumentFragment { + $error = new MessageValue( $errorKey, $errorParams ?? [] ); + return self::renderParsoidErrorSpan( $extApi, $error ); + } + + /** + * Adds classes and lead on an existing Parsoid rendering of an error message, sets the tracking category and + * returns the completed fragment + */ + public static function renderParsoidErrorSpan( + ParsoidExtensionAPI $extApi, MessageValue $error + ): DocumentFragment { + $extApi->addTrackingCategory( 'cite-tracking-category-cite-error' ); + $fragment = $extApi->createInterfaceI18nFragment( 'cite_error', [ $error ] ); + $fragSpan = DOMCompat::getLastElementChild( $fragment ); + DOMUtils::addAttributes( $fragSpan, [ 'class' => 'error mw-ext-cite-error' ] ); + return $fragment; + } +} diff --git a/src/Parsoid/RefGroup.php b/src/Parsoid/RefGroup.php index eeb5e2a32..328ae7cd4 100644 --- a/src/Parsoid/RefGroup.php +++ b/src/Parsoid/RefGroup.php @@ -5,6 +5,7 @@ namespace Cite\Parsoid; use Wikimedia\Parsoid\DOM\Document; use Wikimedia\Parsoid\DOM\Element; +use Wikimedia\Parsoid\Ext\DOMDataUtils; use Wikimedia\Parsoid\Ext\DOMUtils; use Wikimedia\Parsoid\Ext\ParsoidExtensionAPI; use Wikimedia\Parsoid\Utils\DOMCompat; @@ -83,6 +84,16 @@ class RefGroup { } $li->appendChild( $reftextSpan ); + foreach ( $ref->nodes as $node ) { + foreach ( DOMDataUtils::getDataMw( $node )->errors ?? [] as $error ) { + $errorFragment = ErrorUtils::renderParsoidError( $extApi, $error->key, $error->params ); + $li->appendChild( $errorFragment ); + if ( ErrorUtils::isDiffingError( $error->key ) ) { + $extApi->addTrackingCategory( 'cite-tracking-category-cite-diffing-error' ); + } + } + } + // mw:referencedBy is added to the for the named refs case // and to the tag to the unnamed refs case. This difference // is used by CSS to style backlinks in MediaWiki:Common.css diff --git a/src/Parsoid/References.php b/src/Parsoid/References.php index 4fafffc00..3560cd454 100644 --- a/src/Parsoid/References.php +++ b/src/Parsoid/References.php @@ -6,11 +6,13 @@ declare( strict_types = 1 ); namespace Cite\Parsoid; +use Cite\Cite; use Cite\MarkSymbolRenderer; use Closure; use MediaWiki\Config\Config; use MediaWiki\MediaWikiServices; use stdClass; +use Wikimedia\Message\MessageValue; use Wikimedia\Parsoid\Core\DomSourceRange; use Wikimedia\Parsoid\DOM\DocumentFragment; use Wikimedia\Parsoid\DOM\Element; @@ -158,6 +160,18 @@ class References extends ExtensionTagHandler { ); } + static $validAttributes = [ + 'group' => true, + 'name' => true, + Cite::SUBREF_ATTRIBUTE => true, + 'follow' => true, + 'dir' => true + ]; + + if ( array_diff_key( (array)$refDmw->attrs, $validAttributes ) !== [] ) { + $errs[] = new DataMwError( 'cite_error_ref_too_many_keys' ); + } + // NOTE: This will have been trimmed in Utils::getExtArgInfo()'s call // to TokenUtils::kvToHash() and ExtensionHandler::normalizeExtOptions() $refName = $refDmw->attrs->name ?? ''; @@ -764,6 +778,7 @@ class References extends ExtensionTagHandler { foreach ( $refsOpts as $key => $value ) { if ( !in_array( strtolower( (string)$key ), $knownAttributes, true ) ) { $extApi->pushError( 'cite_error_references_invalid_parameters' ); + $error = new MessageValue( 'cite_error_references_invalid_parameters' ); break; } } @@ -782,6 +797,14 @@ class References extends ExtensionTagHandler { } ); $domFragment->appendChild( $frag ); + + if ( isset( $error ) ) { + $errorFragment = ErrorUtils::renderParsoidErrorSpan( $extApi, $error ); + // we're pushing it after the reference block while it tends to be before in legacy (error + rerender) + $extApi->addTrackingCategory( 'cite-tracking-category-cite-diffing-error' ); + $frag->appendChild( $errorFragment ); + } + return $domFragment; } diff --git a/tests/parser/citeParserTests-knownFailures.json b/tests/parser/citeParserTests-knownFailures.json index cdd6ed754..7ecd8a7b7 100644 --- a/tests/parser/citeParserTests-knownFailures.json +++ b/tests/parser/citeParserTests-knownFailures.json @@ -73,7 +73,7 @@ }, "Erroneous refs": { "wt2wt": "Zero\n\nAlso zero, but differently! (Normal ref)\n\n\n\n\n\n\n\n\n\n", - "html2wt": "Zero\n\nAlso zero, but differently! (Normal ref)\n\n\n\n\n\n\n\n\n\n", + "html2wt": "Zero\n\nAlso zero, but differently! (Normal ref)\n\n\n\n\n\n\n\n\n", "selser [0,0,4,2,4,0,0,3,0,0,0,0,0]": "Zero\n\nwie687\n\n1u7xv04\n\n1pm36tv\n\n\n\n\n\n\n\n", "selser [3,2,0,0,1,2,0,0,1,0,0,2,0]": "1tb8tca\n\nAlso zero, but differently! (Normal ref)\n\n\n\nl49cv6\n\n\n\n\n\nql4aqu\n\n", "selser [0,3,0,0,0,2,0,0,1,0,0,3,0]": "Zero\n\nAlso zero, but differently! (Normal ref)\n\n\n\n1if3p11\n\n\n\n\n\n", @@ -244,8 +244,8 @@ }, "References: 9. Generate missing references list at the end": { "wt2wt": "A foo\nB bar\n\n", - "html2html": "

A [1] B [inexistent 1]

\n
  1. ↑ foo
    2. \n
\n\n
  1. ↑ bar
    2. \n
", "html2wt": "A foo B bar\n\n", + "html2html": "

A [1] B [inexistent 1]

\n
  1. ↑ foo
    2. \n
\n\n
  1. ↑ bar
    2. \n
", "selser [0,4,0,3,0]": "A foo\nB bar\n\ng0czw3", "selser [4,4,0,4,0]": "whc1pl\n\n1mmf2281nhjc7y", "selser [[2,0,2,0],0,0,3,0]": "1cd0gbiA foo117zkke\nB bar\n", @@ -267,10 +267,6 @@ "selser [[4,0,0,0],3,0,3,0]": "8kgcebfoo\nB bar", "selser [3,0,0,0,0]": "\n" }, - "Report bad attributes in ref tags": { - "wt2html": "

[theGroup 1]

\n
  1. ↑ theValue
    2. \n
", - "html2wt": "Cite error: Invalid parameter in tag\n" - }, "Simple , with in group, with groupname in Chinese": { "html2wt": "AAAref aBBBnote bCCCref c\n\n; refs\n\n\n; notes\n" }, diff --git a/tests/parser/citeParserTests.txt b/tests/parser/citeParserTests.txt index 3874fe7d3..7862bee22 100644 --- a/tests/parser/citeParserTests.txt +++ b/tests/parser/citeParserTests.txt @@ -394,7 +394,7 @@ Regression: non-blank ref "0" followed by ref with content

[1]

-
  1. 1 2 0
+
  1. 1 2 0
!! end !! test @@ -418,7 +418,7 @@ Regression sanity check: non-blank ref "1" followed by ref with content

[1]

-
  1. 1 2 1
+
  1. 1 2 1
!! end !! test @@ -476,6 +476,11 @@ T184912: Consistent normalization of consecutive underscores !! test Erroneous refs +!! options +cat +!! metadata +cat=Pages_with_reference_errors sort= +cat=Pages_with_reference_errors_that_trigger_visual_diffs sort= !! wikitext Zero @@ -515,8 +520,8 @@ Erroneous refs

[5]

-
  1. ↑ Zero
  2. ↑ Also zero, but differently! (Normal ref)
  3. ↑
  4. ↑
  5. ↑
- +
  1. ↑ Zero
  2. ↑ Also zero, but differently! (Normal ref)
  3. ↑
  4. ↑
  5. ↑
+
    !! end @@ -530,7 +535,7 @@ Can't have name="…" and follow="…" the same time

    !! html/parsoid

    [1]

    -
    1. ↑ theValue
    +
    1. ↑ theValue
    !! end !! test @@ -542,8 +547,8 @@ Conflicting name="…" and follow="…" together with another invalid parameter

    Cite error: Invalid parameter in <ref> tag

    !! html/parsoid -

    [1]

    -
    1. ↑ theValue
    +

    [1]

    +
    1. ↑ theValue
    !! end !! test @@ -562,7 +567,7 @@ It's not possible to follow="…" a defined in the section

    !! html/parsoid

    [1]

    -
    1. ↑ theFollows
    2. theValue
    +
    1. ↑ theFollows
    2. theValue
    !! end ## Note that the Cite extension of the legacy parser is putting paragraphs @@ -586,7 +591,7 @@ A follow="…" before its parent is not merged !! html/parsoid

    [1] [2]

    -
    1. ↑ theFollows
    2. ↑ theValue
    +
    1. ↑ theFollows
    2. ↑ theValue
    !! end ## This is a nasty edge case which was dropping the ref entirely from the @@ -617,7 +622,7 @@ A follow="…" before its parent is not merged [2] [3]

    -
    1. ↑ First
    2. ↑ Second
    3. ↑ Third
    +
    1. ↑ First
    2. ↑ Second
    3. ↑ Third
    !! end !! test @@ -646,7 +651,7 @@ A follow="…" before its parent is not merged [2] [3]

    -
    1. ↑ First
    2. ↑ Second
    3. ↑ Third
    +
    1. ↑ First
    2. ↑ Second
    3. ↑ Third
    !! end !! test @@ -705,7 +710,7 @@ Follow following a named ref with multiple definitions

    [1] [1] [1]

    -
    1. 1 2 123 345
    +
    1. 1 2 123 345
    !! end !! test @@ -736,15 +741,22 @@ MOD
    1. 1 2 theFollowValue anotherFollowValue
    !!end -# T307741: Parsoid fails this test in both standalone and integrated modes. !! test Report bad attributes in ref tags !! wikitext theValue -!! html +!! html/php

    Cite error: Invalid parameter in <ref> tag

    +!! html/parsoid +

    [theGroup 1]

    +
    +
      +
    1. ↑ theValue
      2. +
    +
    + !! end !! test @@ -965,7 +977,7 @@ T242437 - Nested references edge case, inner tag function with LDR !! html/parsoid

    [2 1]

    -
    1. ↑
    2. BAR
    3. BAR BAR
    +
    1. ↑
    2. BAR
    3. BAR BAR
    1. bad group
    !! end @@ -979,10 +991,10 @@ THREECONTENT !! html/parsoid

    ONE[1]

    -
    1. ↑
    +
    1. ↑

    TWO[NOTES 1] THREE[NOTES 2]

    -
    1. ↑
    2. ↑ CONTENT
    +
    1. ↑
    2. ↑ CONTENT
    !! html/php

    ONE[1]

    @@ -1029,7 +1041,7 @@ Error conditions on non-visible content

    [1] [2] [3]

    -
    1. ↑ x
    2. ↑
    3. ↑
    +
    1. ↑ x
    2. ↑
    3. ↑
    !! end # This article is used in the ' with custom group link' test below @@ -1143,7 +1155,7 @@ zeroCite error: The op

    Bla.[1] foo.[2] zero[3]

    -
    1. ↑
    2. ↑
    3. ↑
    +
    1. ↑
    2. ↑
    3. ↑
    !! end !! test @@ -1156,7 +1168,7 @@ Bla.

    !! html/parsoid

    Bla.[1]

    -
    1. ↑
    +
    1. ↑
    !! end !! test @@ -1173,7 +1185,7 @@ Ho Cite error: The ope !! html/parsoid

    Hi [1] Ho [2]

    -
    1. ↑
    2. ↑
    +
    1. ↑
    2. ↑
    !! end !! test @@ -1189,7 +1201,7 @@ Bla. !! html/parsoid

    Bla.[1]

    -
    1. ↑
    +
    1. ↑
    !! end !! test @@ -1229,7 +1241,7 @@ Multiple definition (outside ) !! html/parsoid

    [1] [1]

    -
    1. 1 2 abc
    +
    1. 1 2 abc
    !! end !! test @@ -1290,7 +1302,7 @@ T202593: Conflicting dir attributes [1]

      -
    1. 1 2 abc
      2. +
    2. 1 2 abc
    !! end @@ -1314,7 +1326,7 @@ T202593: Conflicting dir attributes with the full ref first [1]

      -
    1. 1 2 abc
      2. +
    2. 1 2 abc
    !! end @@ -1419,7 +1431,7 @@ C

    A [1] B [2] C [3]

    -
    1. ↑ foo
    2. ↑ foo
    3. ↑
    +
    1. ↑ foo
    2. ↑ foo
    3. ↑
    !! end !! test @@ -1758,7 +1770,7 @@ C B [1] C [1]

    -
    1. 1 2 3 Foo one
      2. +
      1. 1 2 3 Foo one
      !! end @@ -1784,7 +1796,7 @@ POST [1] THEVALUE [2] POST [3]

      -
      1. ↑ preValue
      2. ↑ theValue
      3. ↑ postValue
      +
      1. ↑ preValue
      2. ↑ theValue
      3. ↑ postValue
      !! end !! test @@ -1800,7 +1812,7 @@ Verify invalid use of a numeric character in a ref name and follow !! html/parsoid

      [1] [1]

      -
      1. ↑ theValue theFollow
      +
      1. ↑ theValue theFollow
      !! end !! test @@ -2023,16 +2035,16 @@ BETA[NOTES 1] TWO[NOTES 1]

      1. 1 2 food
      -
      1. 1 2
      +
      1. 1 2

      THREE[NOTES 1] FOUR[NOTES 2] FIVE[NOTES 2] SIX[1]

      -
      1. ↑ CONTENT
      2. 1 2
      +
      1. ↑ CONTENT
      2. 1 2
      1. ↑ NOGROUPCONTENT

      SEVEN[NOTES 1] EIGHT[NOTES 2]

      -
      1. ↑
      2. ↑
      +
      1. ↑
      2. ↑

      NINE[NOTES 1] TEN[NOTES 2]

      1. ↑ NINECONTENT
      2. ↑ TENCONTENT
      @@ -2060,7 +2072,7 @@ B bar
    2. ↑ foo
      -
    1. ↑ bar
      2. +
    2. ↑ bar
    !! end @@ -2187,9 +2199,9 @@ aDifferentNameGROUP3 [theGroup2 2] anotherGROUP3 [theGroup3 1] aDifferentNameGROUP [theGroup 2]

    -
    1. 1 2 theValue3
    2. ↑ anotherValue3differentName
    -
    1. ↑ theValue
    2. ↑ anotherValueDifferentName
    -
    1. ↑ theValue2
    2. ↑ anotherValue2DifferentName
    +
    1. 1 2 theValue3
    2. ↑ anotherValue3differentName
    +
    1. ↑ theValue
    2. ↑ anotherValueDifferentName
    +
    1. ↑ theValue2
    2. ↑ anotherValue2DifferentName
    !! end # Doesn't wt2wt cleanly because we don't encode the & in html2wt direction @@ -2617,7 +2629,7 @@ T15673: with direction "nonsense" and "" (empty), strip invalid dir attrib !! html/parsoid

    [1] [2]

    -
    1. ↑ NONSENSE_DIR_TEST
    2. ↑ EMPTY_DIR_TEST
    +
    1. ↑ NONSENSE_DIR_TEST
    2. ↑ EMPTY_DIR_TEST
    !! end !! test @@ -2982,11 +2994,14 @@ Report bad attributes in reference tags


    Cite error: <ref> tags exist for a group named "theGroup", but no corresponding <references group="theGroup"/> tag was found

    !! html/parsoid

    [1]

    -
    1. ↑ noGroupOrNameValue
    +
    1. ↑ noGroupOrNameValue
    +

    [1]

    -
    1. ↑ theNameValue
    +
    1. ↑ theNameValue
    +

    [theGroup 1]

    -
    1. ↑ theGroupValue
    +
    1. ↑ theGroupValue
    +
    !! end !! test @@ -3354,3 +3369,27 @@ Sections: Sections: h2 index:1 toclevel:1 number:1 title:Parser_test off:0 anchor/linkAnchor:c[1] line:c[1] !! end + +!! test +Non-diffing cite error +!! options +cat +parsoid="wt2html" +!! wikitext + +!! metadata +cat=Pages_with_reference_errors sort= +!! html/php +

    [1] +

    +
      +
    1. ↑ Cite error: Invalid <ref> tag; no text was provided for refs named aaa
      2. +
    +!! html/parsoid +

    [1]

    +
    +
      +
    1. ↑
      2. +
    +
    +!! end diff --git a/tests/phpunit/integration/CiteParsoidTest.php b/tests/phpunit/integration/CiteParsoidTest.php index 863602280..dc0964878 100644 --- a/tests/phpunit/integration/CiteParsoidTest.php +++ b/tests/phpunit/integration/CiteParsoidTest.php @@ -8,6 +8,8 @@ use Cite\Parsoid\References; use MediaWiki\Config\Config; use MediaWiki\Registration\ExtensionRegistry; use Wikimedia\ObjectFactory\ObjectFactory; +use Wikimedia\Parsoid\Config\PageConfig; +use Wikimedia\Parsoid\Core\ContentMetadataCollector; use Wikimedia\Parsoid\Core\SelserData; use Wikimedia\Parsoid\DOM\Element; use Wikimedia\Parsoid\Ext\ParsoidExtensionAPI; @@ -20,6 +22,7 @@ use Wikimedia\Parsoid\Parsoid; use Wikimedia\Parsoid\Utils\DOMCompat; use Wikimedia\Parsoid\Utils\DOMDataUtils; use Wikimedia\Parsoid\Utils\DOMUtils; +use Wikimedia\Parsoid\Utils\TitleValue; /** * @coversDefaultClass \Cite\Parsoid\Cite @@ -91,7 +94,21 @@ class CiteParsoidTest extends \MediaWikiIntegrationTestCase { $siteOptions = [ 'linting' => true ] + $options; $siteConfig = $this->getSiteConfig( $siteOptions ); - $dataAccess = new MockDataAccess( $siteConfig, [] ); + $dataAccess = new class( $siteConfig, [] ) extends MockDataAccess { + /** @inheritDoc */ + public function addTrackingCategory( + PageConfig $pageConfig, + ContentMetadataCollector $metadata, + string $key + ): void { + if ( $key === 'cite-tracking-category-cite-error' ) { + $tv = TitleValue::tryNew( 14, 'Pages with reference errors' ); + $metadata->addCategory( $tv ); + return; + } + parent::addTrackingCategory( $pageConfig, $metadata, $key ); + } + }; $parsoid = new Parsoid( $siteConfig, $dataAccess ); $content = new MockPageContent( [ $opts['pageName'] => $wt ] );