r78231 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r78230‎ | r78231 | r78232 >
Date:22:54, 11 December 2010
Author:platonides
Status:ok (Comments)
Tags:
Comment:
Remove that ugly unset( $vars['_lsExists'] );
Shouldn't the second file_exists() also need a wfSuppressWarnings?
Follow up to r78118.
Modified paths:
  • /trunk/phase3/includes/installer/Installer.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/installer/Installer.php
——@@ -225,18 +225,17 @@
226226 $_lsExists = file_exists( "$IP/LocalSettings.php" );
227227 wfRestoreWarnings();
228228
229 - if( $_lsExists ) {
230 - require( "$IP/includes/DefaultSettings.php" );
231 - require( "$IP/LocalSettings.php" );
232 - if ( file_exists( "$IP/AdminSettings.php" ) ) {
233 - require( "$IP/AdminSettings.php" );
234 - }
235 - $vars = get_defined_vars();
236 - unset( $vars['_lsExists'] );
237 - return $vars;
238 - } else {
 229+ if( !$_lsExists ) {
239230 return false;
240231 }
 232+ unset($_lsExists);
 233+
 234+ require( "$IP/includes/DefaultSettings.php" );
 235+ require( "$IP/LocalSettings.php" );
 236+ if ( file_exists( "$IP/AdminSettings.php" ) ) {
 237+ require( "$IP/AdminSettings.php" );
 238+ }
 239+ return get_defined_vars();
241240 }
242241
243242 /**

Follow-up revisions

RevisionCommit summaryAuthorDate
r801901.17: MFT first batch of installer changes: r78043, r78231, r78259, r78300, r...catrope20:47, 13 January 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r78118* Made the web upgrade process more friendly. Instead of saying "access denie...tstarling08:24, 9 December 2010

Comments

#Comment by Tim Starling (talk | contribs)   23:32, 12 December 2010

Neither of the file_exists() calls need wfSuppressWarnings(), as far as I can tell.

Obviously file_exists() is not going to give a warning if the file does not exist, that's the whole point of it. I thought that maybe the wfSuppressWarnings() was there because the person who added it found that it it would give a warning if the directory was not readable, which implies that if it returned true the first time, it's not necessary for it to suppress errors the second time. But that's not the case, it doesn't give a warning in that situation.

In php_stat() in the PHP source, the only calls to error functions are in branches that are unreachable when it is called as file_exists(). And php_checkuid_ex(), which does safe_mode checks, is called with CHECKUID_NO_ERRORS, so it doesn't give warnings either.

Status & tagging log