r113888 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r113887‎ | r113888 | r113889 >
Date:01:52, 15 March 2012
Author:saper
Status:deferred (Comments)
Tags:1.19 
Comment:
Unbreak maintenance/deleteDefaultMessages.php for PostgreSQL

deleteDefaultMessages.php was failing during upgrade
from MediaWiki 1.7.3 with a databaser error.

A stub user:

$user = User::newFromName( 'MediaWiki default' );

has user ID 0, so that $user->isAnon() is true.

Unfortunately, ManualLogEntry::publish() from r96441
tries to insert $user->getName() ("MediaWiki default")
into rc_ip.

PostgreSQL won't allow this, because rc_ip is of
Postgres-specific CIDR type.

Traceback:

Checking existence of old default messages...
...deleting old default messages (this may take a long time!)...A database query syntax error has occurred.
The last attempted database query was:
"INSERT INTO "recentchanges" (rc_timestamp,rc_cur_time,rc_namespace,rc_title,rc_type,rc_minor,rc_user,rc_user_text,rc_comment,rc_this_oldid,rc_last_oldid,rc_bot,rc_moved_to_ns,rc_moved_to_title,rc_ip,rc_patrolled,rc_new,rc_old_len,rc_new_len,rc_deleted,rc_logid,rc_log_type,rc_log_action,rc_params,rc_id) VALUES ('2012-03-14 21:51:05 GMT','2012-03-14 21:51:05 GMT','8','1movedto2','3','0','0','MediaWiki default','No longer required','0','0',1,'0','','MediaWiki default','1','0',NULL,NULL,'0','1','delete','delete','a:0:{}','1')"
from within function "RecentChange::save".
MySQL returned error "1: ERROR: invalid input syntax for type cidr: "MediaWiki default"
LINE 1: ...ki default','No longer required','0','0',1,'0','','MediaWiki...
^"
Backtrace:
#0 /usr/home/saper/public_html/pg/w/includes/db/DatabasePostgres.php(332): DatabaseBase->reportQueryError('ERROR: invalid...', 1, 'INSERT INTO "re...', 'RecentChange::s...', '')
#1 /usr/home/saper/public_html/pg/w/includes/db/Database.php(904): DatabasePostgres->reportQueryError('ERROR: invalid...', 1, 'INSERT INTO "re...', 'RecentChange::s...', '')
#2 /usr/home/saper/public_html/pg/w/includes/db/DatabasePostgres.php(604): DatabaseBase->query('INSERT INTO "re...', 'RecentChange::s...', '')
#3 /usr/home/saper/public_html/pg/w/includes/RecentChange.php(199): DatabasePostgres->insert('recentchanges', Array, 'RecentChange::s...')
#4 /usr/home/saper/public_html/pg/w/includes/logging/LogEntry.php(479): RecentChange->save('pleasedontudp')
#5 /usr/home/saper/public_html/pg/w/includes/WikiPage.php(2042): ManualLogEntry->publish('1')
#6 /usr/home/saper/public_html/pg/w/includes/WikiPage.php(1937): WikiPage->doDeleteArticleReal('No longer requi...', false, 0, false, '', Object(User))
#7 /usr/home/saper/public_html/pg/w/maintenance/deleteDefaultMessages.php(73): WikiPage->doDeleteArticle('No longer requi...', false, 0, false, '', Object(User))
#8 /usr/home/saper/public_html/pg/w/maintenance/update.php(128): DeleteDefaultMessages->execute()
#9 /usr/home/saper/public_html/pg/w/maintenance/doMaintenance.php(105): UpdateMediaWiki->execute()
#10 /usr/home/saper/public_html/pg/w/maintenance/update.php(151): require_once('/usr/home/saper...')
#11 {main}
Modified paths:
  • /trunk/phase3/includes/RecentChange.php (modified) (history)
  • /trunk/phase3/includes/logging/LogEntry.php (modified) (history)
  • /trunk/phase3/maintenance/deleteDefaultMessages.php (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/deleteDefaultMessages.php
——@@ -74,7 +74,7 @@
7575 $dbw->commit( __METHOD__ );
7676 }
7777
78 - $this->output( 'done!', 'msg' );
 78+ $this->output( 'done!\n', 'msg' );
