Hi!
We're in RC now, and I'm very concerned about the status of DateTime
functionality (see bugs 60236, 60237 and XFAILs) - I've reported these
problems in June, but not much happened since then. I'm trying to figure
out what's going on there, but I'm not sure if I understand the code
correctly and if the fix is right.
So far this is what I got:
When parsing string "2010-11-06 18:38:28 EDT" with timelib_strtotime, I
get back the correct datetime elements, and these for timezone:
z = 300,
tz_abbr = 0x2633670 "EDT",
tz_info = 0x0,
dst = 1,
And from this and the code in do_adjust_timezone() I gather that "z" is
supposed to be combined with "dst" when calculating timezone offsets.
But timelib_unixtime2local() uses only "z" and ignores "dst" completely.
So which one "z" is supposed to be?
If it is the former, the patch seems to be pretty simple:
--- lib/unixtime2tm.c (revision 319533)
+++ lib/unixtime2tm.c (working copy)
@@ -146,7 +146,7 @@
int z = tm->z;
signed int dst = tm->dst;
-
timelib_unixtime2gmt(tm, tm->sse - (tm->z * 60));
-
timelib_unixtime2gmt(tm, tm->sse - (tm->z * 60) + tm->dst*3600); tm->z = z; tm->dst = dst;
@@ -184,7 +184,7 @@
int z = tm->z;
signed int dst = tm->dst;
-
timelib_unixtime2gmt(tm, ts - (tm->z * 60));
-
timelib_unixtime2gmt(tm, (ts - (tm->z * 60) + tm->dst*3600)); tm->z = z; tm->dst = dst;
It does not add any test breakage and fixes two XFAILs. Also, I see that
sse value is not assigned here, though later the code goes:
/* we need to reset the sse here as unixtime2gmt modifies it */
tm->sse = ts;
Do we need to update sse in this branch too?
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi Stas:
We're in RC now, and I'm very concerned about the status of DateTime
functionality (see bugs 60236, 60237 and XFAILs)
60237 is a duplicate of https://bugs.php.net/bug.php?id=55253. I just
marked it as such. I suspect the remaining two bugs are related to the
same underlying problem.
It does not add any test breakage and fixes two XFAILs.
For fixing the TLA / Zone Type 2 bugs, focus on the following tests:
bug55253.phpt
rfc-datetime_and_daylight_saving_time-type2.phpt
DateTime_*-type2-type2.phpt
Some of the other XFAILS will be fixed by implementing the DST RFC and
the remaining ones by my adjusting them once the DST behavior is fixed.
Thanks,
--Dan
--
T H E A N A L Y S I S A N D S O L U T I O N S C O M P A N Y
data intensive web and database programming
http://www.AnalysisAndSolutions.com/
4015 7th Ave #4, Brooklyn NY 11232 v: 718-854-0335 f: 718-854-0409
Hi!
For fixing the TLA / Zone Type 2 bugs, focus on the following tests:
bug55253.phpt
rfc-datetime_and_daylight_saving_time-type2.phpt
Those are exactly the two xfails fixed by my patch.
DateTime_*-type2-type2.phpt
These aren't fixed but it may be related to the fact that EDT is applied
to time that is not actually in EDT but in EST. I didn't find it in RFC
but I think in this case it should act as if it was EST there.
Some of the other XFAILS will be fixed by implementing the DST RFC and
the remaining ones by my adjusting them once the DST behavior is fixed.
Do you have a patch for RFC behavior?
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
If it is the former, the patch seems to be pretty simple:
--- lib/unixtime2tm.c (revision 319533)
+++ lib/unixtime2tm.c (working copy)
@@ -146,7 +146,7 @@
int z = tm->z;
signed int dst = tm->dst;
timelib_unixtime2gmt(tm, tm->sse - (tm->z * 60));
timelib_unixtime2gmt(tm, tm->sse - (tm->z * 60) +tm->dst*3600);
tm->z = z; tm->dst = dst;@@ -184,7 +184,7 @@
int z = tm->z;
signed int dst = tm->dst;
timelib_unixtime2gmt(tm, ts - (tm->z * 60));
timelib_unixtime2gmt(tm, (ts - (tm->z * 60) +tm->dst*3600));
tm->z = z; tm->dst = dst;
That is incorrect, not every timezone DST change is 60 minutes.
cheers,
Derick
Hi!
That is incorrect, not every timezone DST change is 60 minutes.
The I would guess this code in do_adjust_timezone() also wrong:
case TIMELIB_ZONETYPE_ABBR: {
timelib_sll tmp;
tz->is_localtime = 1;
tmp = tz->z;
tmp -= tz->dst * 60;
tmp *= 60;
return tmp;
}
Since it makes the same assumption?
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
That is incorrect, not every timezone DST change is 60 minutes.
The I would guess this code in do_adjust_timezone() also wrong:
case TIMELIB_ZONETYPE_ABBR: { timelib_sll tmp; tz->is_localtime = 1; tmp = tz->z; tmp -= tz->dst * 60; tmp *= 60; return tmp; }Since it makes the same assumption?
Maybe. It's not a huge problem and I think I might have shouted too
soon. Perhaps ABBR types are only triggered for a specific hard coded
list where the DST bit is always an hour. I'll need to check that!
(Added to my list).
Derick
We're in RC now, and I'm very concerned about the status of DateTime
functionality (see bugs 60236, 60237 and XFAILs) - I've reported these
problems in June, but not much happened since then. I'm trying to
figure out what's going on there, but I'm not sure if I understand the
code correctly and if the fix is right.
Still didn't get to it :-/ I'm having some out-of-office no-internet
train time on Thu/Sat so that time is reserved for looking at those
issues.
cheers,
Derick