r104518 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r104517‎ | r104518 | r104519 >
Date:00:21, 29 November 2011
Author:preilly
Status:resolved (Comments)
Tags:
Comment:
remove unnecessary dom parsing
Modified paths:
  • /trunk/extensions/MobileFrontend/MobileFrontend.php (modified) (history)

Diff [purge]

Index: trunk/extensions/MobileFrontend/MobileFrontend.php
——@@ -129,6 +129,8 @@
130130 public static $hideSearchBox = false;
131131 public static $hideLogo = false;
132132 public static $languageUrls;
 133+ public static $wsLoginToken;
 134+ public static $wsLoginFormAction;
133135
134136 public static $messageKeys = array(
135137 'mobile-frontend-show-button',
——@@ -408,6 +410,16 @@
409411 if ( Title::newMainPage()->equals( self::$title ) ) {
410412 self::$isMainPage = true;
411413 }
 414+
 415+ if ( self::$title == 'Special:UserLogin' ) {
 416+ self::$wsLoginToken = $wgRequest->getSessionData( 'wsLoginToken' );
 417+ $returnToVal = $wgRequest->getVal( 'returnto' );
 418+ $returnto = ( !empty ( $returnToVal ) ) ? '&returnto=' . wfUrlencode( $returnToVal ) : '';
 419+ self::$wsLoginFormAction = self::$title->getLocalURL( 'action=submitlogin&type=login' . $returnto );
 420+ } else {
 421+ self::$wsLoginToken = '';
 422+ self::$wsLoginFormAction = '';
 423+ }
412424
413425 self::$htmlTitle = $out->getHTMLTitle();
414426
——@@ -1076,18 +1088,16 @@
10771089 }
10781090
10791091 /**
1080 - * @param $token string
1081 - * @param $action string
10821092 * @return DomElement
10831093 */
1084 - public function renderLogin( $token, $action ) {
 1094+ public function renderLogin() {
10851095 $username = self::$messages['mobile-frontend-username'];
10861096 $password = self::$messages['mobile-frontend-password'];
10871097 $login = self::$messages['mobile-frontend-login'];
10881098 $form = Html::openElement( 'form',
10891099 array( 'name' => 'userlogin',
10901100 'method' => 'post',
1091 - 'action' => $action ) ) .
 1101+ 'action' => self::$wsLoginFormAction ) ) .
10921102 Html::openElement( 'table',
10931103 array( 'class' => 'user-login' ) ) .
10941104 Html::openElement( 'tbody' ) .
——@@ -1138,7 +1148,7 @@
11391149 Html::closeElement( 'tr' ) .
11401150 Html::closeElement( 'tbody' ) .
11411151 Html::closeElement( 'table' ) .
1142 - Html::input( 'wpLoginToken', $token, 'hidden' ) .
 1152+ Html::input( 'wpLoginToken', self::$wsLoginToken, 'hidden' ) .
11431153 Html::closeElement( 'form' );
11441154 return $this->getDomDocumentNodeByTagName( $form, 'form' );
11451155 }
——@@ -1188,24 +1198,9 @@
11891199 if ( self::$title == 'Special:UserLogin' ) {
11901200 $userlogin = $this->doc->getElementById( 'userloginForm' );
11911201
1192 - if ( $userlogin && get_class($userlogin) === 'DOMElement' ) {
1193 - $inputs = $userlogin->getElementsByTagName( 'input' );
1194 - $forms = $userlogin->getElementsByTagName( 'form' );
1195 -
1196 - if ( $forms ) {
1197 - foreach ( $forms as $form ) {
1198 - $action = $form->getAttribute( 'action' );
1199 - }
1200 - }
1201 -
1202 - foreach ( $inputs as $input ) {
1203 - if ( $input->getAttribute( 'name' ) === 'wpLoginToken' ) {
1204 - $wpLoginToken = $input->getAttribute( 'value' );
1205 - }
1206 - }
1207 -
 1202+ if ( !empty( $userlogin ) && get_class($userlogin) === 'DOMElement' ) {
12081203 $firstHeading = $this->doc->getElementById( 'firstHeading' );
1209 - if ( $firstHeading ) {
 1204+ if ( !empty( $firstHeading ) ) {
12101205 $firstHeading->nodeValue = '';
12111206 }
12121207 }
——@@ -1294,8 +1289,8 @@
12951290 }
12961291
12971292 if ( self::$title == 'Special:UserLogin' ) {
1298 - if ( isset( $wpLoginToken ) && isset( $action ) && isset( $userlogin ) ) {
1299 - $login = $this->renderLogin( $wpLoginToken, $action );
 1293+ if ( !empty( $userlogin ) && get_class($userlogin) === 'DOMElement' ) {
 1294+ $login = $this->renderLogin();
13001295 $loginNode = $this->doc->importNode( $login, true );
13011296 $userlogin->appendChild( $loginNode );
13021297 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r105206fix use of __toString to check if is Userloginpreilly19:05, 5 December 2011
r105324getLocalUrl accepts also an array of parameters and does the escapingpreilly18:36, 6 December 2011
r105327remove use of emptypreilly18:43, 6 December 2011
r105328switch logicpreilly18:46, 6 December 2011
r105329remove use of empty from userlogin as it is always setpreilly18:48, 6 December 2011
r105332fix white spacepreilly18:52, 6 December 2011

Comments

#Comment by Nikerabbit (talk | contribs)   07:04, 2 December 2011 
+		if ( self::$title == 'Special:UserLogin' ) {

This just looks wrong.

Don't use empty if the variable is always set. getLocalUrl accepts also an array of parameters and does the escaping for you in that case.

#Comment by Preilly (talk | contribs)   18:54, 5 December 2011

Would you like it better if it was, " if ($pageTitle->isSpecial('Userlogin')"?

#Comment by Dantman (talk | contribs)   18:58, 5 December 2011
) Of course.
#Comment by Preilly (talk | contribs)   19:05, 5 December 2011

Okay, this should be fixed in r105206 now.

#Comment by 😂 (talk | contribs)   18:38, 6 December 2011 
$returnToVal = $wgRequest->getVal( 'returnto' );
$returnto = ( !empty ( $returnToVal ) ) ? '&returnto=' . wfUrlencode( $returnToVal ) : ;
self::$wsLoginFormAction = self::$title->getLocalURL( 'action=submitlogin&type=login' . $returnto );

No need for empty() here, getVal() will return null so you can just do 

$returnto = $returnToVal ? ...

Although like Niklas says above, you should probably just use the key=>value array for getLocalUrl()

Similar situation with $userlogin--you really don't need empty() here. Quoting the coding conventions:

"empty() is inverted conversion to boolean with error suppression. Only use it when you really want to suppress errors. Otherwise just use !. Do not use it to test if an array is empty, unless you simultaneously want to check if the variable is unset."
#Comment by Preilly (talk | contribs)   18:44, 6 December 2011

Empty was removed for that line in r105327.

Status & tagging log