7979 }
8080 }
8181
Index: trunk/phase3/includes/RecentChange.php
——@@ -382,12 +382,6 @@
383383 */
384384 public static function notifyEdit( $timestamp, &$title, $minor, &$user, $comment, $oldId,
385385 $lastTimestamp, $bot, $ip='', $oldSize=0, $newSize=0, $newId=0, $patrol=0 ) {
386 - global $wgRequest;
387 - if( !$ip ) {
388 - $ip = $wgRequest->getIP();
389 - if( !$ip ) $ip = '';
390 - }
391 -
392386 $rc = new RecentChange;
393387 $rc->mAttribs = array(
394388 'rc_timestamp' => $timestamp,
——@@ -405,7 +399,7 @@
406400 'rc_bot' => $bot ? 1 : 0,
407401 'rc_moved_to_ns' => 0,
408402 'rc_moved_to_title' => '',
409 - 'rc_ip' => $ip,
 403+ 'rc_ip' => self::checkIPAddress( $ip ),
410404 'rc_patrolled' => intval($patrol),
411405 'rc_new' => 0, # obsolete
412406 'rc_old_len' => $oldSize,
——@@ -446,14 +440,6 @@
447441 */
448442 public static function notifyNew( $timestamp, &$title, $minor, &$user, $comment, $bot,
449443 $ip='', $size=0, $newId=0, $patrol=0 ) {
450 - global $wgRequest;
451 - if( !$ip ) {
452 - $ip = $wgRequest->getIP();
453 - if( !$ip ) {
454 - $ip = '';
455 - }
456 - }
457 -
458444 $rc = new RecentChange;
459445 $rc->mAttribs = array(
460446 'rc_timestamp' => $timestamp,
——@@ -471,7 +457,7 @@
472458 'rc_bot' => $bot ? 1 : 0,
473459 'rc_moved_to_ns' => 0,
474460 'rc_moved_to_title' => '',
475 - 'rc_ip' => $ip,
 461+ 'rc_ip' => self::checkIPAddress( $ip ),
476462 'rc_patrolled' => intval($patrol),
477463 'rc_new' => 1, # obsolete
478464 'rc_old_len' => 0,
——@@ -540,12 +526,6 @@
541527 public static function newLogEntry( $timestamp, &$title, &$user, $actionComment, $ip,
542528 $type, $action, $target, $logComment, $params, $newId=0, $actionCommentIRC='' ) {
543529 global $wgRequest;
544 - if( !$ip ) {
545 - $ip = $wgRequest->getIP();
546 - if( !$ip ) {
547 - $ip = '';
548 - }
549 - }
550530
551531 $rc = new RecentChange;
552532 $rc->mAttribs = array(
——@@ -564,7 +544,7 @@
565545 'rc_bot' => $user->isAllowed( 'bot' ) ? $wgRequest->getBool( 'bot', true ) : 0,
566546 'rc_moved_to_ns' => 0,
567547 'rc_moved_to_title' => '',
568 - 'rc_ip' => $ip,
 548+ 'rc_ip' => self::checkIPAddress( $ip ),
569549 'rc_patrolled' => 1,
570550 'rc_new' => 0, # obsolete
571551 'rc_old_len' => null,
——@@ -575,6 +555,8 @@
576556 'rc_log_action' => $action,
577557 'rc_params' => $params
578558 );
 559+ wfDebug(__METHOD__ . ": " . var_export($rc->mAttribs, TRUE) . "\n");
 560+
579561 $rc->mExtra = array(
580562 'prefixedDBkey' => $title->getPrefixedDBkey(),
581563 'lastTimestamp' => 0,
——@@ -769,4 +751,18 @@
770752 }
771753 return ChangesList::showCharacterDifference( $old, $new );
772754 }
 755+
 756+ public static function checkIPAddress( $ip ) {
 757+ global $wgRequest;
 758+ if ( $ip ) {
 759+ if ( !IP::isIPAddress( $ip ) ) {
 760+ throw new MWException( "Attempt to write \"" . $ip . "\" as an IP address into recent changes" );
 761+ }
 762+ } else {
 763+ $ip = $wgRequest->getIP();
 764+ if( !$ip )
 765+ $ip = '';
 766+ }
 767+ return $ip;
 768+ }
773769 }
Index: trunk/phase3/includes/logging/LogEntry.php
——@@ -458,12 +458,22 @@
459459
460460 $logpage = SpecialPage::getTitleFor( 'Log', $this->getType() );
461461 $user = $this->getPerformer();
 462+ $ip = "";
 463+ if ( $user->isAnon() ) {
 464+ /*
 465+ * "MediaWiki default" and friends may have
 466+ * no IP address in their name
 467+ */
 468+ if ( IP::isIPAddress( $user->getName() ) ) {
 469+ $ip = $user->getName();
 470+ }
 471+ }
462472 $rc = RecentChange::newLogEntry(
463473 $this->getTimestamp(),
464474 $logpage,
465475 $user,
466476 $formatter->getPlainActionText(),
467 - $user->isAnon() ? $user->getName() : '',
 477+ $ip,
468478 $this->getType(),
469479 $this->getSubtype(),
470480 $this->getTarget(),

Follow-up revisions

RevisionCommit summaryAuthorDate
r113889Followup-To: r113888 Remove extra wfDebug()saper02:00, 15 March 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r96441Committing my new logging classes for review. Will later commit changes that ...nikerabbit15:32, 7 September 2011

Comments

#Comment by Saper (talk | contribs)   01:54, 15 March 2012

Another step to get PostgresUpdater to let you upgrade from 1.7.3 till present.

#Comment by 😂 (talk | contribs)   12:59, 21 March 2012

Couple of minor nitpicks:

  • Is there a reason this new function has to be public static?
  • The output() call in deleteDefaultMessages() needs double quotes if you're adding \n
  • The new wfDebug() call:
    • Could use a little more information about what that information you're exporting is
    • TRUE->true and minor spacing issues there

As to the substance of the change--this looks ok to me but I'd like a second opinion.

#Comment by Saper (talk | contribs)   16:54, 21 March 2012

Thanks, this is useful. You are 100% right ;)

  • public could be private without any problem
  • deleteDefaultMessages() I noticed later, is already fixed in the final set of changes to the updater (currently waiting for git :-)
  • wfDebug() was my omission, removed in r113889

The real substance is in LogEntry.php

Of course, there are alternatives possible like introducing StubUser class that handles the "MediaWiki default" user specially once and for all...

#Comment by Krinkle (talk | contribs)   05:01, 5 July 2012

Reminder: Old revs in svn tagged for merging to 1.19. Please remove tag when done or no longer relevant.

Status & tagging log