From 4aeb08ef1885a89009f44de4229ccbd63c7e4f1c Mon Sep 17 00:00:00 2001
From: =?utf8?q?Thiemo=20M=C3=A4ttig?= <thiemo.maettig@wikimedia.de>
Date: Sat, 10 May 2014 10:58:19 +0200
Subject: [PATCH] Html: Throw exception if array is used for an attribute not
 supporting it

Previously the behavior was more or less undefined for most attributes
except for the ones in $spaceSeparatedListAttributes (currently 'class',
'accesskey' and 'rel'). If an other attribute is set to an array (no
matter what it contains) the method produces broken HTML like
'key=' and only triggers a warning if error_reporting is enabled. If
error_reporting is not enabled a developer may overlook this.

To clarify: The method always *ALLOWS* array values. This is *NOT*
about unexpected types in the call signature but unexpected
combinations of nested values. These combinations are already
checked in the method but the check was incomplete.

I considered several solutions:
* Simply use the first array element. But we can't know if the first
  element is what the caller expected.
* Silently drop all arrays if the attribute doesn't allow lists. This
  is close to the current behavior of always returning 'key=' but is a
  breaking change for boolean attributes like 'checked' and 'selected'.
  Browsers accept the current 'checked=' as true while omiting the
  attribute means false.

Choosing to always throw an exception. As above, this is a
breaking change in some cases.

Change-Id: Id5fcbdef2696d0a81a91d54338939ee678475ca3
---
 includes/Html.php                   |  6 +++-
 tests/phpunit/includes/HtmlTest.php | 51 ++++++++++++++++++++++++++++-
 2 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/includes/Html.php b/includes/Html.php
index 5262ffe619..5f4655c28a 100644
--- a/includes/Html.php
+++ b/includes/Html.php
@@ -386,7 +386,7 @@ class Html {
 	 * For instance, it will omit quotation marks if $wgWellFormedXml is false,
 	 * and will treat boolean attributes specially.
 	 *
-	 * Attributes that should contain space-separated lists (such as 'class') array
+	 * Attributes that can contain space-separated lists ('class', 'accesskey' and 'rel') array
 	 * values are allowed as well, which will automagically be normalized
 	 * and converted to a space-separated string. In addition to a numerical
 	 * array, the attribute value may also be an associative array. See the
@@ -413,6 +413,8 @@ class Html {
 	 *   A value of false means to omit the attribute.  For boolean attributes,
 	 *   you can omit the key, e.g., array( 'checked' ) instead of
 	 *   array( 'checked' => 'checked' ) or such.
+	 *
+	 * @throws MWException if an attribute that doesn't allow lists is set to an array
 	 * @return string HTML fragment that goes between element name and '>'
 	 *   (starting with a space if at least one attribute is output)
 	 */
@@ -500,6 +502,8 @@ class Html {
 
 				// Remove duplicates and create the string
 				$value = implode( ' ', array_unique( $value ) );
+			} else if ( is_array( $value ) ) {
+				throw new MWException( "HTML attribute $key can not contain a list of values" );
 			}
 
 			// See the "Attributes" section in the HTML syntax part of HTML5,
diff --git a/tests/phpunit/includes/HtmlTest.php b/tests/phpunit/includes/HtmlTest.php
index e9349656be..c561e70fd7 100644
--- a/tests/phpunit/includes/HtmlTest.php
+++ b/tests/phpunit/includes/HtmlTest.php
@@ -112,7 +112,8 @@ class HtmlTest extends MediaWikiTestCase {
 			Html::expandAttributes( array( 'foo' => false ) ),
 			'skip keys with false value'
 		);
-		$this->assertNotEmpty(
+		$this->assertEquals(
+			' foo=""',
 			Html::expandAttributes( array( 'foo' => '' ) ),
 			'keep keys with an empty string'
 		);
@@ -153,6 +154,33 @@ class HtmlTest extends MediaWikiTestCase {
 		);
 	}
 
+	/**
+	 * @covers HTML::expandAttributes
+	 */
+	public function testExpandAttributesForNumbers() {
+		$this->assertEquals(
+			' value=1',
+			Html::expandAttributes( array( 'value' => 1 ) ),
+			'Integer value is cast to a string'
+		);
+		$this->assertEquals(
+			' value=1.1',
+			Html::expandAttributes( array( 'value' => 1.1 ) ),
+			'Float value is cast to a string'
+		);
+	}
+
+	/**
+	 * @covers HTML::expandAttributes
+	 */
+	public function testExpandAttributesForObjects() {
+		$this->assertEquals(
+			' value=stringValue',
+			Html::expandAttributes( array( 'value' => new HtmlTestValue() ) ),
+			'Object value is converted to a string'
+		);
+	}
+
 	/**
 	 * Test for Html::expandAttributes()
 	 * Please note it output a string prefixed with a space!
@@ -299,6 +327,21 @@ class HtmlTest extends MediaWikiTestCase {
 		);
 	}
 
+	/**
+	 * @covers Html::expandAttributes
+	 * @expectedException MWException
+	 */
+	public function testExpandAttributes_ArrayOnNonListValueAttribute_ThrowsException() {
+		// Real-life test case found in the Popups extension (see Gerrit cf0fd64),
+		// when used with an outdated BetaFeatures extension (see Gerrit deda1e7)
+		Html::expandAttributes( array(
+			'src' => array(
+				'ltr' => 'ltr.svg',
+				'rtl' => 'rtl.svg'
+			)
+		) );
+	}
+
 	/**
 	 * @covers Html::namespaceSelector
 	 */
@@ -665,3 +708,9 @@ class HtmlTest extends MediaWikiTestCase {
 		);
 	}
 }
+
+class HtmlTestValue {
+	function __toString() {
+		return 'stringValue';
+	}
+}
-- 
2.20.1