r97544 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r97543‎ | r97544 | r97545 >
Date:20:00, 19 September 2011
Author:brion
Status:ok (Comments)
Tags:
Comment:
* (bug 31007) Fix regression in rebuildrecentchanges maintenance script

Was trying to save literal string 'NULL' into rc_old_len / rc_new_len fields due to a bug switching to database query builder methods in r77778.
Modified paths:
  • /trunk/phase3/maintenance/rebuildrecentchanges.php (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/rebuildrecentchanges.php
@@ -132,7 +132,6 @@
133133 } else {
134134 # Grab the entry's text size
135135 $size = $dbw->selectField( 'revision', 'rev_len', array( 'rev_id' => $obj->rc_this_oldid ) );
136 - $size = !is_null( $size ) ? intval( $size ) : 'NULL';
137136
138137 $dbw->update( 'recentchanges',
139138 array(

Follow-up revisions

RevisionCommit summaryAuthorDate
r97545MFT r97544: fix bug 31007 regression in r77778brion20:02, 19 September 2011
r97546MFT r97544: fix bug 31007 regression in r77778brion20:02, 19 September 2011
r107964re Bug 31007 - rebuildrecentchanges.php fails for Postgresql with strings for...mah23:42, 3 January 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r77778One less $wgTitle, and use dbw->update() instead of raw sqldemon05:59, 5 December 2010

Comments

#Comment by Aaron Schulz (talk | contribs)   20:09, 19 September 2011

So it sets the size to false now in that case?

#Comment by Aaron Schulz (talk | contribs)   20:11, 19 September 2011

Well if there is no rev row. I guess rev_len would be NULL if it just wasn't populated.

#Comment by Brion VIBBER (talk | contribs)   20:18, 19 September 2011

No, it sets it to null in that case -- instead of turning it into the string 'NULL', we now pass the null value through.

False could come up when the row doesn't exist; the previous code would have silently converted that to 0, which is probably wrong. New code will pass the false through, which I believe will end up as an empty string "" in the output SQL.

However, that's fairly unlikely as we've just populated 'recentchanges' with items pulled from 'revision'; and if revisions got deleted in the meantime their recentchanges records should have been removed too.

So I'm not sure if that's a case you'd need to handle... but checking for false and returning null wouldn't hurt?

#Comment by Aaron Schulz (talk | contribs)   21:07, 19 September 2011

Couldn't hurt :)

#Comment by Nikerabbit (talk | contribs)   06:09, 20 September 2011

So, how to fix the garbage that this script has added into the database?

#Comment by Nikerabbit (talk | contribs)   08:14, 20 September 2011

Actually, I'm not sure what it has added to the DB? Is 'NULL' in MySQL converted to 0 or null?

#Comment by Brion VIBBER (talk | contribs)   16:23, 20 September 2011

Looks like it turns 'NULL' into 0 inserting into an int column. (This probably throws a warning in strict mode, but otherwise coerces silently to 0.)

If you want to clean up, I guess just run rebuildrecentchanges.php again after the fix. :)

Status & tagging log