r77401 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r77400‎ | r77401 | r77402 >
Date:21:59, 28 November 2010
Author:platonides
Status:ok (Comments)
Tags:
Comment:
Fix bug discovered in r77171 from the user data in If-Modified-Since passed by resource loader.
Added in wfTimestamp reading support for the three http date formats.
Increased conformance reading rfc2822 dates (read support added in r71750/r71751). We may not want full compliance with rfc2822, though.
The only provider of rfc2822 dates is probably http and that uses a subset (it's rfc822 + 4-digit years from rfc1123).
Make wfTimestamp() return false in case of a wrong input, according to CR.
Modified paths:
  • /trunk/phase3/includes/GlobalFunctions.php (modified) (history)
  • /trunk/phase3/maintenance/tests/phpunit/includes/GlobalTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/tests/phpunit/includes/GlobalTest.php
——@@ -195,6 +195,51 @@
196196 '20010115T123456Z',
197197 wfTimestamp( TS_ISO_8601_BASIC, '2001-01-15 12:34:56' ),
198198 'TS_DB to TS_ISO_8601_BASIC' );
 199+
 200+ # rfc2822 section 3.3
 201+
 202+ $this->assertEquals(
 203+ 'Mon, 15 Jan 2001 12:34:56 GMT',
 204+ wfTimestamp( TS_RFC2822, '20010115123456' ),
 205+ 'TS_MW to TS_RFC2822' );
 206+
 207+ $this->assertEquals(
 208+ '20010115123456',
 209+ wfTimestamp( TS_MW, 'Mon, 15 Jan 2001 12:34:56 GMT' ),
 210+ 'TS_RFC2822 to TS_MW' );
 211+
 212+ $this->assertEquals(
 213+ '20010115123456',
 214+ wfTimestamp( TS_MW, ' Mon, 15 Jan 2001 12:34:56 GMT' ),
 215+ 'TS_RFC2822 with leading space to TS_MW' );
 216+
 217+ $this->assertEquals(
 218+ '20010115123456',
 219+ wfTimestamp( TS_MW, '15 Jan 2001 12:34:56 GMT' ),
 220+ 'TS_RFC2822 without optional day-of-week to TS_MW' );
 221+
 222+ # FWS = ([*WSP CRLF] 1*WSP) / obs-FWS ; Folding white space
 223+ # obs-FWS = 1*WSP *(CRLF 1*WSP) ; Section 4.2
 224+ $this->assertEquals(
 225+ '20010115123456',
 226+ wfTimestamp( TS_MW, 'Mon, 15 Jan 2001 12:34:56 GMT' ),
 227+ 'TS_RFC2822 to TS_MW' );
 228+
 229+ # WSP = SP / HTAB ; rfc2234
 230+ $this->assertEquals(
 231+ '20010115123456',
 232+ wfTimestamp( TS_MW, "Mon, 15 Jan\x092001 12:34:56 GMT" ),
 233+ 'TS_RFC2822 with HTAB to TS_MW' );
 234+
 235+ $this->assertEquals(
 236+ '20010115123456',
 237+ wfTimestamp( TS_MW, "Mon, 15 Jan\x09 \x09 2001 12:34:56 GMT" ),
 238+ 'TS_RFC2822 with HTAB and SP to TS_MW' );
 239+
 240+ $this->assertEquals(
 241+ '19941106084937',
 242+ wfTimestamp( TS_MW, "Sun, 6 Nov 94 08:49:37 GMT" ),
 243+ 'TS_RFC2822 with obsolete year to TS_MW' );
