3713 lines changed, by my count, which makes it probably one of the biggest commits ever that weren't orchestrated primarily with sed (yes, you all know who to look at there). I glanced over like half of it before giving up. I found about four definite errors, so it was probably worth it, although it seemed to take an inordinately long time. Good luck to Brion on reviewing it properly. :)
On 10/1/07, aaron@svn.wikimedia.org aaron@svn.wikimedia.org wrote:
$this->mContent = $revision->userCan( Revision::DELETED_TEXT ) ? $revision->getRawText() : ""; //$this->mContent = $revision->getText();
$this->mContent = $revision->revText(); // Loads if user is allowed
Is there any reason to set it using $revision->getRawText() and then immediately afterwards overwrite it with $revision->revText()? You should delete the old line (and the commented-out one while you're at it).
$supress = "<tr><td> </td><td>";$supress .= Xml::checkLabel( wfMsg( 'revdelete-suppress' ), 'wpSuppress', 'wpSuppress', false, array( 'tabindex' => '2' ) );$supress .= "</td></tr>";} else {$supress = '';...
$supress
Should be $suppress, with two p's.
// Bitfields to further supress the content
Here too, although this is just a comment.
} else {$bitfield = 'rev_deleted';}
Uh, storing a string in a variable called $bitfield, which is later stored in a TINYINT?
return $f;
return "<tt>$f</tt>";
This should be in CSS, not inline styling.
} elseif( $rc_namespace == NS_SPECIAL ) {
} else if( $rc_namespace == NS_SPECIAL ) {
You know, "else if" is slower than "elseif". There's no reason to deliberately remove dedicated language constructs in favor of compound constructs just because that's how they do it in C.
$r .= ' ';
Surely there's a better way of doing whatever you're trying to accomplish.
* Generate HTML for the equivilant of a spacer image for tables
"equivalent"
* @access private
Use the private keyword for this. We shouldn't be using @access now that we're using PHP 5 and have an explicit keyword to that effect.
function spacerColumn() {return '<td width="12"></td>';}
This is the sort of thing CSS should be used for. Give the cells a class or id and some padding. You don't need extra columns that contain no content.
// Adds a few spacesfunction spacerIndent() {return ' ';}
There has *got* to be a better way of doing whatever you're trying to do.
+// For deleted images, gererally were all versions of the image are discarded $wgFileStore = array(); $wgFileStore['deleted']['directory'] = false;// Defaults to $wgUploadDirectory/deleted $wgFileStore['deleted']['url'] = null; // Private $wgFileStore['deleted']['hash'] = 3; // 3-level subdirectory split
The added comment doesn't make any sense to me. What's it supposed to mean?
+// To hide usernames +$wgGroupPermissions['oversight']['hideuser'] = true; +// To see hidden revs and unhide revs hidden from Sysops +$wgGroupPermissions['oversight']['hiderevision'] = true; +// For private log access +$wgGroupPermissions['oversight']['oversight'] = true;
Is the Oversight extension being merged into core?
$rdel = ''; $ldel = '';
$rdel = $ldel = ''; is more conventional, although that's nitpicking.
if ( $this->mOldRev && $this->mOldRev->isDeleted(Revision::DELETED_TEXT) ) {wfIncrStats( 'diff_uncacheable' );} else if ( $this->mNewRev && $this->mNewRev->isDeleted(Revision::DELETED_TEXT) ) {wfIncrStats( 'diff_uncacheable' );
There's no reason to have two different if clauses with the same effect. The conditions should be or'd instead.
$this->mOldPagetitle = htmlspecialchars( wfMsg( 'revisionasof', $t ) );
Could just use wfMsgHTML().
if( ($bitfield & $field) == $field ) {
A stylistic issue, but 'if( $bitfield & $field ) {' is simpler.
/*** As we use the same small set of messages in various methods and that* they are called often, we call them once and save them in $this->message*/
Is this actually necessary? Surely this is done on a lower level, by the implementation of the wfMsg() functions.
* @private
You should use the PHP keyword for this, again. At the very least, use the correct Doxygen markup. :)
function newRowFromID( $logid ) {$fname = 'LogReader::newFromTitle';
That seems slightly . . . misleading. How about you just use __METHOD__?
/*** As we use the same small set of messages in various methods and that* they are called often, we call them once and save them in $this->message*/
Same comment as before.
* @private*/function showhideLinks( $s, $title ) {
As above.
if( self::isDeleted($s,self::DELETED_ACTION) )return $revert;
Clearer to say return ''; here.
if( $this->flags & self::NO_ACTION_LINK ) {return $revert;}
And here. The two should be combined into a single conditional, and the initialization of $revert moved after them.
$revert = $this->skin->userToolLinks( 1, $s->log_title );
Surely you don't want the tool links to be hardcoded for user #1.
/*** As we use the same small set of messages in various methods and that* they are called often, we call them once and save them in $this->message*/
Again. At the very least this should be made a globally-accessible function of some kind, not cut-and-pasted for every class.
$wgOut->addHTML( "<h2 id=\"mergehistory\">" . wfMsgHtml( "mergehistory-list" ) . "</h2>\n" );
New id's should start with "mw-" to help avoid conflicts.
if( $this->mTimestamp && $this->mTimestamp >= $maxtimestamp ) {
Why are you checking $this->mTimestamp if immediately afterward you require it be greater than zero (assuming $maxtimestamp is non-negative)?
$wgOut->addHtml( wfMsg('mergehistory-fail') );
You probably want addWikiText() there.
$maxtimestamp = $maxtimestamp ? $maxtimestamp : 0;$this->maxTimestamp = $maxtimestamp;
How about: $this->maxTimestamp = intval( $maxtimestamp );