r44384 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r44383‎ | r44384 | r44385 >
Date:05:29, 10 December 2008
Author:werdna
Status:resolved (Comments)
Tags:
Comment:
Fix up unnecessary display of some settings as 'changed' when they haven't been changed at all.
There were quite a few issues causing this:
* Trimming of settings.
* Some radio buttons would 'jump' to another value when that value had an internal key of '0'.
* isset() returns FALSE for null (STAB STAB STAB). Replaced some with array_key_exists();
* Some settings didn't have their empty values set. Added a check against the default to guard against this. If the default and the setting are empty, we use the default as the empty value.
* Lack of trimming in filterVar.
Modified paths:
  • /trunk/extensions/Configure/Configure.diff.php (modified) (history)
  • /trunk/extensions/Configure/Configure.obj.php (modified) (history)
  • /trunk/extensions/Configure/Configure.page.php (modified) (history)
  • /trunk/extensions/Configure/Configure.settings-core.php (modified) (history)
  • /trunk/extensions/Configure/SpecialConfigure.php (modified) (history)

Diff [purge]

Index: trunk/extensions/Configure/Configure.obj.php
——@@ -11,7 +11,7 @@
1212 protected $mWiki; // Wiki name
1313 protected $mConf = array(); // Our array of settings
1414 protected $mOldSettings = null; // Old settings (before applying our overrides)
15 - protected $mDefaults = array(); // Default values
 15+ protected $mDefaults = null; // Default values
