r54240 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r54239‎ \| r54240 \| r54241 >
Date:	21:55, 2 August 2009
Author:	demon
Status:	ok (Comments)
Tags:
Comment:	Don't put \n on the end of every error() call, just do it in error() itself. Still have to use on output(), because people like "Something...done" stuff.
Modified paths:	/trunk/phase3/maintenance/Maintenance.php (modified) (history) /trunk/phase3/maintenance/addwiki.php (modified) (history) /trunk/phase3/maintenance/benchmarkPurge.php (modified) (history) /trunk/phase3/maintenance/changePassword.php (modified) (history) /trunk/phase3/maintenance/cleanupSpam.php (modified) (history) /trunk/phase3/maintenance/createAndPromote.php (modified) (history) /trunk/phase3/maintenance/deleteBatch.php (modified) (history) /trunk/phase3/maintenance/deleteOldRevisions.php (modified) (history) /trunk/phase3/maintenance/edit.php (modified) (history) /trunk/phase3/maintenance/fixTimestamps.php (modified) (history) /trunk/phase3/maintenance/getLagTimes.php (modified) (history) /trunk/phase3/maintenance/moveBatch.php (modified) (history) /trunk/phase3/maintenance/populateLogSearch.php (modified) (history) /trunk/phase3/maintenance/populateParentId.php (modified) (history) /trunk/phase3/maintenance/protect.php (modified) (history) /trunk/phase3/maintenance/reassignEdits.php (modified) (history) /trunk/phase3/maintenance/rebuildFileCache.php (modified) (history) /trunk/phase3/maintenance/rebuildtextindex.php (modified) (history) /trunk/phase3/maintenance/removeUnusedAccounts.php (modified) (history) /trunk/phase3/maintenance/renameDbPrefix.php (modified) (history) /trunk/phase3/maintenance/runJobs.php (modified) (history) /trunk/phase3/maintenance/sql.php (modified) (history) /trunk/phase3/maintenance/stats.php (modified) (history) /trunk/phase3/maintenance/updateRestrictions.php (modified) (history) /trunk/phase3/maintenance/updateSpecialPages.php (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/updateRestrictions.php
—	—	@@ -35,12 +35,12 @@
36	36	public function execute() {
37	37	$db = wfGetDB( DB_MASTER );
38	38	if( !$db->tableExists( 'page_restrictions' ) ) {
39		~~- $this->error( "page_restrictions table does not exist\n", true );~~
	39	+ $this->error( "page_restrictions table does not exist", true );
40	40	}
41	41
42	42	$start = $db->selectField( 'page', 'MIN(page_id)', false, __METHOD__ );
43	43	if( !$start ) {
44		~~- $this->error( "Nothing to do.\n", true );~~
	44	+ $this->error( "Nothing to do.", true );
45	45	}
46	46	$end = $db->selectField( 'page', 'MAX(page_id)', false, __METHOD__ );
47	47
Index: trunk/phase3/maintenance/deleteOldRevisions.php
—	—	@@ -34,7 +34,7 @@
35	35	public function execute() {
36	36	$this->output( "Delete old revisions\n\n" );
37	37	if( count( $this->mArgs ) < 1 ) {
38		~~- $this->error( "Must pass at least 1 page_id\n", true );~~
	38	+ $this->error( "Must pass at least 1 page_id", true );
39	39	}
40	40	$this->doDelete( $this->hasOption( 'delete' ), $this->mArgs );
41	41	}
Index: trunk/phase3/maintenance/protect.php
—	—	@@ -46,9 +46,9 @@
47	47	$wgUser = User::newFromName( $userName );
48	48	$restrictions = array( 'edit' => $protection, 'move' => $protection );
49	49
50		~~- $wgTitle = Title::newFromText( $args[0] );~~
	50	+ $wgTitle = Title::newFromText( $this->getArg() );
51	51	if ( !$wgTitle ) {
52		~~- $this->error( "Invalid title\n", true );~~
	52	+ $this->error( "Invalid title", true );
53	53	}
54	54
55	55	$wgArticle = new Article( $wgTitle );
Index: trunk/phase3/maintenance/removeUnusedAccounts.php
—	—	@@ -48,7 +48,7 @@
49	49	}
50	50	$touched = $this->getOption( 'ignore-touched', "1" );
51	51	if( !ctype_digit( $touched ) ) {
52		~~- $this->error( "Please put a valid positive integer on the --ignore-touched parameter.\n", true );~~
	52	+ $this->error( "Please put a valid positive integer on the --ignore-touched parameter.", true );
53	53	}
54	54	$touchedSeconds = 86400 * $touched;
55	55	while( $row = $dbr->fetchObject( $res ) ) {
Index: trunk/phase3/maintenance/edit.php
—	—	@@ -47,7 +47,7 @@
48	48
49	49	$wgUser = User::newFromName( $userName );
50	50	if ( !$wgUser ) {
51		~~- $this->error( "Invalid username\n", true );~~
	51	+ $this->error( "Invalid username", true );
52	52	}
53	53	if ( $wgUser->isAnon() ) {
54	54	$wgUser->addToDatabase();
—	—	@@ -55,7 +55,7 @@
56	56
57	57	$wgTitle = Title::newFromText( $this->getArg() );
58	58	if ( !$wgTitle ) {
59		~~- $this->error( "Invalid title\n", true );~~
	59	+ $this->error( "Invalid title", true );
60	60	}
61	61
62	62	$wgArticle = new Article( $wgTitle );
Index: trunk/phase3/maintenance/fixTimestamps.php
—	—	@@ -47,7 +47,7 @@
48	48	$row = $dbw->fetchObject( $res );
49	49
50	50	if ( is_null( $row->minrev ) ) {
51		~~- $this->error( "No revisions in search period.\n", true );~~
	51	+ $this->error( "No revisions in search period.", true );
52	52	}
53	53
54	54	$minRev = $row->minrev;
—	—	@@ -98,8 +98,7 @@
99	99	was incorrectly set forward, negative means the clock was incorrectly set back.
100	100
101	101	If the offset is right, then increase the search interval until there are enough
102		~~- good revisions to provide a majority reference.~~
103		~~- ", true );~~
	102	+ good revisions to provide a majority reference.", true );
104	103	} elseif ( $numBadRevs == 0 ) {
105	104	$this->output( "No bad revisions found.\n" );
106	105	exit(0);
Index: trunk/phase3/maintenance/reassignEdits.php
—	—	@@ -50,7 +50,7 @@
51	51	$this->output( "Run the script again without --report to update.\n" );
52	52	} else {
53	53	$ton = $to->getName();
54		~~- $this->error( "User '{$ton}' not found.\n" );~~
	54	+ $this->error( "User '{$ton}' not found." );
55	55	}
56	56	}
57	57	}
Index: trunk/phase3/maintenance/getLagTimes.php
—	—	@@ -31,7 +31,7 @@
32	32
33	33	if( $lb->getServerCount() == 1 ) {
34	34	$this->error( "This script dumps replication lag times, but you don't seem to have\n"
35		~~- . "a multi-host db server configuration.\n" );~~
	35	+ . "a multi-host db server configuration." );
