r79013 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r79012‎ | r79013 | r79014 >
Date:04:38, 26 December 2010
Author:bawolff
Status:ok (Comments)
Tags:
Comment:
(Bug 26425) Due to r71751, wfTimestamp stopped accepting '' to mean get
current timestamp. This caused Block::__construct to put all sort of
messages in the debug log about invalid timestamp. So change Block to use
timestamp of 0 instead of '' if unspecified.
Modified paths:
  • /trunk/phase3/includes/Block.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Block.php
——@@ -24,7 +24,7 @@
2525 const EB_RANGE_ONLY = 4;
2626
2727 function __construct( $address = '', $user = 0, $by = 0, $reason = '',
28 - $timestamp = '' , $auto = 0, $expiry = '', $anonOnly = 0, $createAccount = 0, $enableAutoblock = 0,
 28+ $timestamp = 0, $auto = 0, $expiry = '', $anonOnly = 0, $createAccount = 0, $enableAutoblock = 0,
2929 $hideName = 0, $blockEmail = 0, $allowUsertalk = 0, $byName = false )
3030 {
3131 $this->mId = 0;

Follow-up revisions

RevisionCommit summaryAuthorDate
r79126Add test to supplement Bug 26425 (followup to r79013)soxred9319:27, 28 December 2010
r79129MFT r78011 r78014 r78015 r78016 r78099 r78117 r78161 r78170 r78172 r78199 r78......platonides19:58, 28 December 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r71751Merge r71750 to trunk: let wfTimestamp() accept TS_RFC2822 as an input format...catrope21:02, 26 August 2010

Comments

#Comment by MaxSem (talk | contribs)   18:16, 26 December 2010

Tests?

#Comment by Bawolff (talk | contribs)   01:04, 27 December 2010

What type of tests should I be doing here? Should i add one to assert that wfTimestamp( TS_UNIX, 0 ) == time(). But that might fail if the test is ran on a second boundry so that wfTimestamp call happens a second before the time() call (or is the chance of that happening negligible?)

#Comment by X! (talk | contribs)   01:12, 27 December 2010

I think the test should be written so that if no timestamp is added to the Block constructor, it equals time().

#Comment by MaxSem (talk | contribs)   05:32, 27 December 2010

Yes, race conditions suck. But you can at least check safely that it does not produce dates from 1970. Ideally, you can perform checks with one minute tolerance to ensure that result will not be impacted by changed time() result.

#Comment by X! (talk | contribs)   19:27, 28 December 2010

Test added in r79126.

#Comment by Bawolff (talk | contribs)   03:14, 29 December 2010

Thanks.

Status & tagging log