r69527 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r69526‎ | r69527 | r69528 >
Date:03:16, 19 July 2010
Author:jeroendedauw
Status:resolved (Comments)
Tags:
Comment:
Clarified field and method visibility
Modified paths:
  • /trunk/phase3/includes/installer/CliInstaller.php (modified) (history)
  • /trunk/phase3/includes/installer/WebInstaller.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/installer/WebInstaller.php
——@@ -2,14 +2,14 @@
33
44 class WebInstaller extends Installer {
55 /** WebRequest object */
6 - var $request;
 6+ public $request;
77
88 /** Cached session array */
9 - var $session;
 9+ public $session;
1010
1111 /** Captured PHP error text. Temporary.
1212 */
13 - var $phpErrors;
 13+ public $phpErrors;
1414
1515 /**
1616 * The main sequence of page names. These will be displayed in turn.
——@@ -18,7 +18,7 @@
1919 * * Add a config-page-<name> message
2020 * * Add a WebInstaller_<name> class
2121 */
22 - var $pageSequence = array(
 22+ public $pageSequence = array(
2323 'Language',
2424 'Welcome',
2525 'DBConnect',
——@@ -31,9 +31,9 @@
3232 );
3333
3434 /**
35 - * Out of sequence pages, selectable by the user at any time
 35+ * Out of sequence pages, selectable by the user at any time.
3636 */
37 - var $otherPages = array(
 37+ public $otherPages = array(
3838 'Restart',
3939 'Readme',
4040 'ReleaseNotes',
——@@ -43,29 +43,29 @@
4444
4545 /**
4646 * Array of pages which have declared that they have been submitted, have validated
47 - * their input, and need no further processing
 47+ * their input, and need no further processing.
4848 */
49 - var $happyPages;
 49+ public $happyPages;
5050
5151 /**
5252 * List of "skipped" pages. These are pages that will automatically continue
5353 * to the next page on any GET request. To avoid breaking the "back" button,
5454 * they need to be skipped during a back operation.
5555 */
56 - var $skippedPages;
 56+ public $skippedPages;
5757
5858 /**
59 - * Flag indicating that session data may have been lost
 59+ * Flag indicating that session data may have been lost.
6060 */
61 - var $showSessionWarning = false;
 61+ public $showSessionWarning = false;
6262
63 - var $helpId = 0;
64 - var $tabIndex = 1;
 63+ public $helpId = 0;
 64+ public $tabIndex = 1;
6565
66 - var $currentPageName;
 66+ public $currentPageName;
6767
6868 /** Constructor */
69 - function __construct( $request ) {
 69+ public function __construct( $request ) {
7070 parent::__construct();
7171 $this->output = new WebInstallerOutput( $this );
7272 $this->request = $request;
——@@ -73,10 +73,12 @@
7474
7575 /**
7676 * Main entry point.
 77+ *
7778 * @param $session Array: initial session array
 79+ *
7880 * @return Array: new session array
7981 */
80 - function execute( $session ) {
 82+ public function execute( $session ) {
8183 $this->session = $session;
8284 if ( isset( $session['settings'] ) ) {
8385 $this->settings = $session['settings'] + $this->settings;
——@@ -210,7 +212,7 @@
211213 return $this->finish();
212214 }
213215
214 - function getLowestUnhappy() {
 216+ public function getLowestUnhappy() {
215217 if ( count( $this->happyPages ) == 0 ) {
216218 return 0;
217219 } else {
——@@ -221,7 +223,7 @@
222224 /**
223225 * Start the PHP session. This may be called before execute() to start the PHP session.
224226 */
225 - function startSession() {
 227+ public function startSession() {
226228 $sessPath = $this->getSessionSavePath();
227229 if( $sessPath != '' ) {
228230 if( strval( ini_get( 'open_basedir' ) ) != '' ) {
——@@ -271,7 +273,7 @@
272274 /**
273275 * Show an error message in a box. Parameters are like wfMsg().
274276 */
275 - function showError( $msg /*...*/ ) {
 277+ public function showError( $msg /*...*/ ) {
276278 $args = func_get_args();
277279 array_shift( $args );
278280 $args = array_map( 'htmlspecialchars', $args );
——@@ -280,9 +282,9 @@
281283 }
282284
283285 /**
284 - * Temporary error handler for session start debugging
 286+ * Temporary error handler for session start debugging.
285287 */
286 - function errorHandler( $errno, $errstr ) {
 288+ public function errorHandler( $errno, $errstr ) {
287289 $this->phpErrors[] = $errstr;
288290 }
289291
——@@ -298,9 +300,9 @@
299301 }
300302
301303 /**
302 - * Get a URL for submission back to the same script
 304+ * Get a URL for submission back to the same script.
303305 */
304 - function getUrl( $query = array() ) {
 306+ public function getUrl( $query = array() ) {
305307 $url = $this->request->getRequestURL();
306308 # Remove existing query
307309 $url = preg_replace( '/\?.*$/', '', $url );
——@@ -311,26 +313,26 @@
312314 }
313315
314316 /**
315 - * Get a WebInstallerPage from the main sequence, by ID
 317+ * Get a WebInstallerPage from the main sequence, by ID.
316318 */
317 - function getPageById( $id ) {
 319+ public function getPageById( $id ) {
318320 $pageName = $this->pageSequence[$id];
319321 $pageClass = 'WebInstaller_' . $pageName;
320322 return new $pageClass( $this );
321323 }
322324
323325 /**
324 - * Get a WebInstallerPage by name
 326+ * Get a WebInstallerPage by name.
325327 */
326 - function getPageByName( $pageName ) {
 328+ public function getPageByName( $pageName ) {
327329 $pageClass = 'WebInstaller_' . $pageName;
328330 return new $pageClass( $this );
329331 }
330332
331333 /**
332 - * Get a session variable
 334+ * Get a session variable.
333335 */
334 - function getSession( $name, $default = null ) {
 336+ public function getSession( $name, $default = null ) {
335337 if ( !isset( $this->session[$name] ) ) {
336338 return $default;
337339 } else {
——@@ -339,23 +341,23 @@
340342 }
341343
342344 /**
343 - * Set a session variable
 345+ * Set a session variable.
344346 */
345 - function setSession( $name, $value ) {
 347+ public function setSession( $name, $value ) {
346348 $this->session[$name] = $value;
347349 }
348350
349351 /**
350 - * Get the next tabindex attribute value
 352+ * Get the next tabindex attribute value.
351353 */
352 - function nextTabIndex() {
 354+ public function nextTabIndex() {
353355 return $this->tabIndex++;
354356 }
355357
356358 /**
357 - * Initializes language-related variables
 359+ * Initializes language-related variables.
358360 */
359 - function setupLanguage() {
 361+ public function setupLanguage() {
360362 global $wgLang, $wgContLang, $wgLanguageCode;
361363 if ( $this->getSession( 'test' ) === null && !$this->request->wasPosted() ) {
362364 $wgLanguageCode = $this->getAcceptLanguage();
——@@ -370,9 +372,9 @@
371373 }
372374
373375 /**
374 - * Retrieves MediaWiki language from Accept-Language HTTP header
 376+ * Retrieves MediaWiki language from Accept-Language HTTP header.
375377 */
376 - function getAcceptLanguage() {
 378+ public function getAcceptLanguage() {
377379 global $wgLanguageCode;
378380
379381 $mwLanguages = Language::getLanguageNames();
——@@ -396,9 +398,9 @@
397399 }
398400
399401 /**
400 - * Called by execute() before page output starts, to show a page list
 402+ * Called by execute() before page output starts, to show a page list.
401403 */
402 - function startPageWrapper( $currentPageName ) {
 404+ public function startPageWrapper( $currentPageName ) {
403405 $s = "<div class=\"config-page-wrapper\">\n" .
404406 "<div class=\"config-page-list\"><ul>\n";
405407 $lastHappy = -1;
——@@ -423,9 +425,9 @@
424426 }
425427
426428 /**
427 - * Get a list item for the page list
 429+ * Get a list item for the page list.
428430 */
429 - function getPageListItem( $pageName, $enabled, $currentPageName ) {
 431+ public function getPageListItem( $pageName, $enabled, $currentPageName ) {
430432 $s = "<li class=\"config-page-list-item\">";
431433 $name = wfMsg( 'config-page-' . strtolower( $pageName ) );
432434 if ( $enabled ) {
——@@ -461,9 +463,9 @@
462464 }
463465
464466 /**
465 - * Output some stuff after a page is finished
 467+ * Output some stuff after a page is finished.
466468 */
467 - function endPageWrapper() {
 469+ public function endPageWrapper() {
468470 $this->output->addHTMLNoFlush(
469471 "</div>\n" .
470472 "<br style=\"clear:both\"/>\n" .
——@@ -471,31 +473,31 @@
472474 }
473475
474476 /**
475 - * Get HTML for an error box with an icon
 477+ * Get HTML for an error box with an icon.
476478 *
477479 * @param $text String: wikitext, get this with wfMsgNoTrans()
478480 */
479 - function getErrorBox( $text ) {
 481+ public function getErrorBox( $text ) {
480482 return $this->getInfoBox( $text, 'critical-32.png', 'config-error-box' );
481483 }
482484
483485 /**
484 - * Get HTML for a warning box with an icon
 486+ * Get HTML for a warning box with an icon.
485487 *
486488 * @param $text String: wikitext, get this with wfMsgNoTrans()
487489 */
488 - function getWarningBox( $text ) {
 490+ public function getWarningBox( $text ) {
489491 return $this->getInfoBox( $text, 'warning-32.png', 'config-warning-box' );
490492 }
491493
492494 /**
493 - * Get HTML for an info box with an icon
 495+ * Get HTML for an info box with an icon.
494496 *
495497 * @param $text String: wikitext, get this with wfMsgNoTrans()
496498 * @param $icon String: icon name, file in skins/common/images
497499 * @param $class String: additional class name to add to the wrapper div
498500 */
499 - function getInfoBox( $text, $icon = 'info-32.png', $class = false ) {
 501+ public function getInfoBox( $text, $icon = 'info-32.png', $class = false ) {
500502 $s =
501503 "<div class=\"config-info $class\">\n" .
502504 "<div class=\"config-info-left\">\n" .
——@@ -518,7 +520,7 @@
519521 * Get small text indented help for a preceding form field.
520522 * Parameters like wfMsg().
521523 */
522 - function getHelpBox( $msg /*, ... */ ) {
 524+ public function getHelpBox( $msg /*, ... */ ) {
523525 $args = func_get_args();
524526 array_shift( $args );
525527 $args = array_map( 'htmlspecialchars', $args );
——@@ -543,19 +545,19 @@
544546 }
545547
546548 /**
547 - * Output a help box
 549+ * Output a help box.
548550 */
549 - function showHelpBox( $msg /*, ... */ ) {
 551+ public function showHelpBox( $msg /*, ... */ ) {
550552 $args = func_get_args();
551553 $html = call_user_func_array( array( $this, 'getHelpBox' ), $args );
552554 $this->output->addHTML( $html );
553555 }
554556
555557 /**
556 - * Show a short informational message
 558+ * Show a short informational message.
557559 * Output looks like a list.
558560 */
559 - function showMessage( $msg /*, ... */ ) {
 561+ public function showMessage( $msg /*, ... */ ) {
560562 $args = func_get_args();
561563 array_shift( $args );
562564 $html = '<div class="config-message">' .
——@@ -566,9 +568,9 @@
567569
568570 /**
569571 * Label a control by wrapping a config-input div around it and putting a
570 - * label before it
 572+ * label before it.
571573 */
572 - function label( $msg, $forId, $contents ) {
 574+ public function label( $msg, $forId, $contents ) {
573575 if ( strval( $msg ) == '' ) {
574576 $labelText = '&#160;';
575577 } else {
——@@ -588,7 +590,7 @@
589591 }
590592
591593 /**
592 - * Get a labelled text box to configure a variable
 594+ * Get a labelled text box to configure a variable.
593595 *
594596 * @param $params Array
595597 * Parameters are:
——@@ -598,7 +600,7 @@
599601 * controlName: The name for the input element (optional)
600602 * value: The current value of the variable (optional)
601603 */
602 - function getTextBox( $params ) {
 604+ public function getTextBox( $params ) {
603605 if ( !isset( $params['controlName'] ) ) {
604606 $params['controlName'] = 'config_' . $params['var'];
605607 }
——@@ -626,7 +628,7 @@
627629 }
628630
629631 /**
630 - * Get a labelled password box to configure a variable
 632+ * Get a labelled password box to configure a variable.
631633 *
632634 * Implements password hiding
633635 * @param $params Array
——@@ -637,7 +639,7 @@
638640 * controlName: The name for the input element (optional)
639641 * value: The current value of the variable (optional)
640642 */
641 - function getPasswordBox( $params ) {
 643+ public function getPasswordBox( $params ) {
642644 if ( !isset( $params['value'] ) ) {
643645 $params['value'] = $this->getVar( $params['var'] );
644646 }
——@@ -650,7 +652,7 @@
651653 }
652654
653655 /**
654 - * Get a labelled checkbox to configure a boolean variable
 656+ * Get a labelled checkbox to configure a boolean variable.
655657 *
656658 * @param $params Array
657659 * Parameters are:
——@@ -660,7 +662,7 @@
661663 * controlName: The name for the input element (optional)
662664 * value: The current value of the variable (optional)
663665 */
664 - function getCheckBox( $params ) {
 666+ public function getCheckBox( $params ) {
665667 if ( !isset( $params['controlName'] ) ) {
666668 $params['controlName'] = 'config_' . $params['var'];
667669 }
——@@ -693,7 +695,7 @@
694696 }
695697
696698 /**
697 - * Get a set of labelled radio buttons
 699+ * Get a set of labelled radio buttons.
698700 *
699701 * @param $params Array
700702 * Parameters are:
——@@ -706,7 +708,7 @@
707709 * controlName: The name for the input element (optional)
708710 * value: The current value of the variable (optional)
709711 */
710 - function getRadioSet( $params ) {
 712+ public function getRadioSet( $params ) {
711713 if ( !isset( $params['controlName'] ) ) {
712714 $params['controlName'] = 'config_' . $params['var'];
713715 }
——@@ -748,9 +750,9 @@
749751 }
750752
751753 /**
752 - * Output an error or warning box using a Status object
 754+ * Output an error or warning box using a Status object.
753755 */
754 - function showStatusBox( $status ) {
 756+ public function showStatusBox( $status ) {
755757 if( !$status->isGood() ) {
756758 $text = $status->getWikiText();
757759 if( $status->isOk() ) {
——@@ -762,7 +764,7 @@
763765 }
764766 }
765767
766 - function showStatusMessage( $status ) {
 768+ public function showStatusMessage( $status ) {
767769 $text = $status->getWikiText();
768770 $this->output->addWikiText(
769771 "<div class=\"config-message\">\n" .
——@@ -779,7 +781,7 @@
780782 * @param $varNames Array
781783 * @param $prefix String: the prefix added to variables to obtain form names
782784 */
783 - function setVarsFromRequest( $varNames, $prefix = 'config_' ) {
 785+ public function setVarsFromRequest( $varNames, $prefix = 'config_' ) {
784786 $newValues = array();
785787 foreach ( $varNames as $name ) {
786788 $value = trim( $this->request->getVal( $prefix . $name ) );
——@@ -799,25 +801,25 @@
800802 }
801803
802804 /**
803 - * Get the starting tags of a fieldset
 805+ * Get the starting tags of a fieldset.
804806 *
805807 * @param $legend String: message name
806808 */
807 - function getFieldsetStart( $legend ) {
 809+ public function getFieldsetStart( $legend ) {
808810 return "\n<fieldset><legend>" . wfMsgHtml( $legend ) . "</legend>\n";
809811 }
810812
811813 /**
812 - * Get the end tag of a fieldset
 814+ * Get the end tag of a fieldset.
813815 */
814 - function getFieldsetEnd() {
 816+ public function getFieldsetEnd() {
815817 return "</fieldset>\n";
816818 }
817819
818820 /**
819821 * Helper for Installer::docLink()
820822 */
821 - function getDocUrl( $page ) {
 823+ public function getDocUrl( $page ) {
822824 $url = "{$_SERVER['PHP_SELF']}?page=" . urlencode( $page );
823825 if ( in_array( $this->currentPageName, $this->pageSequence ) ) {
824826 $url .= '&lastPage=' . urlencode( $this->currentPageName );
——@@ -827,15 +829,15 @@
828830 }
829831
830832 abstract class WebInstallerPage {
831 - function __construct( $parent ) {
 833+ public function __construct( $parent ) {
832834 $this->parent = $parent;
833835 }
834836
835 - function addHTML( $html ) {
 837+ public function addHTML( $html ) {
836838 $this->parent->output->addHTML( $html );
837839 }
838840
839 - function startForm() {
 841+ public function startForm() {
840842 $this->addHTML(
841843 "<div class=\"config-section\">\n" .
842844 Xml::openElement(
——@@ -848,7 +850,7 @@
849851 );
850852 }
851853
852 - function endForm( $continue = 'continue' ) {
 854+ public function endForm( $continue = 'continue' ) {
853855 $this->parent->output->outputWarnings();
854856 $s = "<div class=\"config-submit\">\n";
855857 $id = $this->getId();
——@@ -878,21 +880,21 @@
879881 $this->addHTML( $s );
880882 }
881883
882 - function getName() {
 884+ public function getName() {
883885 return str_replace( 'WebInstaller_', '', get_class( $this ) );
884886 }
885887
886 - function getId() {
 888+ public function getId() {
887889 return array_search( $this->getName(), $this->parent->pageSequence );
888890 }
889891
890 - abstract function execute();
 892+ public abstract function execute();
891893
892 - function getVar( $var ) {
 894+ public function getVar( $var ) {
893895 return $this->parent->getVar( $var );
894896 }
895897
896 - function setVar( $name, $value ) {
 898+ public function setVar( $name, $value ) {
897899 $this->parent->setVar( $name, $value );
898900 }
899901 }
——@@ -1755,4 +1757,4 @@
17561758 private static function replaceLeadingSpaces( $matches ) {
17571759 return "\n" . str_repeat( '&#160;', strlen( $matches[0] ) );
17581760 }
1759 -}
 1761+}
\ No newline at end of file
Index: trunk/phase3/includes/installer/CliInstaller.php
——@@ -24,9 +24,8 @@
2525 'dbpath' => 'wgSQLiteDataDir',
2626 );
2727
28 -
2928 /** Constructor */
30 - function __construct( $siteName, $admin = null, $option = array()) {
 29+ function __construct( $siteName, $admin = null, $option = array() ) {
3130 parent::__construct();
3231
3332 foreach ( $this->optionMap as $opt => $global ) {
——@@ -64,7 +63,7 @@
6564 /**
6665 * Main entry point.
6766 */
68 - public function execute( ) {
 67+ public function execute() {
6968 $this->performInstallation(
7069 array( $this, 'startStage'),
7170 array( $this, 'endStage' )
——@@ -91,13 +90,13 @@
9291 $this->showMessage( wfMsg( 'config-install-step-done' ) ."\n");
9392 }
9493
95 - function showMessage( $msg /*, ... */ ) {
 94+ public function showMessage( $msg /*, ... */ ) {
9695 echo html_entity_decode( strip_tags( $msg ), ENT_QUOTES );
9796 flush();
9897 }
9998
100 - function showStatusMessage( $status ) {
 99+ public function showStatusMessage( $status ) {
101100 $this->showMessage( $status->getWikiText() );
102101 }
103102
104 -}
 103+}
\ No newline at end of file

Comments

#Comment by 😂 (talk | contribs)   08:00, 20 October 2010

Some of these should probably be more restricted. I can't think of any reason for $pageSequence to be public, for example.

#Comment by Jeroen De Dauw (talk | contribs)   08:08, 20 October 2010

Sure, I figured as much when I made this change. However, it's hard to know for me as I didn't write this code, so please modify as you see fit.

What's with marking this fixme? The stuff was already publicly accessible, I didn't make any change there :)

#Comment by 😂 (talk | contribs)   08:15, 20 October 2010

I'll make a pass at fixing these.

And there wasn't a functional change, you're right, but you changed them to being declared a public interface whereas before they were just ambiguous. I don't like public variables especially. Its too easy for people to modify things they shouldn't :p You'll see I'm a big fan of protected, with appropriate getters and setters.

Status & tagging log