From 7b5e9c7856b5262067d539e15af50d6d90c6f59d Mon Sep 17 00:00:00 2001
From: thiemowmde <thiemo.kreuz@wikimedia.de>
Date: Fri, 16 Aug 2024 16:01:20 +0200
Subject: [PATCH] Replace confusing count() on associative arrays with known
 keys

Calling count() on an array with string keys technically works. But
it's confusing and ultimately meaningless. An array might contain
2 elements, but we have no idea how they are named. The code
continues to access some very specific elements, but really just
assumes they are there. We are lucky this never crashed.

We have very powerful language features like isset() or ?? to
express much more precisely what this code is supposed to do.

Change-Id: I7a37371b577c1e44a3c8514316198cf0f4288250
---
 includes/TitleBlacklist.php | 41 +++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/includes/TitleBlacklist.php b/includes/TitleBlacklist.php
index 30f14eb0..2ec88335 100644
--- a/includes/TitleBlacklist.php
+++ b/includes/TitleBlacklist.php
@@ -38,13 +38,9 @@ class TitleBlacklist {
 
 	/**
 	 * Get an instance of this class
-	 *
-	 * @return TitleBlacklist
 	 */
-	public static function singleton() {
-		if ( self::$instance === null ) {
-			self::$instance = new self;
-		}
+	public static function singleton(): self {
+		self::$instance ??= new self();
 		return self::$instance;
 	}
 
@@ -119,23 +115,31 @@ class TitleBlacklist {
 	/**
 	 * Get the text of a blacklist from a specified source
 	 *
-	 * @param array $source A blacklist source from $wgTitleBlacklistSources
+	 * @param array{type: string, src: ?string} $source A blacklist source from $wgTitleBlacklistSources
 	 * @return string The content of the blacklist source as a string
 	 */
 	private static function getBlacklistText( $source ) {
-		if ( !is_array( $source ) || count( $source ) <= 0 ) {
+		if ( !is_array( $source ) || !isset( $source['type'] ) ) {
 			// Return empty string in error case
 			return '';
 		}
 
-		if ( $source['type'] == 'message' ) {
+		if ( $source['type'] === 'message' ) {
 			return wfMessage( 'titleblacklist' )->inContentLanguage()->text();
-		} elseif ( $source['type'] == 'localpage' && count( $source ) >= 2 ) {
-			$title = Title::newFromText( $source['src'] );
-			if ( $title === null ) {
+		}
+
+		$src = $source['src'] ?? null;
+		// All following types require the "src" element in the array
+		if ( !$src ) {
+			return '';
+		}
+
+		if ( $source['type'] === 'localpage' ) {
+			$title = Title::newFromText( $src );
+			if ( !$title ) {
 				return '';
 			}
-			if ( $title->getNamespace() == NS_MEDIAWIKI ) {
+			if ( $title->inNamespace( NS_MEDIAWIKI ) ) {
 				$msg = wfMessage( $title->getText() )->inContentLanguage();
 				return $msg->isDisabled() ? '' : $msg->text();
 			} else {
@@ -145,13 +149,10 @@ class TitleBlacklist {
 					return ( $content instanceof TextContent ) ? $content->getText() : "";
 				}
 			}
-		} elseif ( $source['type'] == 'url' && count( $source ) >= 2 ) {
-			return self::getHttp( $source['src'] );
-		} elseif ( $source['type'] == 'file' && count( $source ) >= 2 ) {
-			if ( !file_exists( $source['src'] ) ) {
-				return '';
-			}
-			return file_get_contents( $source['src'] );
+		} elseif ( $source['type'] === 'url' ) {
+			return self::getHttp( $src );
+		} elseif ( $source['type'] === 'file' ) {
+			return file_exists( $src ) ? file_get_contents( $src ) : '';
 		}
 
 		return '';