Could you elaborate what is broken without this fix?

t.glaser wrote:

Hm, it was quite some time ago, and I had to enable error reporting catching and backtraces for that, but the gist is, some things (extensions, for example, I believe, RSS_Reader) call time() and date() instead of timeanddate() when needed, and the $ts argument was not converted into the format sprintfDate() expects (which it accesses with substr() out of all things).

The timeanddate() method used wfTimestamp() for the conversion, but time() and date() didn’t.

This led to wrong timestamps as well as accesses to array index -1 when month==0 (to get the human-readable month name).

(In reply to comment #2)

Hm, it was quite some time ago, and I had to enable error reporting catching
and backtraces for that, but the gist is, some things (extensions, for example,
I believe, RSS_Reader) call time() and date() instead of timeanddate() when
needed, and the $ts argument was not converted into the format sprintfDate()
expects (which it accesses with substr() out of all things).

The timeanddate() method used wfTimestamp() for the conversion, but time() and
date() didn’t.

This led to wrong timestamps as well as accesses to array index -1 when
month==0 (to get the human-readable month name).

Wouldn't this be a bug in the extension not mediawiki? The function documentation says that the first parameter must be in YYYYMMDDHHMMSS format.

Wouldn't this be a bug in the extension not mediawiki? The function
documentation says that the first parameter must be in YYYYMMDDHHMMSS format.

Do you mean the text "the time format which needs to be turned into a
date('YmdHis') format with wfTimestamp(TS_MW,$ts)"?

yes

Well, it is not clear if that line specifies how you should format your input or what the function is internally doing with that parameter.

Just saying "time in YYYYMMDDHHMMSS format" would be clearer.

Lets see which are the broken extension.

Although it probably didn't do any harm in this case (the patch is fairly trivial), you should submit patches against trunk, not against the latest release version (1.16.0), and definitely not against the version before that (1.15.5).

t.glaser wrote:

OK, will do next time. I initially submitted that to Debian and asked them
to forward this, when 1.15.x was still current, but apparently nobody did
so I put it in here.

Anyway, please apply, it should be really trivial.

Fixed in r74778 based on your patch which didn't apply cleanly.

t.glaser wrote:

Based on the comments in r74778 I’ve prepared an additional patch mitigating this issue. I’ve tested this with mediawiki-in-Debian and rebased on trunk, which I however cannot test.

t.glaser wrote:

make wfTimestamp go beyond limitations of 32-bit integer

Attached:

(In reply to comment #11)

Created attachment 7736 [details]
make wfTimestamp go beyond limitations of 32-bit integer

DateTime was introduced in PHP 5.2.0, and MediaWiki still claims compatibility with 5.1.

Attached:

Potential solution is to just make wfTimestamp just return its second argument if its converting to TS_MW and the timestamp is already in mediawiki format. However that would change behaviour if wfTimestamp was passed an invalid timestamp in mediawiki format and asked to convert to TS_MW (but I can't imagine anyone is using wfTimestamp to validate timestamps).

You should use date_create() instead of DateTime, it doesn't throw an exception.

You are missing timezones.

This line duplication is ugly.

t.glaser wrote:

@5.1 compatibility: ouch, damn. (But then, without DateTime, there’s a limitation to the int type anyway.)

@timezones: no, the non-DateTime code uses gm*() which do not use timezones either, so this is actually correct

@duplication: sure, but that patch was minimal-intrusive

@timezones: no, the non-DateTime code uses gm*() which do not use timezones
either, so this is actually correct

Even when the timezone was appended in the PHP? :)

Regarding duplication, I'd prefer to have it as an array of constants->formats and then choose DateTime or gmtime() depending if it's available (' GMT' appending makes is a bit trickier).

Another option would be to create a DateTime class for PHP < 5.1 to use as fallback everywhere.

I looked at going to date_create() if gmmktime() returned false, but as gmdate() internally casted the string to 32 bits int, the result was also wrong.

Also note you miss a second DateTimeZone() parameter to skip the PHP date warning.

Fixed in r77171 (r77204).

overlordq wrote:

Breaks timestamps

overlordq wrote:

Fixed in r77409