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 );
I've fixed most of these on my setup. I can't find the @ private stuff with grep.
The $bitfield = 'rev_deleted' is not an error though.
Simetrical-3 wrote:
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 ifuser 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 = '';...
$supressShould be $suppress, with two p's.
// Bitfields to further supress the contentHere 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 privateUse 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 methodsand 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.
* @privateYou 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 methodsand 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 methodsand 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 );
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org http://lists.wikimedia.org/mailman/listinfo/wikitech-l
On 10/1/07, Voice of All jschulz_4587@msn.com wrote:
The $bitfield = 'rev_deleted' is not an error though.
It's definitely a variable naming error if nothing else. If you go out of your way to call a variable $bitfield, it should *really* contain a bitfield.
And I don't understand how UPDATE archive SET ... ar_deleted='rev_deleted' ... is not an error when ar_deleted is a TINYINT.
All it does is take the value of rev_deleted and put that into ar_deleted.
Simetrical-3 wrote:
On 10/1/07, Voice of All jschulz_4587@msn.com wrote:
The $bitfield = 'rev_deleted' is not an error though.
It's definitely a variable naming error if nothing else. If you go out of your way to call a variable $bitfield, it should *really* contain a bitfield.
And I don't understand how UPDATE archive SET ... ar_deleted='rev_deleted' ... is not an error when ar_deleted is a TINYINT.
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org http://lists.wikimedia.org/mailman/listinfo/wikitech-l
On 10/1/07, Voice of All jschulz_4587@msn.com wrote:
All it does is take the value of rev_deleted and put that into ar_deleted.
0_o
On 10/1/07, Voice of All jschulz_4587@msn.com wrote:
All it does is take the value of rev_deleted and put that into ar_deleted.
Ah. That's insertSelect(), it seems, not update() or select() or anything else, and insertSelect() apparently uses an entirely different escaping convention from EVERY OTHER DATABASE FUNCTION for associative arrays. Which I suppose makes sense in context, but I have to say it makes Python's database input escaping format look more attractive (although that, admittedly, doesn't allow differences for database engines quite so easily).
I still question the wisdom of calling the variable $bitfield when it does not, in fact, necessarily contain a bitfield.
On 10/1/07, Simetrical Simetrical+wikilist@gmail.com wrote: [snip]
Which I suppose makes sense in context, but I have to say it makes Python's database input escaping format look more attractive (although that, admittedly, doesn't allow differences for database engines quite so easily).
Python's aren't perfect.
Some python database modules implement the ('%{dictkey}s',{'dictkey':...}) convention and some only implement the tuple convention. :(
Still better than the SQL injection bait that is the PHP database interface, but...
On 10/1/07, Gregory Maxwell gmaxwell@gmail.com wrote:
On 10/1/07, Simetrical Simetrical+wikilist@gmail.com wrote: [snip]
Which I suppose makes sense in context, but I have to say it makes Python's database input escaping format look more attractive (although that, admittedly, doesn't allow differences for database engines quite so easily).
Python's aren't perfect.
Some python database modules implement the ('%{dictkey}s',{'dictkey':...}) convention and some only implement the tuple convention. :(
It's comprehensible, is my point, to anyone who's familiar with sprintf(). It's also not as awkward as ours can be, although that's largely because it doesn't try to do as much (e.g., abstraction of joins and various options).
Simetrical wrote:
if( ($bitfield & $field) == $field ) {A stylistic issue, but 'if( $bitfield & $field ) {' is simpler.
It's not exactly the same. One could want to check if it's DELETED_FILE and DELETED_RESTRICTED with (DELETED_RESTRICTED | DELETED_FILE). But then $field wouldn't be one of DELETED_* constants...
On another coding conventions, why did member fields lose their initial m?
http://svn.wikimedia.org/viewvc/mediawiki/trunk/phase3/includes/filerepo/Arc...
On 10/2/07, Platonides Platonides@gmail.com wrote:
Simetrical wrote:
if( ($bitfield & $field) == $field ) {A stylistic issue, but 'if( $bitfield & $field ) {' is simpler.
It's not exactly the same. One could want to check if it's DELETED_FILE and DELETED_RESTRICTED with (DELETED_RESTRICTED | DELETED_FILE). But then $field wouldn't be one of DELETED_* constants...
Hmm, you're correct. I was assuming $field would be one bit. (13 & 3) == 1, for instance, not 3.
On another coding conventions, why did member fields lose their initial m?
http://svn.wikimedia.org/viewvc/mediawiki/trunk/phase3/includes/filerepo/Arc...
We were never especially rigorous in following that. I'm not sure there's too much point in it for a language that only allows member variable access prefixed with the object's name anyway, though. In C++ it might be a good idea so you don't get locals mixed up with members, but the whole $this-> requirement makes that kind of pointless for PHP. At least AFAICT.
On 02/10/2007, Simetrical Simetrical+wikilist@gmail.com wrote:
We were never especially rigorous in following that. I'm not sure there's too much point in it for a language that only allows member variable access prefixed with the object's name anyway, though. In C++ it might be a good idea so you don't get locals mixed up with members, but the whole $this-> requirement makes that kind of pointless for PHP. At least AFAICT.
I believe the rationale was that prefixing member variables with "m" would make it a bit easier to spot a missing "$this", which would cause PHP to initialise a brand new variable without warning...
...then again, I don't follow this "convention" either.
Rob Church
On 10/2/07, Rob Church robchur@gmail.com wrote:
I believe the rationale was that prefixing member variables with "m" would make it a bit easier to spot a missing "$this", which would cause PHP to initialise a brand new variable without warning...
I admit I made this mistake a few times when I first started writing in PHP, but it hardly seems serious. Hopefully the individual in question has notice reporting enabled anyway.
wikitech-l@lists.wikimedia.org