From 4bbfe6cb5d3f025469fd238622a731fcc4f8eb2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartosz=20Dziewo=C5=84ski?= Date: Mon, 19 Apr 2021 20:34:55 +0200 Subject: [PATCH] Rename CommentFormatter::addReplyLinks Bug: T280351 Change-Id: I0d7627d63407e11cca6091f78e4d440eec6efa91 --- includes/CommentFormatter.php | 27 +++++++++++++++----------- includes/Hooks/PageHooks.php | 8 ++++---- includes/Hooks/ParserHooks.php | 2 +- modules/controller.js | 2 +- tests/phpunit/CommentFormatterTest.php | 10 +++++----- 5 files changed, 27 insertions(+), 22 deletions(-) diff --git a/includes/CommentFormatter.php b/includes/CommentFormatter.php index c3d3ff676..8a9f305b0 100644 --- a/includes/CommentFormatter.php +++ b/includes/CommentFormatter.php @@ -19,6 +19,8 @@ class CommentFormatter { HookUtils::TOPICSUBSCRIPTION, ]; + protected const MARKER_COMMENT = ''; + // Compatibility with old cached content protected const REPLY_LINKS_COMMENT = ''; /** @@ -34,33 +36,36 @@ class CommentFormatter { } /** - * Add reply links to some HTML + * Add discussion tools to some HTML * * @param string &$text Parser text output * @param Language $lang Interface language */ - public static function addReplyLinks( string &$text, Language $lang ) : void { + public static function addDiscussionTools( string &$text, Language $lang ) : void { $start = microtime( true ); - // Never add links twice. - // This is required because we try again to add links to cached content + // Never add tools twice. + // This is required because we try again to add tools to cached content // to support query string or cookie enabling + if ( strpos( $text, static::MARKER_COMMENT ) !== false ) { + return; + } + // Compatibility with old cached content if ( strpos( $text, static::REPLY_LINKS_COMMENT ) !== false ) { return; } - $text = $text . "\n" . static::REPLY_LINKS_COMMENT; + $text = $text . "\n" . static::MARKER_COMMENT; try { - // Add reply links and hidden data about comment ranges. - $newText = static::addReplyLinksInternal( $text, $lang ); + $newText = static::addDiscussionToolsInternal( $text, $lang ); } catch ( Throwable $e ) { // Catch errors, so that they don't cause the entire page to not display. // Log it and add the request ID in a comment to make it easier to find in the logs. MWExceptionHandler::logException( $e ); $requestId = htmlspecialchars( WebRequest::getRequestId() ); - $info = ""; + $info = ""; $text .= "\n" . $info; return; @@ -74,13 +79,13 @@ class CommentFormatter { } /** - * Add reply links to some HTML + * Add discussion tools to some HTML * * @param string $html HTML * @param Language $lang Interface language - * @return string HTML with reply links + * @return string HTML with discussion tools */ - protected static function addReplyLinksInternal( string $html, Language $lang ) : string { + protected static function addDiscussionToolsInternal( string $html, Language $lang ) : string { // The output of this method can end up in the HTTP cache (Varnish). Avoid changing it; // and when doing so, ensure that frontend code can handle both the old and new outputs. // See controller#init in JS. diff --git a/includes/Hooks/PageHooks.php b/includes/Hooks/PageHooks.php index 0a4ed863d..58ed9de48 100644 --- a/includes/Hooks/PageHooks.php +++ b/includes/Hooks/PageHooks.php @@ -106,13 +106,13 @@ class PageHooks implements */ public function onOutputPageBeforeHTML( $output, &$text ) { $lang = $output->getLanguage(); - // Check after the parser cache if reply links need to be added for + // Check after the parser cache if tools need to be added for // non-cacheable reasons i.e. query string or cookie - // The addReplyLinks method is responsible for ensuring that - // reply links aren't added twice. + // The addDiscussionTools method is responsible for ensuring that + // tools aren't added twice. foreach ( CommentFormatter::USE_WITH_FEATURES as $feature ) { if ( HookUtils::isFeatureEnabledForOutput( $output, $feature ) ) { - CommentFormatter::addReplyLinks( $text, $lang ); + CommentFormatter::addDiscussionTools( $text, $lang ); break; } } diff --git a/includes/Hooks/ParserHooks.php b/includes/Hooks/ParserHooks.php index 3d14d44a8..f79103137 100644 --- a/includes/Hooks/ParserHooks.php +++ b/includes/Hooks/ParserHooks.php @@ -33,7 +33,7 @@ class ParserHooks implements $popts = $parser->getOptions(); // ParserOption for dtreply was set in onArticleParserOptions if ( $popts->getOption( 'dtreply' ) ) { - CommentFormatter::addReplyLinks( $text, $popts->getUserLangObj() ); + CommentFormatter::addDiscussionTools( $text, $popts->getUserLangObj() ); } } diff --git a/modules/controller.js b/modules/controller.js index a5943df9a..024de6256 100644 --- a/modules/controller.js +++ b/modules/controller.js @@ -310,7 +310,7 @@ function init( $container, state ) { // The page can be served from the HTTP cache (Varnish), containing data-mw-comment generated // by an older version of our PHP code. Code below must be able to handle that. - // See CommentFormatter::addReplyLinks() in PHP. + // See CommentFormatter::addDiscussionTools() in PHP. // Iterate over commentNodes backwards so replies are always deserialized before their parents. for ( i = commentNodes.length - 1; i >= 0; i-- ) { diff --git a/tests/phpunit/CommentFormatterTest.php b/tests/phpunit/CommentFormatterTest.php index 1ce2dbf4a..5d6e7a4db 100644 --- a/tests/phpunit/CommentFormatterTest.php +++ b/tests/phpunit/CommentFormatterTest.php @@ -11,10 +11,10 @@ use Wikimedia\TestingAccessWrapper; class CommentFormatterTest extends IntegrationTestCase { /** - * @dataProvider provideAddReplyLinksInternal - * @covers ::addReplyLinksInternal + * @dataProvider provideAddDiscussionToolsInternal + * @covers ::addDiscussionToolsInternal */ - public function testAddReplyLinksInternal( + public function testAddDiscussionToolsInternal( string $name, string $dom, string $expected, string $config, string $data ) : void { $dom = self::getHtml( $dom ); @@ -28,7 +28,7 @@ class CommentFormatterTest extends IntegrationTestCase { $commentFormatter = TestingAccessWrapper::newFromClass( MockCommentFormatter::class ); - $actual = $commentFormatter->addReplyLinksInternal( $dom, RequestContext::getMain()->getLanguage() ); + $actual = $commentFormatter->addDiscussionToolsInternal( $dom, RequestContext::getMain()->getLanguage() ); // Optionally write updated content to the "reply HTML" files if ( getenv( 'DISCUSSIONTOOLS_OVERWRITE_TESTS' ) ) { @@ -38,7 +38,7 @@ class CommentFormatterTest extends IntegrationTestCase { self::assertEquals( $expected, $actual, $name ); } - public function provideAddReplyLinksInternal() : array { + public function provideAddDiscussionToolsInternal() : array { return self::getJson( '../cases/formattedreply.json' ); }