r64866 MediaWiki - Code Review archive

• Fix type validation on Int fields to correctly handle numbers larger than PHP_MAX_INT or with leading zeroes (eg phone numbers on 32 bit systems)

Phone numbers are not integeters.

+if ( $value !==  
+	&& ( !is_numeric( $value ) || round( $value ) != $value ) ) 

If you really do want is_numeric and (all the crap it accepts) here, please document it. As it is input like "0xff" is considered valid, which I think it should not.

From an academic point of view, maybe not. We're not building an academic thought experiment. What are int fields really going to be used for? If someone really wants to specify a block length in hex, or a maximum number of results as an exponential, is that actually a problem?

The fact that 0123 != 123 is a problem though...

Yes if that input is going into database integer field. Anyway integer is integer, if you change its meaning to anything that looks like a number, don't call it integer, but something else. In any case good documentation should be provided for non-obvious code like that if condition.

Marking as 1.16, because a r65025 requires it.

Somehow this revision causes all hidden fields to be prefixed with "wp". I can't find out where exactly though. I think this is unnecessarily breaking b/c, so I suggest reverting it.

Hmm...

Test:

$fields = array(
	'FieldByKey' => array(
		'type' => 'hidden',
		'default' => 'foo', ),
	'FieldByName' => array(
		'type' => 'hidden',
		'default' => 'foo',
		'name' => 'AnotherFieldByName' ), );
$form = new HTMLForm( $fields );
$form->addHiddenField( 'FunctionByKey', 'foo' );
$form->addHiddenField( 'FunctionByName', 'foo', array( 'name' => 'AnotherFunctionByName') );

Old behaviour: FunctionByKey FunctionByName FieldByKey FieldByName

New behaviour: FunctionByKey FunctionByName wpFieldByKey wpAnotherFieldByName

I agree that's undesirable; as is the inability to force names when adding fields using addHiddenField().

Rationalised in r65040.

New behaviour:

FunctionByKey 
AnotherFunctionByName 
FieldByKey 
AnotherFieldByName 

How does this affect r65029 and its MFT? I don't really understand what the issue was there.

HTML5 defines:

A string is a valid floating point number if it consists of:

1. Optionally, a U+002D HYPHEN-MINUS character (-).
2. A series of one or more characters in the range U+0030 DIGIT ZERO (0) to U+0039 DIGIT NINE (9).
3. Optionally:
   1. A single U+002E FULL STOP character (.).
   2. A series of one or more characters in the range U+0030 DIGIT ZERO (0) to U+0039 DIGIT NINE (9).
4. Optionally:
   1. Either a U+0065 LATIN SMALL LETTER E character (e) or a U+0045 LATIN CAPITAL LETTER E character (E).
   2. Optionally, a U+002D HYPHEN-MINUS character (-) or U+002B PLUS SIGN character (+).
   3. A series of one or more characters in the range U+0030 DIGIT ZERO (0) to U+0039 DIGIT NINE (9).

<http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#valid-floating-point-number>

We may as well harmonize with that, because that's what will be checked on the client side in HTML5-supporting UAs, if HTML5 is enabled. Doesn't seem like we need to allow 0xff or such. For telephone numbers, you should use <input type="tel">, not type="number".

. . . also, why not support required for everything in the parent class, instead of in particular subclasses? The HTML attribute applies to all input types except "hidden".

(late) Agreed. I'd much prefer that the support for required were put higher up the food chain.

Both done in r78246.

- $section = ; - if ( isset( $info['section'] ) ) - $section = $info['section']; + $section = isset( $info['section'] ) + ? $info['section'] + : ;

- $info['name'] = $fieldname; + $info['name'] = isset( $info['name'] ) + ? $info['name'] + : $fieldname;

Please don't. Ternaries suck, if blocks are so much clearer.

Make that:

-			$section = '';
-			if ( isset( $info['section'] ) )
-				$section = $info['section'];
+			$section = isset( $info['section'] ) 
+				? $info['section']
+				: '';
 
-			$info['name'] = $fieldname;
+			$info['name'] = isset( $info['name'] )
+				? $info['name']
+				: $fieldname;

I disagree, in this instance. The semantics of "this variable is either set to one thing or another depending on something" is more clearly conveyed by a ternary than if/else. And I'm not seeing anything in Coding Conventions to suggest otherwise.

To me it looks like it should be a class with default values. This isn't about either-or.

I also don't understand this:

-				$checkbox = Xml::check( $this->mName . '[]', in_array( $info, $value ),
+				$checkbox = Xml::check( $this->mName . '[]', $info === $value,

If it's a multi-select, surely you should be using in_array(), and the change here should have been to pass true as the final parameter (to insist on strict comparisons).

This is almost certainly a bug, marking fixme.

This was fixed way back in r65216.