changetags: Fix management of a '0' tag

The tag wouldn't be treated as active, wouldn't be displayed
in changes lists and it wouldn't be possible to filter it,
although it was possible to create it.

The changes in ::modifyDisplayQuery are needed, because
ContribsPager may provide false.

Add a regression test for ::formatSummaryRow.

Bug: T296642
Change-Id: Iddb1e978387a0009425f6fad1821d9d15c0f5f23

includes/changetags/ChangeTags.php

@@ -168,22 +168,21 @@ class ChangeTags {
 	 * Creates HTML for the given tags
 	 *
 	 * @param string $tags Comma-separated list of tags
-	 * @param string $page A label for the type of action which is being displayed,
+	 * @param string $page Unused
-	 *   for example: 'history', 'contributions' or 'newpages'
+	 * @param MessageLocalizer|null $localizer
-	 * @param IContextSource|null $context
+	 * @note Even though it takes null as a valid argument, a MessageLocalizer is preferred
-	 * @note Even though it takes null as a valid argument, an IContextSource is preferred
 	 *       in a new code, as the null value is subject to change in the future
 	 * @return array Array with two items: (html, classes)
 	 *   - html: String: HTML for displaying the tags (empty string when param $tags is empty)
 	 *   - classes: Array of strings: CSS classes used in the generated html, one class for each tag
 	 * @return-taint onlysafefor_htmlnoent
 	 */
-	public static function formatSummaryRow( $tags, $page, IContextSource $context = null ) {
+	public static function formatSummaryRow( $tags, $page, MessageLocalizer $localizer = null ) {
-		if ( !$tags ) {
+		if ( $tags === '' ) {
 			return [ '', [] ];
 		}
-		if ( !$context ) {
+		if ( !$localizer ) {
-			$context = RequestContext::getMain();
+			$localizer = RequestContext::getMain();
 		}
 
 		$classes = [];
@@ -196,11 +195,11 @@ class ChangeTags {
 
 		$displayTags = [];
 		foreach ( $tags as $tag ) {
-			if ( !$tag ) {
+			if ( $tag === '' ) {
 				continue;
 			}
 			$classes[] = Sanitizer::escapeClass( "mw-tag-$tag" );
-			$description = self::tagDescription( $tag, $context );
+			$description = self::tagDescription( $tag, $localizer );
 			if ( $description === false ) {
 				continue;
 			}
@@ -216,7 +215,7 @@ class ChangeTags {
 			return [ '', $classes ];
 		}
 
-		$markers = $context->msg( 'tag-list-wrapper' )
+		$markers = $localizer->msg( 'tag-list-wrapper' )
 			->numParams( count( $displayTags ) )
 			->rawParams( implode( ' ', $displayTags ) )
 			->parse();
@@ -358,8 +357,18 @@ class ChangeTags {
 		&$rev_id = null, &$log_id = null, $params = null, RecentChange $rc = null,
 		UserIdentity $user = null
 	) {
-		$tagsToAdd = array_filter( (array)$tagsToAdd ); // Make sure we're submitting all tags...
+		$tagsToAdd = array_filter(
-		$tagsToRemove = array_filter( (array)$tagsToRemove );
+			(array)$tagsToAdd, // Make sure we're submitting all tags...
+			static function ( $value ) {
+				return ( $value ?? '' ) !== '';
+			}
+		);
+		$tagsToRemove = array_filter(
+			(array)$tagsToRemove,
+			static function ( $value ) {
+				return ( $value ?? '' ) !== '';
+			}
+		);
 
 		if ( !$rc_id && !$rev_id && !$log_id ) {
 			throw new MWException( 'At least one of: RCID, revision ID, and log ID MUST be ' .
@@ -876,7 +885,7 @@ class ChangeTags {
 	 * @param string|array &$conds Conditions used in query, see Database::select
 	 * @param array &$join_conds Join conditions, see Database::select
 	 * @param string|array &$options Options, see Database::select
-	 * @param string|array $filter_tag Tag(s) to select on (OR)
+	 * @param false|string|array $filter_tag Tag(s) to select on (OR)
 	 * @param bool $exclude If true, exclude tag(s) from $filter_tag (NOR)
 	 *
 	 * @throws MWException When unable to determine appropriate JOIN condition for tagging
@@ -907,7 +916,11 @@ class ChangeTags {
 			throw new MWException( 'Unable to determine appropriate JOIN condition for tagging.' );
 		}
 
-		if ( $useTagFilter && $filter_tag ) {
+		if ( !$useTagFilter ) {
+			return;
+		}
+
+		if ( $filter_tag !== '' && $filter_tag !== false && $filter_tag !== [] ) {
 			// Somebody wants to filter on a tag.
 			// Add an INNER JOIN on change_tag
 			$tagTable = self::getDisplayTableName();
@@ -1654,7 +1667,7 @@ class ChangeTags {
 					$fname
 				);
 
-				return array_filter( array_unique( $tags ) );
+				return array_unique( $tags );
 			},
 			[
 				'checkKeys' => [ $cache->makeKey( 'valid-tags-db' ) ],
@@ -1689,7 +1702,7 @@ class ChangeTags {
 				$setOpts += Database::getCacheSetOptions( wfGetDB( DB_REPLICA ) );
 
 				$hookRunner->onListDefinedTags( $tags );
-				return array_filter( array_unique( $tags ) );
+				return array_unique( $tags );
 			},
 			[
 				'checkKeys' => [ $cache->makeKey( 'valid-tags-hook' ) ],

includes/logging/LogPager.php

@@ -116,7 +116,7 @@ class LogPager extends ReverseChronologicalPager {
 		$this->limitTitle( $page, $pattern );
 		$this->limitAction( $action );
 		$this->getDateCond( $year, $month, $day );
-		$this->mTagFilter = $tagFilter;
+		$this->mTagFilter = (string)$tagFilter;
 	}
 
 	public function getDefaultQuery() {
@@ -376,7 +376,7 @@ class LogPager extends ReverseChronologicalPager {
 		// `logging` and filesorting is somehow better than querying $limit+1 rows from `logging`.
 		// Tell it not to reorder the query. But not when tag filtering or log_search was used, as it
 		// seems as likely to be harmed as helped in that case.
-		if ( !$this->mTagFilter && !array_key_exists( 'ls_field', $this->mConds ) ) {
+		if ( $this->mTagFilter === '' && !array_key_exists( 'ls_field', $this->mConds ) ) {
 			$options[] = 'STRAIGHT_JOIN';
 		}
 

includes/specials/SpecialRecentChanges.php

@@ -366,7 +366,7 @@ class SpecialRecentChanges extends ChangesListSpecialPage {
 		$fields[] = 'page_latest';
 		$join_conds['page'] = [ 'LEFT JOIN', 'rc_cur_id=page_id' ];
 
-		$tagFilter = $opts['tagfilter'] ? explode( '|', $opts['tagfilter'] ) : [];
+		$tagFilter = $opts['tagfilter'] !== '' ? explode( '|', $opts['tagfilter'] ) : [];
 		ChangeTags::modifyDisplayQuery(
 			$tables,
 			$fields,

includes/specials/SpecialRecentChangesLinked.php

@@ -127,7 +127,7 @@ class SpecialRecentChangesLinked extends SpecialRecentChanges {
 		$join_conds['page'] = [ 'LEFT JOIN', 'rc_cur_id=page_id' ];
 		$select[] = 'page_latest';
 
-		$tagFilter = $opts['tagfilter'] ? explode( '|', $opts['tagfilter'] ) : [];
+		$tagFilter = $opts['tagfilter'] !== '' ? explode( '|', $opts['tagfilter'] ) : [];
 		ChangeTags::modifyDisplayQuery(
 			$tables,
 			$select,

includes/specials/SpecialWatchlist.php

@@ -435,7 +435,7 @@ class SpecialWatchlist extends ChangesListSpecialPage {
 			], LIST_OR );
 		}
 
-		$tagFilter = $opts['tagfilter'] ? explode( '|', $opts['tagfilter'] ) : [];
+		$tagFilter = $opts['tagfilter'] !== '' ? explode( '|', $opts['tagfilter'] ) : [];
 		ChangeTags::modifyDisplayQuery(
 			$tables,
 			$fields,

tests/phpunit/includes/changetags/ChangeTagsTest.php

@@ -661,4 +661,62 @@ class ChangeTagsTest extends MediaWikiIntegrationTestCase {
 			[ 'ORDER BY' => 'ctd_name' ]
 		);
 	}
+
+	public function provideFormatSummaryRow() {
+		yield 'nothing' => [ '', [ '', [] ] ];
+		yield 'valid tag' => [
+			'tag1',
+			[
+				'<span class="mw-tag-markers">(tag-list-wrapper: 1, '
+				. '<span class="mw-tag-marker mw-tag-marker-tag1">(tag-tag1)</span>'
+				. ')</span>',
+				[ 'mw-tag-tag1' ]
+			]
+		];
+		yield '0 tag' => [
+			'0',
+			[
+				'<span class="mw-tag-markers">(tag-list-wrapper: 1, '
+				. '<span class="mw-tag-marker mw-tag-marker-0">(tag-0)</span>'
+				. ')</span>',
+				[ 'mw-tag-0' ]
+			]
+		];
+		yield 'hidden tag' => [
+			'hidden-tag',
+			[
+				'',
+				[ 'mw-tag-hidden-tag' ]
+			]
+		];
+		yield 'mutliple tags' => [
+			'tag1,0,,hidden-tag',
+			[
+				'<span class="mw-tag-markers">(tag-list-wrapper: 2, '
+				. '<span class="mw-tag-marker mw-tag-marker-tag1">(tag-tag1)</span>'
+				. ' <span class="mw-tag-marker mw-tag-marker-0">(tag-0)</span>'
+				. ')</span>',
+				[ 'mw-tag-tag1', 'mw-tag-0', 'mw-tag-hidden-tag' ]
+			]
+		];
+	}
+
+	/**
+	 * @dataProvider provideFormatSummaryRow
+	 */
+	public function testFormatSummaryRow( $tags, $expected ) {
+		$qqx = new MockMessageLocalizer();
+		$localizer = $this->createMock( MessageLocalizer::class );
+		$localizer->method( 'msg' )
+			->willReturnCallback( static function ( $key, ...$params ) use ( $qqx ) {
+				if ( $key === 'tag-hidden-tag' ) {
+					return new RawMessage( '-' );
+				}
+				return $qqx->msg( $key, ...$params );
+			} );
+
+		$out = ChangeTags::formatSummaryRow( $tags, 'dummy', $localizer );
+		$this->assertSame( $expected, $out );
+	}
+
 }

tests/phpunit/includes/pager/HistoryPagerTest.php

@@ -103,7 +103,7 @@ class HistoryPagerTest extends MediaWikiLangTestCase {
 			[
 				[
 					'rev_page' => 'title',
-					'ts_tags' => [],
+					'ts_tags' => '',
 					'rev_deleted' => false,
 					'rev_minor_edit' => false,
 					'rev_parent_id' => 1,
@@ -128,7 +128,7 @@ class HistoryPagerTest extends MediaWikiLangTestCase {
 			'rev_page' => 'title',
 			'rev_deleted' => false,
 			'rev_minor_edit' => false,
-			'ts_tags' => [],
+			'ts_tags' => '',
 			'rev_parent_id' => 1,
 			'user_name' => 'Jdlrobson',
 			'rev_comment_data' => '{}',
@@ -173,7 +173,7 @@ class HistoryPagerTest extends MediaWikiLangTestCase {
 			'rev_page' => 'title',
 			'rev_deleted' => false,
 			'rev_minor_edit' => false,
-			'ts_tags' => [],
+			'ts_tags' => '',
 			'rev_parent_id' => 1,
 			'user_name' => 'Jdlrobson',
 			'rev_comment_data' => '{}',