# datetime status - Thread: https://externals.io/message/56444 - Original archive: https://news-web.php.net/php.internals/56444 - Started by Stas Malyshev on 2011-11-21T01:41:17+00:00 - Messages: 7 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #56444 — Stas Malyshev — 2011-11-21T01:41:17+00:00 URL: https://externals.io/message/56444#56444 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 --- ## #56446 — Daniel Convissor — 2011-11-21T03:20:01+00:00 URL: https://externals.io/message/56444#56446 · In reply to: #56444 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 --- ## #56449 — Stas Malyshev — 2011-11-21T07:40:44+00:00 URL: https://externals.io/message/56444#56449 · In reply to: #56446 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 --- ## #56451 — Derick Rethans — 2011-11-21T08:42:53+00:00 URL: https://externals.io/message/56444#56451 · In reply to: #56444 > 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 --- ## #56452 — Derick Rethans — 2011-11-21T08:46:07+00:00 URL: https://externals.io/message/56444#56452 · In reply to: #56444 > 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 --- ## #56453 — Stas Malyshev — 2011-11-21T08:59:42+00:00 URL: https://externals.io/message/56444#56453 · In reply to: #56451 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 --- ## #56454 — Derick Rethans — 2011-11-21T09:27:30+00:00 URL: https://externals.io/message/56444#56454 · In reply to: #56453 > > […] > > 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