From 0fd9a2b5f4d9484a919b30f48e65ae461bb19572 Mon Sep 17 00:00:00 2001
From: Thiemo Kreuz <thiemo.kreuz@wikimedia.de>
Date: Wed, 9 Jan 2019 11:42:44 +0100
Subject: [PATCH] Fix score calculation in LinksUpdateHookHandler failing on
 unordered input

This patch is motivated by Iad694e0.

* I rearranged the code a little bit to avoid a duplicate line of code.
* I added a ksort() and a comment explaining it.
* Additional tests demonstrate why the ksort() is needed.
* I had to refactor the tests a little bit to allow for more test cases
  that have been missing before.

Bug: T212013
Change-Id: Ia96dc8c6cf57ddcea410a7300756d0013052ac79
---
 includes/LinksUpdateHookHandler.php          | 13 ++--
 tests/phpunit/LinksUpdateHookHandlerTest.php | 68 ++++++++++++--------
 2 files changed, 49 insertions(+), 32 deletions(-)

diff --git a/includes/LinksUpdateHookHandler.php b/includes/LinksUpdateHookHandler.php
index a3998fb..c6a9565 100644
--- a/includes/LinksUpdateHookHandler.php
+++ b/includes/LinksUpdateHookHandler.php
@@ -161,12 +161,15 @@ class LinksUpdateHookHandler {
 	protected function scoreFromTable( $value, array $scores ) {
 		$lastScore = 0;
 
-		foreach ( $scores as $boundary => $score ) {
-			if ( $value <= $boundary ) {
-				return $score;
-			}
-
+		// The loop stops at the *first* match, and therefore *requires* the input array keys to be
+		// in increasing order.
+		ksort( $scores, SORT_NUMERIC );
+		foreach ( $scores as $upperBoundary => $score ) {
 			$lastScore = $score;
+
+			if ( $value <= $upperBoundary ) {
+				break;
+			}
 		}
 
 		return $lastScore;
diff --git a/tests/phpunit/LinksUpdateHookHandlerTest.php b/tests/phpunit/LinksUpdateHookHandlerTest.php
index b7ddb47..94fb9a8 100644
--- a/tests/phpunit/LinksUpdateHookHandlerTest.php
+++ b/tests/phpunit/LinksUpdateHookHandlerTest.php
@@ -290,40 +290,54 @@ class LinksUpdateHookHandlerTest extends MediaWikiTestCase {
 	 * @dataProvider provideScoreFromTable
 	 * @covers \PageImages\Hooks\LinksUpdateHookHandler::scoreFromTable
 	 */
-	public function testScoreFromTable( $type, $value, $expected ) {
-		global $wgPageImagesScores;
-
+	public function testScoreFromTable( array $scores, $value, $expected ) {
 		$handlerWrapper = TestingAccessWrapper::newFromObject( new LinksUpdateHookHandler );
 
-		$score = $handlerWrapper->scoreFromTable( $value, $wgPageImagesScores[$type] );
+		$score = $handlerWrapper->scoreFromTable( $value, $scores );
 		$this->assertEquals( $expected, $score );
 	}
 
 	public function provideScoreFromTable() {
-		return [
-			[ 'width', 100, -100 ],
-			[ 'width', 119, -100 ],
-			[ 'width', 300, 10 ],
-			[ 'width', 400, 10 ],
-			[ 'width', 500, 5 ],
-			[ 'width', 600, 5 ],
-			[ 'width', 601, 0 ],
-			[ 'width', 999, 0 ],
-			[ 'galleryImageWidth', 99, -100 ],
-			[ 'galleryImageWidth', 100, 0 ],
-			[ 'galleryImageWidth', 500, 0 ],
-			[ 'ratio', 1, -100 ],
-			[ 'ratio', 3, -100 ],
-			[ 'ratio', 4, 0 ],
-			[ 'ratio', 5, 0 ],
-			[ 'ratio', 10, 5 ],
-			[ 'ratio', 20, 5 ],
-			[ 'ratio', 25, 0 ],
-			[ 'ratio', 30, 0 ],
-			[ 'ratio', 31, -100 ],
-			[ 'ratio', 40, -100 ],
+		global $wgPageImagesScores;
 
-			'T212013' => [ 'width', 0, -100 ],
+		return [
+			'no match' => [ [], 100, 0 ],
+
+			'always min when below range' => [ [ 200 => 2, 800 => 1 ], 0, 2 ],
+			'always max when above range' => [ [ 200 => 2, 800 => 1 ], 1000, 1 ],
+
+			'always min when below range (reversed)' => [ [ 800 => 1, 200 => 2 ], 0, 2 ],
+			'always max when above range (reversed)' => [ [ 800 => 1, 200 => 2 ], 1000, 1 ],
+
+			'min match' => [ [ 200 => 2, 400 => 3, 800 => 1 ], 200, 2 ],
+			'above min' => [ [ 200 => 2, 400 => 3, 800 => 1 ], 201, 3 ],
+			'second last match' => [ [ 200 => 2, 400 => 3, 800 => 1 ], 400, 3 ],
+			'above second last' => [ [ 200 => 2, 400 => 3, 800 => 1 ], 401, 1 ],
+
+			// These test cases use the default values from extension.json
+			[ $wgPageImagesScores['width'], 100, -100 ],
+			[ $wgPageImagesScores['width'], 119, -100 ],
+			[ $wgPageImagesScores['width'], 300, 10 ],
+			[ $wgPageImagesScores['width'], 400, 10 ],
+			[ $wgPageImagesScores['width'], 500, 5 ],
+			[ $wgPageImagesScores['width'], 600, 5 ],
+			[ $wgPageImagesScores['width'], 601, 0 ],
+			[ $wgPageImagesScores['width'], 999, 0 ],
+			[ $wgPageImagesScores['galleryImageWidth'], 99, -100 ],
+			[ $wgPageImagesScores['galleryImageWidth'], 100, 0 ],
+			[ $wgPageImagesScores['galleryImageWidth'], 500, 0 ],
+			[ $wgPageImagesScores['ratio'], 1, -100 ],
+			[ $wgPageImagesScores['ratio'], 3, -100 ],
+			[ $wgPageImagesScores['ratio'], 4, 0 ],
+			[ $wgPageImagesScores['ratio'], 5, 0 ],
+			[ $wgPageImagesScores['ratio'], 10, 5 ],
+			[ $wgPageImagesScores['ratio'], 20, 5 ],
+			[ $wgPageImagesScores['ratio'], 25, 0 ],
+			[ $wgPageImagesScores['ratio'], 30, 0 ],
+			[ $wgPageImagesScores['ratio'], 31, -100 ],
+			[ $wgPageImagesScores['ratio'], 40, -100 ],
+
+			'T212013' => [ $wgPageImagesScores['width'], 0, -100 ],
 		];
 	}