r57096 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r57095‎ | r57096 | r57097 >
Date:09:46, 30 September 2009
Author:happy-melon
Status:ok (Comments)
Tags:
Comment:
Recommit r56947, partially overwritten in r57024.
Modified paths:
  • /trunk/phase3/includes/HTMLForm.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/HTMLForm.php
@@ -852,11 +852,14 @@
853853 $attribs[$param] = '';
854854 }
855855 }
 856+ }
856857
857 - # Implement tiny differences between some field variants
858 - # here, rather than creating a new class for each one which
859 - # is essentially just a clone of this one.
860 - if ( isset( $this->mParams['type'] ) ) {
 858+ # Implement tiny differences between some field variants
 859+ # here, rather than creating a new class for each one which
 860+ # is essentially just a clone of this one.
 861+ if ( isset( $this->mParams['type'] ) ) {
 862+ # Options that apply only to HTML5
 863+ if( $wgHtml5 ){
861864 switch ( $this->mParams['type'] ) {
862865 case 'email':
863866 $attribs['type'] = 'email';
@@ -868,11 +871,14 @@
869872 $attribs['type'] = 'number';
870873 $attribs['step'] = 'any';
871874 break;
872 - case 'password':
873 - $attribs['type'] = 'password';
874 - break;
875875 }
876876 }
 877+ # Options that apply to HTML4 as well
 878+ switch( $this->mParams['type'] ){
 879+ case 'password':
 880+ $attribs['type'] = 'password';
 881+ break;
 882+ }
877883 }
878884
879885 return Html::element( 'input', $attribs );

Follow-up revisions

RevisionCommit summaryAuthorDate
r59234Avoid pointless $wgHtml5 checks...simetrical01:24, 19 November 2009

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r56947Fix bug in HTMLForm where password fields only got the type="password" attrib...happy-melon11:20, 26 September 2009
r57024Recommit some of the 'backend' stuff from the Login branch, after talking wit...happy-melon19:04, 28 September 2009

Comments

#Comment by Simetrical (talk | contribs)   23:03, 15 November 2009

This doesn't look right to me. Files other than Html.php (and maybe some related like Sanitizer.php) should generally not be aware of $wgHtml5 – the decision of what type to output for the form should be made by the Html methods. 'type' => 'email' and such should be passed unconditionally, and they'll get filtered out by Html::rawElement() if not in HTML5 mode. What bug was occurring here, exactly, and how does this fix it?

#Comment by Happy-melon (talk | contribs)   23:32, 15 November 2009

The type "password" is accepted in both HTML4 and HTML5, so should always be set for password fields. The old code only applied *any* attributes if $wgHtml5 was set, including password. So password fields were rendered in plaintext if the variable was false. I tend to agree with you that the filtering for these attributes should be at the Html.php stage, not HTMLForm. However, this is neither introducing nor addressing that issue, it's merely correcting an obvious bug: password fields should be password fields, whatever the value of $wgHtml5 :D. Does that clarify?

#Comment by Simetrical (talk | contribs)   23:55, 15 November 2009

Ah, I see. I was misreading the code, just looking at the diff rather than the whole file. The correct fix is to remove the entire if() block and get rid of all $wgHtml5 mention, AFAICT. I'll double-check that and commit it.

Status & tagging log