1616
1717 /**
1818 * Construct a new object.
——@@ -84,14 +84,17 @@
8585 // Include files before so that customized settings won't be overridden
8686 // by the default ones
8787 $this->includeFiles();
88 -
89 - // Snapshot current settings before overriding.
90 - // -ialex tells me this weird contraption will work
91 - $allSettings = array_keys( ConfigurationSettings::singleton( CONF_SETTINGS_BOTH )->getAllSettings() );
92 -
93 - foreach( $allSettings as $setting ) {
94 - if ( isset( $GLOBALS[$setting] ) )
95 - $this->mDefaults[$setting] = $GLOBALS[$setting];
 88+
 89+ if (!is_array($this->mDefaults)) {
 90+ $this->mDefaults = array();
 91+ // Snapshot current settings before overriding.
 92+ // -ialex tells me this weird contraption will work
 93+ $allSettings = array_keys( ConfigurationSettings::singleton( CONF_SETTINGS_BOTH )->getAllSettings() );
 94+
 95+ foreach( $allSettings as $setting ) {
 96+ if ( array_key_exists( $setting, $GLOBALS ) )
 97+ $this->mDefaults[$setting] = $GLOBALS[$setting];
 98+ }
9699 }
97100
98101 list( $site, $lang ) = $this->siteFromDB( $this->mWiki );
——@@ -164,11 +167,15 @@
165168
166169 /** Recursive doohicky for normalising variables so we can compare them. */
167170 public static function filterVar( $var ) {
 171+ if (empty($var) && !$var) {
 172+ return null;
 173+ }
 174+
168175 if ( is_array( $var ) ) {
169176 return array_filter( array_map( array( __CLASS__, 'filterVar' ), $var ) );
170177 }
171178
172 - return $var;
 179+ return trim($var);
173180 }
174181
175182 /**
——@@ -227,7 +234,7 @@
228235 foreach ( $keys as $setting ) {
229236 if ( isset( $wikiDefaults[$setting] ) && !is_null( $wikiDefaults[$setting] ) )
230237 $ret[$setting] = $wikiDefaults[$setting];
231 - elseif ( isset( $globalDefaults[$setting] ) )
 238+ elseif ( array_key_exists( $setting, $globalDefaults ) )
232239 $ret[$setting] = $globalDefaults[$setting];
233240 }
234241 return $ret;
Index: trunk/extensions/Configure/Configure.page.php
——@@ -477,7 +477,7 @@
478478 $arrType = $this->getArrayType( $name );
479479 switch( $arrType ) {
480480 case 'simple':
481 - $text = trim($wgRequest->getText( 'wp' . $name ));
 481+ $text = rtrim($wgRequest->getText( 'wp' . $name ));
482482 if ( $text == '' )
483483 $arr = array();
484484 else
——@@ -524,7 +524,7 @@
525525 global $wgContLang;
526526 $arr = array();
527527 foreach ( $wgContLang->getNamespaces() as $ns => $unused ) {
528 - $arr[$ns] = trim( $wgRequest->getVal( 'wp' . $name . '-ns' . strval( $ns ) ) );
 528+ $arr[$ns] = $wgRequest->getVal( 'wp' . $name . '-ns' . strval( $ns ) );
529529 }
530530 $settings[$name] = $arr;
531531 break;
——@@ -543,7 +543,7 @@
544544 foreach ( $wgContLang->getNamespaces() as $ns => $unused ) {
545545 if ( $ns < 0 )
546546 continue;
547 - $text = trim( $wgRequest->getText( 'wp' . $name . '-ns' . strval( $ns ) ) );
 547+ $text = rtrim($wgRequest->getText( 'wp' . $name . '-ns' . strval( $ns ) ) );
548548 if ( $text == '' )
549549 $nsProtection = array();
550550 else
——@@ -617,7 +617,7 @@
618618 case 'text':
619619 case 'lang':
620620 case 'image-url':
621 - $setting = trim( $wgRequest->getVal( 'wp' . $name ) );
 621+ $setting = $wgRequest->getVal( 'wp' . $name );
622622
623623 if ( $file = wfFindFile( $setting ) ) {
624624 ## It's actually a local file.
——@@ -646,12 +646,13 @@
647647 }
648648 }
649649
650 - if ( isset( $settings[$name] ) ) {
 650+ if ( array_key_exists( $name, $settings ) ) {
651651 $settings[$name] = $this->cleanupSetting( $name, $settings[$name] );
652652 if ( $settings[$name] === null )
653653 unset( $settings[$name] );
654654 }
655655 }
 656+
656657 return $settings;
657658 }
658659
——@@ -663,13 +664,27 @@
664665 * @return Mixed
665666 */
666667 protected function cleanupSetting( $name, $val ) {
 668+ global $wgConf;
 669+
 670+ if (!empty($val) || $val) {
 671+ return $val;
 672+ }
 673+
667674 static $list = null;
668675 if ( $list === null )
669676 $list = $this->mConfSettings->getEmptyValues();
670 - if ( isset( $list[$name] ) && empty( $val ) )
 677+
 678+ static $defaults = null;
 679+ if ($defaults === null)
 680+ $defaults = $wgConf->getDefaultsForWiki( $this->mWiki );
 681+
 682+ if ( array_key_exists( $name, $list ) ) {
671683 return $list[$name];
672 - else
673 - return $val;
 684+ } elseif ( !array_key_exists( $name, $defaults ) ) {
 685+ return null;
 686+ } elseif ( empty( $defaults[$name] ) ) {
 687+ return $defaults[$name];
 688+ }
674689 }
675690
676691 /**
——@@ -928,7 +943,7 @@
929944 return '<code>' . htmlspecialchars( $default ) . '</code>';
930945 $ret = "\n";
931946 foreach ( $type as $val => $name ) {
932 - $ret .= Xml::radioLabel( $name, 'wp' . $conf, $val, 'wp' . $conf . $val, $default == $val ) . "\n";
 947+ $ret .= Xml::radioLabel( $name, 'wp' . $conf, $val, 'wp' . $conf . $val, $default === $val ) . "\n";
933948 }
934949 return $ret;
935950 }
——@@ -1347,9 +1362,11 @@
13481363 'value' => $this->getSettingValue( $setting ),
13491364 'link' => $showlink,
13501365 );
 1366+
 1367+ $customised = !array_key_exists( $setting, $defaults );
 1368+ $customised = $customised || ( WebConfiguration::filterVar( $defaults[$setting] ) != WebConfiguration::filterVar( $params['value'] ) );
13511369
1352 - $params['customised'] = ( !isset( $defaults[$setting] ) ||
1353 - WebConfiguration::filterVar( $defaults[$setting] ) != WebConfiguration::filterVar( $params['value'] ) );
 1370+ $params['customised'] = $customised;
13541371
13551372 $show = $this->mCanEdit ?
13561373 ( isset( $params['edit'] ) ? $params['edit'] : $this->userCanEdit( $setting ) ) :
Index: trunk/extensions/Configure/Configure.diff.php
——@@ -145,8 +145,9 @@
146146 foreach ( $groupSettings as $setting => $type ) {
147147 $oldSetting = isset( $old[$setting] ) ? $old[$setting] : null;
148148 $newSetting = isset( $new[$setting] ) ? $new[$setting] : null;
149 - if ( $oldSetting === $newSetting || !$this->isSettingViewable( $setting ) )
 149+ if ( $oldSetting === $newSetting || !$this->isSettingViewable( $setting ) ) {
150150 continue;
 151+ }
151152 else
152153 $groupDiff .= $this->processDiffSetting( $setting, $oldSetting, $newSetting, $type ) . "\n";
153154 }
Index: trunk/extensions/Configure/Configure.settings-core.php
——@@ -853,12 +853,12 @@
854854 'wgLocalMessageCache' => null,
855855 'wgInterwikiCache' => false,
856856 'wgReadOnly' => null,
857 - 'wgRateLimitLog' => false,
 857+ 'wgRateLimitLog' => null,
858858 'wgSessionName' => false,
859859 'wgHTTPProxy' => false,
860860 'wgSharedThumbnailScriptPath' => false,
861861 'wgSharedUploadDBname' => false,
862 - 'wgMimeDetectorCommand' => false,
 862+ 'wgMimeDetectorCommand' => null,
863863 'wgCustomConvertCommand' => false,
864864 'wgThumbnailScriptPath' => false,
865865 'wgDjvuDump' => null,
——@@ -875,6 +875,18 @@
876876 'wgSearchForwardUrl' => null,
877877 'wgHTCPMulticastAddress' => false,
878878 'wgExternalDiffEngine' => false,
 879+ 'wgSearchType' => null,
 880+ 'wgLocaltimezone' => null,
 881+ 'wgLocalTZoffset' => null,
 882+ 'wgReadOnly' => null,
 883+ 'wgDjvuDump' => null,
 884+ 'wgAntivirus' => null,
 885+ 'wgImportTargetNamespace' => null,
 886+ 'wgCopyrightIcon' => null,
 887+ 'wgSearchForwardUrl' => null,
 888+ 'wgExemptFromUserRobotsControl' => null,
 889+ 'wgArticlePath' => false,
 890+
879891 );
880892
881893 /**
Index: trunk/extensions/Configure/SpecialConfigure.php
——@@ -19,10 +19,12 @@
2020 protected function doSubmit() {
2121 global $wgConf, $wgOut, $wgConfigureUpdateCacheEpoch, $wgUser, $wgRequest;
2222
 23+ // These two lines left in and commented-out until I figure out what they're for.
 24+ // They seem to break stuff :)
2325 $reason = $wgRequest->getText( 'wpReason' );
24 - $current = $wgConf->getCurrent( $this->mWiki );
 26+// $current = $wgConf->getCurrent( $this->mWiki );
2527 $settings = $this->importFromRequest();
26 - $settings += $current;
 28+// $settings += $current;
2729 $settings = $this->removeDefaults( $settings );
2830 if ( $wgConfigureUpdateCacheEpoch )
2931 $settings['wgCacheEpoch'] = max( $settings['wgCacheEpoch'], wfTimestampNow() );

Follow-up revisions

RevisionCommit summaryAuthorDate
r44390Fix up r44384. I now understand what those lines are and what they do.werdna08:26, 10 December 2008

Comments

#Comment by IAlex (talk | contribs)   07:36, 10 December 2008

The lines you commented out in SpecialConfigure.php are there to not loose extensions configuration when you edit Special:Configure.

#Comment by Werdna (talk | contribs)   07:39, 10 December 2008

Oh, I see. They seem to break stuff :). I guess we should filter for extension settings, rather than all settings.

#Comment by Werdna (talk | contribs)   08:26, 10 December 2008

Does r44390 fix it up?

#Comment by IAlex (talk | contribs)   11:20, 10 December 2008

Now I get this two errors:

  • Notice : Use of undefined constant CONF_SETTINGS_EXTENSIONS - assumed 'CONF_SETTINGS_EXTENSIONS' in extensions/Configure/SpecialConfigure.php (line 26)
  • Warning : Missing argument 1 for WebConfiguration::getCurrent(), called in extensions/Configure/SpecialConfigure.php on line 27 and defined dans /extensions/Configure/Configure.obj.php (line 139)
#Comment by IAlex (talk | contribs)   11:57, 10 December 2008

Should be fixed in r44391.

Status & tagging log