From 65c8967c32b7de4ea4438e661d3b31003a2df6f1 Mon Sep 17 00:00:00 2001
From: Thiemo Kreuz <thiemo.kreuz@wikimedia.de>
Date: Thu, 21 Nov 2019 12:52:47 +0100
Subject: [PATCH] =?UTF-8?q?Fix=20internal=20presentation=20of=20the=20dir?=
 =?UTF-8?q?=3D"=E2=80=A6"=20attribute?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This fixes a whole bunch of inconsistencies:

* The dir attribute is now trimmed, as most others already are. This is
an actual user-facing change.

* The internal representation is now false in case the value was invalid,
not an empty string any more.

* Null means the attribute was not present. This is now always used,
even in the return values that are meant to represent an error state. No
existing behavior changes.

* The internal representation does not contain an HTML snippet any more,
but the raw value "ltr" or "rtl", or null. Note this might influence the
API, because the API actually exposes the internal representation.
However, we are pretty sure the API is not used anywhere. Even if,
exposing HTML code was most certainly an unwanted and unexpected effect
of the patch that introduced the dir attribute. This does make this a
bugfix, I would argue.

Change-Id: Ic385d9ab36fa0545c374d3d63063028ae4e449d4
---
 src/Cite.php                    | 21 +++++++++------------
 tests/phpunit/unit/CiteTest.php | 18 +++++++++---------
 2 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/src/Cite.php b/src/Cite.php
index d3a0357c9..5267f54c1 100644
--- a/src/Cite.php
+++ b/src/Cite.php
@@ -244,8 +244,8 @@ class Cite {
 	) {
 		# The key here is the "name" attribute.
 		list( $key, $group, $follow, $dir, $extends ) = $this->refArg( $argv );
-		// empty string indicate invalid dir
-		if ( $dir === '' && $text !== '' ) {
+
+		if ( $dir === false && $text !== '' ) {
 			$text .= $this->errorReporter->wikitext( 'cite_error_ref_invalid_dir', $argv['dir'] );
 		}
 		# Split these into groups.
@@ -417,13 +417,10 @@ class Cite {
 		$extends = null;
 
 		if ( isset( $argv['dir'] ) ) {
-			// compare the dir attribute value against an explicit whitelist.
-			$dir = '';
-			$isValidDir = in_array( strtolower( $argv['dir'] ), [ 'ltr', 'rtl' ] );
-			if ( $isValidDir ) {
-				$dir = Html::expandAttributes( [ 'class' => 'mw-cite-dir-' . strtolower( $argv['dir'] ) ] );
+			$dir = strtolower( trim( $argv['dir'] ) );
+			if ( !in_array( $dir, [ 'ltr', 'rtl' ] ) ) {
+				$dir = false;
 			}
-
 			unset( $argv['dir'] );
 		}
 
@@ -435,7 +432,7 @@ class Cite {
 		if ( isset( $argv['follow'] ) &&
 			( isset( $argv['name'] ) || isset( $argv[self::BOOK_REF_ATTRIBUTE] ) )
 		) {
-			return [ false, false, false, false, false ];
+			return [ false, false, false, null, false ];
 		}
 
 		if ( isset( $argv['name'] ) ) {
@@ -460,7 +457,7 @@ class Cite {
 
 		if ( $argv !== [] ) {
 			// Unexpected invalid attribute.
-			return [ false, false, false, false, false ];
+			return [ false, false, false, null, false ];
 		}
 
 		return [ $key, $group, $follow, $dir, $extends ];
@@ -837,7 +834,7 @@ class Cite {
 				$this->normalizeKey( self::getReferencesKey( $id ) ),
 				$this->normalizeKey( $backlinkId ),
 				$text,
-				$val['dir']
+				$val['dir'] ? Html::expandAttributes( [ 'class' => 'mw-cite-dir-' . $val['dir'] ] ) : ''
 			)->inContentLanguage()->plain();
 		}
 
@@ -861,7 +858,7 @@ class Cite {
 			),
 			$this->listToText( $backlinks ),
 			$text,
-			$val['dir']
+			$val['dir'] ? Html::expandAttributes( [ 'class' => 'mw-cite-dir-' . $val['dir'] ] ) : ''
 		)->inContentLanguage()->plain();
 	}
 
diff --git a/tests/phpunit/unit/CiteTest.php b/tests/phpunit/unit/CiteTest.php
index 072947044..e63c2ef91 100644
--- a/tests/phpunit/unit/CiteTest.php
+++ b/tests/phpunit/unit/CiteTest.php
@@ -35,27 +35,27 @@ class CiteTest extends \MediaWikiUnitTestCase {
 			[ [], [ null, null, false, null, null ] ],
 
 			// One attribute only
-			[ [ 'dir' => 'invalid' ], [ null, null, false, '', null ] ],
-			[ [ 'dir' => 'rtl' ], [ null, null, false, ' class="mw-cite-dir-rtl"', null ] ],
+			[ [ 'dir' => 'invalid' ], [ null, null, false, false, null ] ],
+			[ [ 'dir' => ' rtl ' ], [ null, null, false, 'rtl', null ] ],
 			[ [ 'follow' => ' f ' ], [ null, null, 'f', null, null ] ],
 			// FIXME: Unlike all other attributes, group isn't trimmed. Why?
 			[ [ 'group' => ' g ' ], [ null, ' g ', null, null, null ] ],
-			[ [ 'invalid' => 'i' ], [ false, false, false, false, false ] ],
-			[ [ 'invalid' => null ], [ false, false, false, false, false ] ],
+			[ [ 'invalid' => 'i' ], [ false, false, false, null, false ] ],
+			[ [ 'invalid' => null ], [ false, false, false, null, false ] ],
 			[ [ 'name' => ' n ' ], [ 'n', null, null, null, null ] ],
-			[ [ 'name' => null ], [ false, false, false, false, false ] ],
+			[ [ 'name' => null ], [ false, false, false, null, false ] ],
 			[ [ 'extends' => ' e ' ], [ null, null, null, null, 'e' ] ],
 
 			// Pairs
-			[ [ 'follow' => 'f', 'name' => 'n' ], [ false, false, false, false, false ] ],
-			[ [ 'follow' => null, 'name' => null ], [ false, false, false, false, false ] ],
-			[ [ 'follow' => 'f', 'extends' => 'e' ], [ false, false, false, false, false ] ],
+			[ [ 'follow' => 'f', 'name' => 'n' ], [ false, false, false, null, false ] ],
+			[ [ 'follow' => null, 'name' => null ], [ false, false, false, null, false ] ],
+			[ [ 'follow' => 'f', 'extends' => 'e' ], [ false, false, false, null, false ] ],
 			[ [ 'group' => 'g', 'name' => 'n' ], [ 'n', 'g', null, null, null ] ],
 
 			// Combinations of 3 or more attributes
 			[
 				[ 'group' => 'g', 'name' => 'n', 'extends' => 'e', 'dir' => 'rtl' ],
-				[ 'n', 'g', null, ' class="mw-cite-dir-rtl"', 'e' ]
+				[ 'n', 'g', null, 'rtl', 'e' ]
 			],
 		];
 	}