199244 }
200245
201246 /**
——@@ -274,6 +319,35 @@
275320 'ISO 8601:2004 [[year 0]], also called [[1 BC]]');
276321 }
277322
 323+ function testHttpDate() {
 324+ # The Resource Loader uses wfTimestamp() to convert timestamps
 325+ # from If-Modified-Since header.
 326+ # Thus it must be able to parse all rfc2616 date formats
 327+ # http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.3.1
 328+
 329+ $this->assertEquals(
 330+ '19941106084937',
 331+ wfTimestamp( TS_MW, 'Sun, 06 Nov 1994 08:49:37 GMT' ),
 332+ 'RFC 822 date' );
 333+
 334+ $this->assertEquals(
 335+ '19941106084937',
 336+ wfTimestamp( TS_MW, 'Sunday, 06-Nov-94 08:49:37 GMT' ),
 337+ 'RFC 850 date' );
 338+
 339+ $this->assertEquals(
 340+ '19941106084937',
 341+ wfTimestamp( TS_MW, 'Sun Nov 6 08:49:37 1994' ),
 342+ "ANSI C's asctime() format" );
 343+
 344+ // See http://www.squid-cache.org/mail-archive/squid-users/200307/0122.html and r77171
 345+ $this->assertEquals(
 346+ '20101122141242',
 347+ wfTimestamp( TS_MW, 'Mon, 22 Nov 2010 14:12:42 GMT; length=52626' ),
 348+ "Netscape extension to HTTP/1.0" );
 349+
 350+ }
 351+
278352 function testBasename() {
279353 $sets = array(
280354 '' => '',
Index: trunk/phase3/includes/GlobalFunctions.php
——@@ -1981,7 +1981,7 @@
19821982 * function will autodetect which format is supplied and act
19831983 * accordingly.
19841984 * @param $ts Mixed: the timestamp to convert or 0 for the current timestamp
1985 - * @return String: in the format specified in $outputtype
 1985+ * @return Mixed: String / false The same date in the format specified in $outputtype or false
19861986 */
19871987 function wfTimestamp( $outputtype = TS_UNIX, $ts = 0 ) {
19881988 $uts = 0;
——@@ -2014,14 +2014,23 @@
20152015 # TS_POSTGRES
20162016 } elseif (preg_match('/^(\d{4})\-(\d\d)\-(\d\d) (\d\d):(\d\d):(\d\d)\.\d\d\d$/',$ts,$da)) {
20172017 # TS_DB2
2018 - } elseif ( preg_match( '/^[A-Z][a-z]{2}, \d\d [A-Z][a-z]{2} \d{4} \d\d:\d\d:\d\d/', $ts ) ) {
2019 - # TS_RFC2822
 2018+ } elseif ( preg_match( '/^[ \t\r\n]*([A-Z][a-z]{2},[ \t\r\n]*)?' . # Day of week
 2019+ '\d\d?[ \t\r\n]*[A-Z][a-z]{2}[ \t\r\n]*\d{2}(?:\d{2})?' . # dd Mon yyyy
 2020+ '[ \t\r\n]*\d\d[ \t\r\n]*:[ \t\r\n]*\d\d[ \t\r\n]*:[ \t\r\n]*\d\d/S', $ts ) ) { # hh:mm:ss
 2021+ # TS_RFC2822, accepting a trailing comment. See http://www.squid-cache.org/mail-archive/squid-users/200307/0122.html / r77171
 2022+ # The regex is a superset of rfc2822 for readability
 2023+ $strtime = strtok( $ts, ';' );
 2024+ } elseif ( preg_match( '/^[A-Z][a-z]{5,8}, \d\d-[A-Z][a-z]{2}-\d{2} \d\d:\d\d:\d\d/', $ts ) ) {
 2025+ # TS_RFC850
20202026 $strtime = $ts;
 2027+ } elseif ( preg_match( '/^[A-Z][a-z]{2} [A-Z][a-z]{2} +\d{1,2} \d\d:\d\d:\d\d \d{4}/', $ts ) ) {
 2028+ # asctime
 2029+ $strtime = $ts;
20212030 } else {
20222031 # Bogus value; fall back to the epoch...
20232032 wfDebug("wfTimestamp() fed bogus time value: $outputtype; $ts\n");
20242033
2025 - return "Invalid date";
 2034+ return null;
20262035 }
20272036
20282037
——@@ -2050,9 +2059,17 @@
20512060 (int)$da[4], (int)$da[5], (int)$da[6]);
20522061
20532062 $d = date_create( $ds, new DateTimeZone( 'GMT' ) );
 2063+ } elseif ( $strtime ) {
 2064+ $d = date_create( $strtime, new DateTimeZone( 'GMT' ) );
20542065 } else {
2055 - $d = date_create( $strtime, new DateTimeZone( 'GMT' ) );
 2066+ return false;
20562067 }
 2068+
 2069+ if ( !$d ) {
 2070+ wfDebug("wfTimestamp() fed bogus time value: $outputtype; $ts\n");
 2071+ return false;
 2072+ }
 2073+
20572074 $output = $d->format( $formats[$outputtype] );
20582075 } else {
20592076 if ( count( $da ) ) {
——@@ -2065,7 +2082,8 @@
20662083 }
20672084
20682085 if ( $uts === false ) {
2069 - return "Can't handle date";
 2086+ wfDebug("wfTimestamp() can't parse the timestamp (non 32-bit time? Update php): $outputtype; $ts\n");
 2087+ return false;
20702088 }
20712089
20722090 if ( TS_UNIX == $outputtype ) {

Follow-up revisions

RevisionCommit summaryAuthorDate
r77404Follow up r77401....platonides22:27, 28 November 2010
r77584Strip If-Modified-Since header to a valid timestamp in ResourceLoader; some c...catrope16:54, 2 December 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r71750resourceloader: Implement support for inputting TS_RFC2822 into wfTimestamp()...catrope20:55, 26 August 2010
r71751Merge r71750 to trunk: let wfTimestamp() accept TS_RFC2822 as an input format...catrope21:02, 26 August 2010
r77171Bug 25451: time and date calculation in 32-bit ...platonides18:25, 23 November 2010

Comments

#Comment by OverlordQ (talk | contribs)   22:22, 28 November 2010

Breaks timestamps 

# php maintenance/tests/parserTests.php --record
This is MediaWiki version 1.17alpha ([[Special:Code/MediaWiki/77401|r77401]]).

PHP Warning:  pg_query(): Query failed: ERROR:  column "tr_date" is of type timestamp with time zone but expression is of type integer
LINE 1: ...on,tr_php_version,tr_db_version,tr_uname) VALUES (0,'1.17alp...
#Comment by OverlordQ (talk | contribs)   22:34, 28 November 2010

Wrong revision, broken by 77401

#Comment by Nikerabbit (talk | contribs)   07:03, 29 November 2010

I think this is the wrong place. It should be done in the resource loader processing the header.

#Comment by Platonides (talk | contribs)   09:36, 29 November 2010

I considered that, but I decided that it was better adding the strtok() in wfTimestamp().

It's also more robust in case other places also call wfTimestamp in a similar way. It worked on the previous wfTimestamp just by chance since the resource loader wasn't providing anything previous to the epoch, so it acted as if the If-Modified-Header was not there.

#Comment by Catrope (talk | contribs)   16:59, 2 December 2010 
+		# FWS = ([*WSP CRLF] 1*WSP) / obs-FWS ; Folding white space
+        # obs-FWS = 1*WSP *(CRLF 1*WSP) ; Section 4.2
+		$this->assertEquals(

The middle line uses spaces instead of tabs for indentation.

#Comment by Platonides (talk | contribs)   00:44, 3 December 2010

Was fixed by mah in r77539.

Status & tagging log