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(a)svn.wikimedia.org <aaron(a)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 spaces
> + function 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 );