Last modified: 2010-11-28 22:58:37 UTC
Created attachment 7720 [details] Bugfix, as committed in Debian’s pkg-mediawiki SVN, by myself The timeanddate method is correct. The time and date methods aren’t. This fix was developed as part of the Evolvis project integrating MediaWiki and FusionForge further and packaging stuff in Debian. Please apply.
Could you elaborate what is broken without this fix?
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).
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.
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.
Created attachment 7736 [details] make wfTimestamp go beyond limitations of 32-bit integer
(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.
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.
@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).
Breaks timestamps
Fixed in r77409
