r90660 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r90659‎ | r90660 | r90661 >
Date:14:04, 23 June 2011
Author:salvatoreingala
Status:deferred
Tags:
Comment:
- Minor fixes in Gadget::isPrefsDescriptionValid
- Added unit tests for Gadget::isPrefsDescriptionValid
Modified paths:
  • /branches/salvatoreingala/Gadgets/Gadgets_tests.php (modified) (history)
  • /branches/salvatoreingala/Gadgets/backend/Gadget.php (modified) (history)

Diff [purge]

Index: branches/salvatoreingala/Gadgets/Gadgets_tests.php
——@@ -44,7 +44,7 @@
4545 $this->assertEquals( array( 'jquery.ui' ), $g->getDependencies() );
4646 }
4747
48 - function testPreferences() {
 48+ function testOptions() {
4949 global $wgUser;
5050
5151 // This test makes call to the parser which requires valids Outputpage
——@@ -79,4 +79,204 @@
8080 $wgOut = $old_wgOut;
8181 $wgTitle = $old_wgTitle;
8282 }
 83+
 84+ //Test preferences descriptions validator (generic)
 85+ function testPrefsDescriptions() {
 86+ $this->assertFalse( Gadget::isPrefsDescriptionValid( null ) );
 87+ $this->assertFalse( Gadget::isPrefsDescriptionValid( array() ) );
 88+ $this->assertFalse( Gadget::isPrefsDescriptionValid( array( 'fields' => array() ) ) );
 89+
 90+ //Test with wrong type
 91+ $this->assertFalse( Gadget::isPrefsDescriptionValid( array(
 92+ 'fields' => array(
 93+ 'testUnexisting' => array(
 94+ 'type' => 'unexistingtype',
 95+ 'label' => 'foo',
 96+ 'default' => 'bar'
 97+ )
 98+ )
 99+ ) ) );
 100+
 101+ //Test with wrong preference name
 102+ $this->assertFalse( Gadget::isPrefsDescriptionValid( array(
 103+ 'fields' => array(
 104+ 'testWrongN@me' => array(
 105+ 'type' => 'boolean',
 106+ 'label' => 'foo',
 107+ 'default' => true
 108+ )
 109+ )
 110+ ) ) );
 111+
 112+ //Test with an unexisting field parameter
 113+ $this->assertFalse( Gadget::isPrefsDescriptionValid( array(
 114+ 'fields' => array(
 115+ 'testBoolean' => array(
 116+ 'type' => 'boolean',
 117+ 'label' => 'foo',
 118+ 'default' => true,
 119+ 'unexistingParamThatMustFail' => 'foo'
 120+ )
 121+ )
 122+ ) ) );
 123+ }
 124+
 125+ //Tests for 'boolean' type preferences
 126+ function testPrefsDescriptionsBoolean() {
 127+ $correct = array(
 128+ 'fields' => array(
 129+ 'testBoolean' => array(
 130+ 'type' => 'boolean',
 131+ 'label' => 'some label',
 132+ 'default' => true
 133+ )
 134+ )
 135+ );
 136+
 137+ $this->assertTrue( Gadget::isPrefsDescriptionValid( $correct ) );
 138+
 139+ $correct2 = array(
 140+ 'fields' => array(
 141+ 'testBoolean' => array(
 142+ 'type' => 'boolean',
 143+ 'label' => 'some label',
 144+ 'default' => false
 145+ )
 146+ )
 147+ );
 148+
 149+ $this->assertTrue( Gadget::isPrefsDescriptionValid( $correct2 ) );
 150+
 151+ //Tests with wrong default values
 152+ $wrong = $correct;
 153+ foreach ( array( 0, 1, '', 'false', 'true', null, array() ) as $def ) {
 154+ $wrong['fields']['testBoolean']['default'] = $def;
 155+ $this->assertFalse( Gadget::isPrefsDescriptionValid( $wrong ) );
 156+ }
 157+ }
 158+
 159+ //Tests for 'string' type preferences
 160+ function testPrefsDescriptionsString() {
 161+ $correct = array(
 162+ 'fields' => array(
 163+ 'testString' => array(
 164+ 'type' => 'string',
 165+ 'label' => 'some label',
 166+ 'minlength' => 6,
 167+ 'maxlength' => 10,
 168+ 'required' => false,
 169+ 'default' => 'default'
 170+ )
 171+ )
 172+ );
 173+
 174+ $this->assertTrue( Gadget::isPrefsDescriptionValid( $correct ) );
 175+
 176+ //Tests with wrong default values
 177+ $wrong = $correct;
 178+ foreach ( array( null, true, false, 0, 1, array(), 'short', 'veryverylongstring' ) as $def ) {
 179+ $wrong['fields']['testString']['default'] = $def;
 180+ $this->assertFalse( Gadget::isPrefsDescriptionValid( $wrong ) );
 181+ }
 182+
 183+ //Tests with correct default values (when required is false)
 184+ $correct2 = $correct;
 185+ foreach ( array( '', '6chars', '1234567890' ) as $def ) {
 186+ $correct2['fields']['testString']['default'] = $def;
 187+ $this->assertTrue( Gadget::isPrefsDescriptionValid( $correct2 ) );
 188+ }
 189+
 190+ //Test with empty default when "required" is true
 191+ $wrong = $correct;
 192+ $wrong['fields']['testString']['required'] = true;
 193+ $wrong['fields']['testString']['default'] = '';
 194+ $this->assertFalse( Gadget::isPrefsDescriptionValid( $wrong ) );
 195+ }
 196+
 197+ //Tests for 'number' type preferences
 198+ function testPrefsDescriptionsNumber() {
 199+ $correctFloat = array(
 200+ 'fields' => array(
 201+ 'testNumber' => array(
 202+ 'type' => 'number',
 203+ 'label' => 'some label',
 204+ 'min' => -15,
 205+ 'max' => 36,
 206+ 'required' => true,
 207+ 'default' => 3.14
 208+ )
 209+ )
 210+ );
 211+
 212+ $correctInt = array(
 213+ 'fields' => array(
 214+ 'testNumber' => array(
 215+ 'type' => 'number',
 216+ 'label' => 'some label',
 217+ 'min' => -15,
 218+ 'max' => 36,
 219+ 'integer' => true,
 220+ 'required' => true,
 221+ 'default' => 12
 222+ )
 223+ )
 224+ );
 225+
 226+ $this->assertTrue( Gadget::isPrefsDescriptionValid( $correctFloat ) );
 227+ $this->assertTrue( Gadget::isPrefsDescriptionValid( $correctInt ) );
 228+
 229+ //Tests with wrong default values (with 'required' = true)
 230+ $wrongFloat = $correctFloat;
 231+ foreach ( array( '', false, true, null, array(), -100, +100 ) as $def ) {
 232+ $wrongFloat['fields']['testNumber']['default'] = $def;
 233+ $this->assertFalse( Gadget::isPrefsDescriptionValid( $wrongFloat ) );
 234+ }
 235+
 236+ $wrongInt = $correctInt;
 237+ foreach ( array( '', false, true, null, array(), -100, +100, 2.7182818 ) as $def ) {
 238+ $wrongInt['fields']['testNumber']['default'] = $def;
 239+ $this->assertFalse( Gadget::isPrefsDescriptionValid( $wrongInt ) );
 240+ }
 241+
 242+ //If required=false, default=null must be accepted, too
 243+ foreach ( array( $correctFloat, $correctInt ) as $correct ) {
 244+ $correct['fields']['testNumber']['required'] = false;
 245+ $correct['fields']['testNumber']['default'] = null;
 246+ $this->assertTrue( Gadget::isPrefsDescriptionValid( $correct ) );
 247+ }
 248+ }
 249+
 250+ //Tests for 'select' type preferences
 251+ function testPrefsDescriptionsSelect() {
 252+ $correct = array(
 253+ 'fields' => array(
 254+ 'testSelect' => array(
 255+ 'type' => 'select',
 256+ 'label' => 'some label',
 257+ 'default' => 3,
 258+ 'options' => array(
 259+ 'opt1' => null,
 260+ 'opt2' => true,
 261+ 'opt3' => 3,
 262+ 'opt4' => 'test'
 263+ )
 264+ )
 265+ )
 266+ );
 267+
 268+
 269+ //Tests with correct default values
 270+ $correct2 = $correct;
 271+ foreach ( array( null, true, 3, 'test' ) as $def ) {
 272+ $correct2['fields']['testSelect']['default'] = $def;
 273+ $this->assertTrue( Gadget::isPrefsDescriptionValid( $correct2 ) );
 274+ }
 275+
 276+ //Tests with wrong default values
 277+ $wrong = $correct;
 278+ foreach ( array( '', 'true', 'null', false, array(), 0, 1, 3.0001 ) as $def ) {
 279+ $wrong['fields']['testSelect']['default'] = $def;
 280+ $this->assertFalse( Gadget::isPrefsDescriptionValid( $wrong ) );
 281+ }
 282+ }
83283 }
Index: branches/salvatoreingala/Gadgets/backend/Gadget.php
——@@ -73,7 +73,7 @@
7474 'number' => array(
7575 'default' => array(
7676 'isMandatory' => true,
77 - 'checker' => 'Gadget::isFloatOrInt'
 77+ 'checker' => 'Gadget::isFloatOrIntOrNull'
7878 ),
7979 'label' => array(
8080 'isMandatory' => true,
——@@ -156,9 +156,24 @@
157157 private static function isFloatOrInt( $param ) {
158158 return is_float( $param ) || is_int( $param );
159159 }
 160+
 161+ private static function isFloatOrIntOrNull( $param ) {
 162+ return is_float( $param ) || is_int( $param ) || $param === null;
 163+ }
160164
161165 //Further checks for 'number' options
162166 private static function checkNumberOptionDefinition( $option ) {
 167+
 168+ if ( $option['default'] === null ) {
 169+ if ( isset( $option['required'] ) && $option['required'] === false ) {
 170+ //Accept null default if "required" is false
 171+ return true;
 172+ } else {
 173+ //Reject null default if "required" is true
 174+ return false;
 175+ }
 176+ }
 177+
163178 if ( isset( $option['integer'] ) && $option['integer'] === true ) {
164179 //Check if 'min', 'max' and 'default' are integers (if given)
165180 if ( intval( $option['default'] ) != $option['default'] ) {
——@@ -575,8 +590,11 @@
576591
577592 //Checks if the given description of the preferences is valid
578593 public static function isPrefsDescriptionValid( $prefsDescription ) {
579 -
580 - if ( $prefsDescription === null || !isset( $prefsDescription['fields'] ) ) {
 594+ if ( !is_array( $prefsDescription )
 595+ || !isset( $prefsDescription['fields'] )
 596+ || !is_array( $prefsDescription['fields'] )
 597+ || count( $prefsDescription['fields'] ) == 0 )
 598+ {
581599 return false;
582600 }
583601
——@@ -606,7 +624,10 @@
607625 return false;
608626 }
609627
610 - //TODO: check $option name compliance
 628+ //check $option name compliance
 629+ if ( !preg_match( '/^[a-zA-Z_][a-zA-Z0-9_]*$/', $option ) ) {
 630+ return false;
 631+ }
611632
612633 //Check if all fields satisfy specification
613634 $typeSpec = self::$prefsDescriptionSpecifications[$type];

Status & tagging log