36	36	} else {
37	37	$lags = $lb->getLagTimes();
38	38	foreach( $lags as $n => $lag ) {
Index: trunk/phase3/maintenance/populateParentId.php
—	—	@@ -34,7 +34,7 @@
35	35	public function execute() {
36	36	$db = wfGetDB( DB_MASTER );
37	37	if ( !$db->tableExists( 'revision' ) ) {
38		~~- $this->error( "revision table does not exist\n", true );~~
	38	+ $this->error( "revision table does not exist", true );
39	39	}
40	40	$this->output( "Populating rev_parent_id column\n" );
41	41	$start = $db->selectField( 'revision', 'MIN(rev_id)', false, __FUNCTION__ );
Index: trunk/phase3/maintenance/populateLogSearch.php
—	—	@@ -35,7 +35,7 @@
36	36	public function execute() {
37	37	$db = wfGetDB( DB_MASTER );
38	38	if ( !$db->tableExists( 'log_search' ) ) {
39		~~- $this->error( "log_search does not exist\n", true );~~
	39	+ $this->error( "log_search does not exist", true );
40	40	}
41	41	$start = $db->selectField( 'logging', 'MIN(log_id)', false, __FUNCTION__ );
42	42	if( !$start ) {
Index: trunk/phase3/maintenance/createAndPromote.php
—	—	@@ -41,9 +41,9 @@
42	42
43	43	$user = User::newFromName( $username );
44	44	if( !is_object( $user ) ) {
45		~~- $this->error( "invalid username.\n", true );~~
	45	+ $this->error( "invalid username.", true );
46	46	} elseif( 0 != $user->idForName() ) {
47		~~- $this->error( "account exists.\n", true );~~
	47	+ $this->error( "account exists.", true );
48	48	}
49	49
50	50	# Try to set the password
Index: trunk/phase3/maintenance/deleteBatch.php
—	—	@@ -59,7 +59,7 @@
60	60
61	61	# Setup
62	62	if( !$file ) {
63		~~- $this->error( "Unable to read file, exiting\n", true );~~
	63	+ $this->error( "Unable to read file, exiting", true );
64	64	}
65	65	$wgUser = User::newFromName( $user );
66	66	$dbw = wfGetDB( DB_MASTER );
Index: trunk/phase3/maintenance/runJobs.php
—	—	@@ -42,7 +42,7 @@
43	43	if ( $this->hasOption( 'procs' ) ) {
44	44	$procs = intval( $this->getOption('procs') );
45	45	if ( $procs < 1 \|\| $procs > 1000 ) {
46		~~- $this->error( "Invalid argument to --procs\n", true );~~
	46	+ $this->error( "Invalid argument to --procs", true );
47	47	}
48	48	$fc = new ForkController( $procs );
49	49	if ( $fc->start( $procs ) != 'child' ) {
Index: trunk/phase3/maintenance/changePassword.php
—	—	@@ -36,7 +36,7 @@
37	37	public function execute() {
38	38	$user = User::newFromName( $this->getOption('user') );
39	39	if( !$user->getId() ) {
40		~~- $this->error( "No such user: " . $this->getOption('user') . "\n", true );~~
	40	+ $this->error( "No such user: " . $this->getOption('user'), true );
41	41	}
42	42	try {
43	43	$user->setPassword( $this->getOption('password') );
Index: trunk/phase3/maintenance/addwiki.php
—	—	@@ -39,7 +39,7 @@
40	40	$dbName = $this->getArg(2);
41	41
42	42	if ( !isset( $wgLanguageNames[$lang] ) ) {
43		~~- $this->error( "Language $lang not found in \$wgLanguageNames\n", true );~~
	43	+ $this->error( "Language $lang not found in \$wgLanguageNames", true );
44	44	}
45	45	$name = $wgLanguageNames[$lang];
46	46
Index: trunk/phase3/maintenance/Maintenance.php
—	—	@@ -218,7 +218,7 @@
219	219	*/
220	220	protected function error( $err, $die = false ) {
221	221	$f = fopen( 'php://stderr', 'w' );
222		~~- fwrite( $f, $err );~~
	222	+ fwrite( $f, $err . "\n" );
223	223	fclose( $f );
224	224	if( $die ) die();
225	225	}
—	—	@@ -278,7 +278,7 @@
279	279	require_once( $classFile );
280	280	}
281	281	if( !class_exists( $maintClass ) ) {
282		~~- $this->error( "Cannot spawn child: $maintClass\n" );~~
	282	+ $this->error( "Cannot spawn child: $maintClass" );
283	283	}
284	284	}
285	285
—	—	@@ -295,12 +295,12 @@
296	296
297	297	# Abort if called from a web server
298	298	if ( isset( $_SERVER ) && array_key_exists( 'REQUEST_METHOD', $_SERVER ) ) {
299		~~- $this->error( "This script must be run from the command line\n", true );~~
	299	+ $this->error( "This script must be run from the command line", true );
300	300	}
301	301
302	302	# Make sure we can handle script parameters
303	303	if( !ini_get( 'register_argc_argv' ) ) {
304		~~- $this->error( "Cannot get command line arguments, register_argc_argv is set to false\n", true );~~
	304	+ $this->error( "Cannot get command line arguments, register_argc_argv is set to false", true );
305	305	}
306	306
307	307	if( version_compare( phpversion(), '5.2.4' ) >= 0 ) {
—	—	@@ -407,7 +407,7 @@
408	408	if ( isset( $this->mParams[$option] ) && $this->mParams[$option]['withArg'] ) {
409	409	$param = next( $argv );
410	410	if ( $param === false ) {
411		~~- $this->error( "$arg needs a value after it\n", true );~~
	411	+ $this->error( "$arg needs a value after it", true );
412	412	}
413	413	$options[$option] = $param;
414	414	} else {
—	—	@@ -427,7 +427,7 @@
428	428	if ( $this->mParams[$option]['withArg'] ) {
429	429	$param = next( $argv );
430	430	if ( $param === false ) {
431		~~- $this->error( "$arg needs a value after it\n", true );~~
	431	+ $this->error( "$arg needs a value after it", true );
432	432	}
433	433	$options[$option] = $param;
434	434	} else {
—	—	@@ -452,13 +452,13 @@
453	453	# Check to make sure we've got all the required ones
454	454	foreach( $this->mParams as $opt => $info ) {
455	455	if( $info['require'] && !$this->hasOption($opt) ) {
456		~~- $this->error( "Param $opt required.\n", true );~~
	456	+ $this->error( "Param $opt required.", true );
457	457	}
458	458	}
459	459
460	460	# Also make sure we've got enough arguments
461	461	if ( count( $this->mArgs ) < count( $this->mArgList ) ) {
462		~~- $this->error( "Not enough arguments passed\n", true );~~
	462	+ $this->error( "Not enough arguments passed", true );
463	463	}
464	464	}
465	465
—	—	@@ -633,7 +633,7 @@
634	634
635	635	if ( ! is_readable( $settingsFile ) ) {
636	636	$this->error( "A copy of your installation's LocalSettings.php\n" .
637		~~- "must exist and be readable in the source directory.\n", true );~~
	637	+ "must exist and be readable in the source directory.", true );
638	638	}
639	639	$wgCommandLineMode = true;
640	640	$DP = $IP;
Index: trunk/phase3/maintenance/rebuildtextindex.php
—	—	@@ -42,7 +42,7 @@
43	43	// Only do this for MySQL
44	44	$database = wfGetDB( DB_MASTER );
45	45	if( !$database instanceof DatabaseMysql ) {
46		~~- $this->error( "This script is only for MySQL.\n", true );~~
	46	+ $this->error( "This script is only for MySQL.", true );
47	47	}
48	48
49	49	$wgTitle = Title::newFromText( "Rebuild text index script" );
Index: trunk/phase3/maintenance/stats.php
—	—	@@ -33,13 +33,13 @@
34	34
35	35	// Can't do stats if
36	36	if( get_class( $wgMemc ) == 'FakeMemCachedClient' ) {
37		~~- $this->error( "You are running FakeMemCachedClient, I can not provide any statistics.\n", true );~~
	37	+ $this->error( "You are running FakeMemCachedClient, I can not provide any statistics.", true );
38	38	}
39	39	$session = intval($wgMemc->get(wfMemcKey('stats','request_with_session')));
40	40	$noSession = intval($wgMemc->get(wfMemcKey('stats','request_without_session')));
41	41	$total = $session + $noSession;
42	42	if ( $total == 0 ) {
43		~~- $this->error( "You either have no stats or the cache isn't running. Aborting.\n", true );~~
	43	+ $this->error( "You either have no stats or the cache isn't running. Aborting.", true );
44	44	}
45	45	$this->output( "Requests\n" );
46	46	$this->output( sprintf( "with session: %-10d %6.2f%%\n", $session, $session/$total*100 ) );
Index: trunk/phase3/maintenance/updateSpecialPages.php
—	—	@@ -38,7 +38,7 @@
39	39
40	40	foreach( $wgSpecialPageCacheUpdates as $special => $call ) {
41	41	if( !is_callable($call) ) {
42		~~- $this->error( "Uncallable function $call!\n" );~~
	42	+ $this->error( "Uncallable function $call!" );
43	43	continue;
44	44	}
45	45	$t1 = explode( ' ', microtime() );
—	—	@@ -113,7 +113,7 @@
114	114	if ( !wfGetLB()->pingAll()) {
115	115	$this->output( "\n" );
116	116	do {
117		~~- $this->error( "Connection failed, reconnecting in 10 seconds...\n" );~~
	117	+ $this->error( "Connection failed, reconnecting in 10 seconds..." );
118	118	sleep(10);
119	119	} while ( !wfGetLB()->pingAll() );
120	120	$this->output( "Reconnected\n\n" );
Index: trunk/phase3/maintenance/cleanupSpam.php
—	—	@@ -42,7 +42,7 @@
43	43	$spec = $this->getArg();
44	44	$like = LinkFilter::makeLike( $spec );
45	45	if ( !$like ) {
46		~~- $this->error( "Not a valid hostname specification: $spec\n", true );~~
	46	+ $this->error( "Not a valid hostname specification: $spec", true );
47	47	}
48	48
49	49	$dbr = wfGetDB( DB_SLAVE );
—	—	@@ -83,7 +83,7 @@
84	84	private function cleanupArticle( $id, $domain ) {
85	85	$title = Title::newFromID( $id );
86	86	if ( !$title ) {
87		~~- $this->error( "Internal error: no page for ID $id\n" );~~
	87	+ $this->error( "Internal error: no page for ID $id" );
88	88	return;
89	89	}
90	90
Index: trunk/phase3/maintenance/moveBatch.php
—	—	@@ -64,7 +64,7 @@
65	65
66	66	# Setup
67	67	if( !$file ) {
68		~~- $this->error( "Unable to read file, exiting\n", true );~~
	68	+ $this->error( "Unable to read file, exiting", true );
69	69	}
70	70	$wgUser = User::newFromName( $user );
71	71
—	—	@@ -77,13 +77,13 @@
78	78	}
79	79	$parts = array_map( 'trim', explode( '\|', $line ) );
80	80	if ( count( $parts ) != 2 ) {
81		~~- $this->error( "Error on line $linenum, no pipe character\n" );~~
	81	+ $this->error( "Error on line $linenum, no pipe character" );
82	82	continue;
83	83	}
84	84	$source = Title::newFromText( $parts[0] );
85	85	$dest = Title::newFromText( $parts[1] );
86	86	if ( is_null( $source ) \|\| is_null( $dest ) ) {
87		~~- $this->error( "Invalid title on line $linenum\n" );~~
	87	+ $this->error( "Invalid title on line $linenum" );
88	88	continue;
89	89	}
90	90
Index: trunk/phase3/maintenance/renameDbPrefix.php
—	—	@@ -49,7 +49,7 @@
50	50	}
51	51
52	52	if( $old === false \|\| $new === false ) {
53		~~- $this->error( "Invalid prefix!\n", true );~~
	53	+ $this->error( "Invalid prefix!", true );
54	54	}
55	55	if( $old === $new ) {
56	56	$this->output( "Same prefix. Nothing to rename!\n", true );
Index: trunk/phase3/maintenance/benchmarkPurge.php
—	—	@@ -33,7 +33,7 @@
34	34	public function execute() {
35	35	global $wgUseSquid;
36	36	if( !$wgUseSquid ) {
37		~~- $this->error( "Squid purge benchmark doesn't do much without squid support on.\n". true );~~
	37	+ $this->error( "Squid purge benchmark doesn't do much without squid support on.". true );
38	38	} else {
39	39	$this->output( "There are " . count( $wgSquidServers ) . " defined squid servers:\n" );
40	40	if( $this->hasOption( 'count' ) ) {
Index: trunk/phase3/maintenance/sql.php
—	—	@@ -41,7 +41,7 @@
42	42	}
43	43
44	44	if ( !$file )
45		~~- $this->error( "Unable to open input file\n", true );~~
	45	+ $this->error( "Unable to open input file", true );
46	46
47	47	$dbw = wfGetDB( DB_MASTER );
48	48	$error = $dbw->sourceStream( $file, $promptCallback, array( $this, 'sqlPrintResult' ) );
Index: trunk/phase3/maintenance/rebuildFileCache.php
—	—	@@ -33,7 +33,7 @@
34	34	public function execute() {
35	35	global $wgUseFileCache, $wgDisableCounters, $wgTitle, $wgArticle, $wgOut;
36	36	if( !$wgUseFileCache ) {
37		~~- $this->error( "Nothing to do -- \$wgUseFileCache is disabled.\n", true );~~
	37	+ $this->error( "Nothing to do -- \$wgUseFileCache is disabled.", true );
38	38	}
39	39	$wgDisableCounters = false;
40	40	$start = intval( $this->getArg( 0, 0 ) );
—	—	@@ -44,7 +44,7 @@
45	45	$start = $start > 0 ? $start : $dbr->selectField( 'page', 'MIN(page_id)', false, __FUNCTION__ );
46	46	$end = $dbr->selectField( 'page', 'MAX(page_id)', false, __FUNCTION__ );
47	47	if( !$start ) {
48		~~- $this->error( "Nothing to do.\n", true );~~
	48	+ $this->error( "Nothing to do.", true );
49	49	}
50	50
51	51	$_SERVER['HTTP_ACCEPT_ENCODING'] = 'bgzip'; // hack, no real client

Comments

#Comment by Nikerabbit (talk | contribs) 08:49, 3 August 2009

I like my "channel" based method, which allows continuation by giving the same channel id, while automatically adding newlines when channel changes. This way you can get error messages nicely on their own line. See the code.

Can someone fix the link parser?

Status & tagging log

18:25, 19 August 2009 Brion VIBBER (talk | contribs) changed the status of r54240 [removed: new added: ok]