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

diff --git a/includes/Html.php b/includes/Html.php
index 5262ffe..5f4655c 100644 (file)
--- 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 e934965..c561e70 100644 (file)
--- 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';
+       }
+}