r96188 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r96187‎ | r96188 | r96189 >
Date:14:36, 3 September 2011
Author:dantman
Status:ok (Comments)
Tags:
Comment:
Expand r96170's support for space separated attributes with support for boolean keys such as array( 'class' => array( 'selected' => true ) ) to match our array( 'checked' => false ) support.
As per discussion with Krinkle make sure that in array( 'foo', 'foo' => false, 'foo' ) the 'foo' key is authoritive.
Modified paths:
  • /trunk/phase3/includes/Html.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Html.php
——@@ -432,13 +432,31 @@
433433 // values. Implode/explode to get those into the main array as well.
434434 if ( is_array( $value ) ) {
435435 // If input wasn't an array, we can skip this step
436 - $value = implode( ' ', $value );
 436+
 437+ $newValue = array();
 438+ foreach ( $value as $k => $v ) {
 439+ if ( is_string( $v ) ) {
 440+ // String values should be normal `array( 'foo' )`
 441+ // Just append them
 442+ if ( !isset( $value[$v] ) ) {
 443+ // As a special case don't set 'foo' if a
 444+ // separate 'foo' => true/false exists in the array
 445+ // keys should be authoritive
 446+ $newValue[] = $v;
 447+ }
 448+ } elseif ( $v ) {
 449+ // If the value is truthy but not a string this is likely
 450+ // an array( 'foo' => true ), falsy values don't add strings
 451+ $newValue[] = $k;
 452+ }
 453+ }
 454+ $value = implode( ' ', $newValue );
437455 }
438456 $value = explode( ' ', $value );
439457
440458 // Normalize spacing by fixing up cases where people used
441459 // more than 1 space and/or a trailing/leading space
442 - $value = array_diff( $value, array( '', ' ') );
 460+ $value = array_diff( $value, array( '', ' ' ) );
443461
444462 // Remove duplicates and create the string
445463 $value = implode( ' ', array_unique( $value ) );

Follow-up revisions

RevisionCommit summaryAuthorDate
r96256Add documentation for r96170 and r96188.krinkle21:18, 4 September 2011
r100629Tests for r96188 features...hashar17:45, 24 October 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r96170Html.php: The "future"[1] is here. Add features for space-separated value att...krinkle03:55, 3 September 2011

Comments

#Comment by Krinkle (talk | contribs)   14:52, 3 September 2011

So, to repeat the example Dantman gave on IRC: 

$isSelected = someFunc();
Html::element( 'input', array(
  'selected' => $isSelected
) );
#Comment by Krinkle (talk | contribs)   14:57, 3 September 2011

So, to repeat the example Dantman gave on IRC: 

$isSelected = someFunc();
Html::element( 'input', array(
  'selected' => $isSelected
) );

Html::element( 'div', array(
  'id' => 'mw-foo-wrapper',
  'class' => array( 'foo', 'bar', 'baz' =>hasBar()
) );

And another great use case: 

public function getFooAttr(){
 return array(
  'class' => array( 'foo', 'bar', baz' )
 );
}

public function getFooAttr(){
 $attribs = parent::getFooAttr();
 $attribs['id'] = 'mw-my-foo';
 $attribs['class'][] = 'my-foo';
 $attribs['class']['foo'] = false;
 return $attribs;
}

being able to set to false like that makes code a lot more readable and easy. No need to do multiple array_search'es followed by unset to remove all occurences of value 'foo', just set foo to false and it will not be outputted.

We'll need unit tests for this functionality

#Comment by Krinkle (talk | contribs)   21:18, 4 September 2011

As mentioned by Simetrical at rev:96170, this needs an update to the function documentation.

#Comment by Hashar (talk | contribs)   14:46, 24 October 2011

The commit message is not very clear to me: 

array( 'foo', 'foo' => false, 'foo' ) the 'foo' key is authoritative

I am assuming the => false will override the key hence in this case there will be no 'foo'. Can you confirm this is the expected behavior?

#Comment by Krinkle (talk | contribs)   19:01, 24 October 2011

Per r100629 test cases, Yes that is the case.

If there is both a numeral array entry and an associative array entry, the latter will be the one used. If that one has a false-y value it is removed from the array.

This allows users to easily remove a class from the array from a hook without having to go array-index and find the key to unset it (not to mention cases where the to-be-removed key can be part of another string). 

array( 'class' => array( 
  'foo baz quux',
  'baz' => true,
  'quux' => false,
 ) );

# class="foo baz"
>
#Comment by Dantman (talk | contribs)   10:12, 25 October 2011

Dropping tags as hashar has added tests.

Status & tagging log