Many people have wanted to have a Daylight Saving Time setting in the past. If we use the "server time" modell (all times are displayed the same around the world) this isn't all that hard to do.

I found a lot of usefull information here:
http://webexhibits.org/daylightsaving/g.html

The patch introduces a new DST setting and uses it if the "Configurable time zones" setting is switched off.

Files: 
CommentFileSizeAuthor
#362 d7.timezone42.patch36.62 KBmfb
Passed: 7299 passes, 0 fails, 0 exceptions
[ View ]
#352 11077.patch34.44 KBDamien Tournoud
Passed: 7341 passes, 0 fails, 0 exceptions
[ View ]
#350 11077.patch34.44 KBDamien Tournoud
Failed: Failed to install HEAD.
[ View ]
#346 d7.timezone41.patch35.53 KBmfb
Failed: Failed to apply patch.
[ View ]
#341 11077-daylight-saving-time.patch31.93 KBDamien Tournoud
Failed: Failed to apply patch.
[ View ]
#339 11077-daylight-saving-time.patch31.27 KBDamien Tournoud
Failed: Failed to apply patch.
[ View ]
#332 d7.timezone40.patch33.61 KBmfb
Failed: Failed to apply patch.
[ View ]
#324 d7.timezone39.patch34.15 KBmfb
Failed: Failed to apply patch.
[ View ]
#321 d7.timezone38.patch34.14 KBmfb
Failed: Failed to apply patch.
[ View ]
#308 d7.timezone37.patch33.97 KBmfb
Failed: Failed to apply patch.
[ View ]
#305 d7.timezone36.patch33.34 KBmfb
Failed: Failed to apply patch.
[ View ]
#302 d7.timezone35.patch33.31 KBmfb
Failed: Failed to apply patch.
[ View ]
#298 d7.timezone34.patch32.9 KBmfb
Failed: Failed to apply patch.
[ View ]
#292 d7.timezone33.patch33.41 KBmfb
Failed: Failed to apply patch.
[ View ]
#287 drupal.timezone.diff.patch12.79 KBsun
Failed: Failed to apply patch.
[ View ]
#285 d7.timezone32.patch32.1 KBmfb
Failed: Failed to apply patch.
[ View ]
#283 drupal.timezone.patch31.61 KBsun
Failed: Failed to apply patch.
[ View ]
#266 d7.timezone31.patch42.47 KBmfb
Failed: Failed to apply patch.
[ View ]
#259 d7.timezone30.patch42.59 KBjjkd
Failed: Failed to apply patch.
[ View ]
#252 d7.timezone29.patch42.58 KBmfb
Failed: Failed to apply patch.
[ View ]
#246 d7.timezone28.patch42.4 KBmfb
Failed: Failed to apply patch.
[ View ]
#240 d7.timezone27.patch42.15 KBmfb
Failed: Failed to apply patch.
[ View ]
#232 d7.timezone26.patch41.53 KBmfb
Failed: Failed to apply patch.
[ View ]
#228 d7.timezone25.patch39.66 KBmfb
Failed: Failed to apply patch.
[ View ]
#226 d7.timezone24.patch38.25 KBmfb
Failed: Failed to apply patch.
[ View ]
#223 d7.timezone23.patch38.16 KBmfb
Failed: Failed to apply patch.
[ View ]
#220 screenshot-timezone-install.png69.78 KBlilou
#220 screenshot-timezone-config.png92.67 KBlilou
#220 screenshot-timezone-user.png41.43 KBlilou
#219 d7.timezone22.patch38.15 KBmfb
Failed: Failed to apply patch.
[ View ]
#218 d7.timezone21.patch38.16 KBmfb
Failed: Failed to apply patch.
[ View ]
#217 d7.timezone20.patch37.99 KBmfb
Failed: Failed to apply patch.
[ View ]
#215 d7.timezone19.patch37.97 KBmfb
Failed: Failed to apply patch.
[ View ]
#211 d7.timezone18.patch38.07 KBmfb
Failed: Failed to apply patch.
[ View ]
#202 d7.timezone17.patch38.08 KBmfb
Failed: Failed to apply patch.
[ View ]
#201 d7.timezone16.patch38.01 KBmfb
Failed: Failed to apply patch.
[ View ]
#196 drupal7.patch36.71 KBKarenS
Failed: Failed to apply patch.
[ View ]
#193 drupal7.patch25.89 KBKarenS
Failed: Failed to apply patch.
[ View ]
#189 drupal7.patch25.16 KBKarenS
Failed: Failed to apply patch.
[ View ]
#186 d7.timezone15.patch23.05 KBmfb
Failed: Failed to apply patch.
[ View ]
#176 d7.timezone14.patch21.8 KBmfb
Failed: Failed to apply patch.
[ View ]
#175 timezone_translate_build.txt491 bytesmfb
#166 d7.timezone13.patch21.26 KBmfb
Failed: Failed to apply patch.
[ View ]
#165 d7.timezone12.patch21.28 KBmfb
Failed: Failed to apply patch.
[ View ]
#162 d7.timezone11.patch21.48 KBmfb
Failed: Failed to apply patch.
[ View ]
#157 d7.timezone10.patch21.96 KBmfb
Failed: Failed to apply patch.
[ View ]
#155 d7.timezone9.patch20.92 KBmfb
Failed: Failed to apply patch.
[ View ]
#154 d7.timezone8.patch17.68 KBmfb
Failed: Failed to apply patch.
[ View ]
#152 d7.timezone7.patch12.18 KBmfb
Failed: Failed to apply patch.
[ View ]
#151 d7.timezone6.patch12.2 KBmfb
Failed: Failed to apply patch.
[ View ]
#148 d7.timezone5.patch12.19 KBmfb
Failed: Failed to apply patch.
[ View ]
#147 d7.timezone4.patch12.12 KBmfb
Failed: Failed to apply patch.
[ View ]
#146 d7.timezone3.patch12.13 KBmfb
Failed: Failed to apply patch.
[ View ]
#131 d7.timezone2.patch11.45 KBmfb
Failed: Failed to apply patch.
[ View ]
#130 d7.timezone.patch11.45 KBmfb
Failed: Failed to apply patch.
[ View ]
#107 drupal_107.patch32.78 KBnlindley
#104 drupal_72.patch33.29 KBKarenS
#94 newtz_0.patch31.39 KBmfb
#84 newtz.patch31.39 KBmfb
#83 tz.patch31.39 KBmfb
#71 drupal_71.patch16.61 KBKarenS
#44 timezones_0.patch9.3 KBKarenS
#34 dst_1.patch6.42 KBJaza
#29 dst_473.patch.txt4.94 KBgorgen
#3 dst_0.patch6.04 KBkilles@www.drop.org
dst.patch6.04 KBkilles@www.drop.org

Comments

gordon’s picture

This is a lovely patch, but you do realise that you have opened a can of worms.

Daylight saving is a messy business, I had to write the Contestable Electricity billing for a company here in Australia which had multiple installations in multpile timezones, all with different daylight savings rules, which impacted upon the bill. The way that dst is legistated is very messy and here is Australia it is very messy. Daylight savings don't follow state lines, as some towns that are very close to the border may choose to follow the only the daylight saving period of the next state, and when we had the olympics NSW went into daylight savings months before anyone else. There are other examples of weird things like even the 1 hour forward is not also true. I know that in the 1999 change to daylight savings there was a small set of islands north of New Zealand that went ahead by 2 hours as they wanted to be the first into 2000.

From what I can see of your patch it only allows for a single daylight savings period for the entire system. Their can be situations where there can be users from multiple countries and in different timezones/dst.

I think that entire timezone system needs to looked at again, and maybe re-implement it so that it will allow for dst. Maybe make it a bit more user friendly so instead of setting up the timezone by name much like how operating systems do it, and then this will automatically work out the timezone.

+1 for this is something that would be good, but it still needs work.

Anonymous’s picture

The patch works as it is now. I had good reason to limit its capability to the setting where users cannot chose their own timezone. ;)
I also assume that a server administrator knows if he has dst or not. I also didn't (yet) cater for all countries where dst is in use mainly because some insist on not giving a sensible rule for the change dates.

killes@www.drop.org’s picture

StatusFileSize
new6.04 KB

The last comment was mine.

After a code review by Kjartan I provide a slightly improved patch.

I want to stress that solving the DST issue in a "suits everybody" manner isn't the intention of this patch.
It solves the problem for people who have chosen to let all times on their site be displayed in the server's time setting. By covering a large portion of the earth's landmass I hope to be able to help most site admins.

It would be possible to extend the patch to allow users to chose their DST setting, too. In my experience, however, users tend to not care about time settings at all. This may be EUrocentric though, as we have only one time zone. Feedback is welcome.

drumm’s picture

+1

From a code review I think this is an excellent solution. Could use a little more comments in the section that sets $node->date to explain the (probably good) reasoning behind that chunk of code. I don't have anything resembling a good set of tests so I did not apply the patch and try it.

killes@www.drop.org’s picture

I just want to point out that this patch still applies and would make for a fine feature for Drupal 4.6...

alliax’s picture

wouldn't hurt to have this feature in upcoming drupal.
I would use it for sure, my users are from the same place, with dst

Sage’s picture

This is an incredibly useful feature and I would like to see it incorporated into the next release (current release is 4.6.0).

syntaxerror’s picture

Works beautifully.. I'm going to work on getting this to work with event.module.

crunchywelch’s picture

I have just discussed with killes and rorris an enhancement to this patch which I will implement. We have proposed putting the ical timezone db (http://cvs.drupal.org/viewcvs/drupal/contributions/modules/ical/ical.sql...) in core. We will implement a DST field in that db for each zone which coreesponds with the zones in killes' patch here for dtermining whether a zone is currently in dst or not. Currently, the 5 zones in the drupal_is_dst() function in this patch will be the only regions assigned to timezones. If more regions are desired they can be submitted by the community.

To be clear, this must be implemented before additional development on ical/event.module can proceed properly. We would like this to be in core for 4.7, and move one more step with timezone handling in event module to use this as well.

Discuss!

tostinni’s picture

I agree to put more specific region to DST as it's easier for someone to know which city DST he belongs than which DST Zone (Pacific Standard Time...).
I like pretty much the way that windows manage this.
+1

Jaza’s picture

The patch looks good... however, it doesn't take into account the fact that in Australia, DST starts and ends at 2am on Sunday morning. So these lines:

    case 4: // Australia
      // start of DST  (last Sunday in October)
      $dststart = strtotime("-1 week sunday GMT", strtotime("1 november $year GMT")) + $timezone;
      // end of DST (last Sunday in March)
      $dstend = strtotime("-1 week sunday GMT", strtotime("1 april $year GMT")) + $timezone;
      break;

Should read:

    case 4: // Australia
      // start of DST  (last Sunday in October at 2am)
      $dststart = strtotime("-1 week sunday GMT", strtotime("1 november $year GMT")) + 7200 + $timezone;
      // end of DST (last Sunday in March at 2am)
      $dstend = strtotime("-1 week sunday GMT", strtotime("1 april $year GMT")) + 7200 + $timezone;
      break;

There are also a whole lot more countries (and their specific DST rules) on http://webexhibits.org/daylightsaving/g.html than are implemented in this patch. I'd ultimately like to see an admin interface to let the webmaster add additional DST rules for whatever country they live in (or are developing for). These rules could be entered in raw PHP, or using a somewhat friendlier interface.

Jaza.

Steven’s picture

+1 on this patch. Catches most cases and at least provides a base for others to work on.

gordon’s picture

Do not take this the wrong what, but when it comes to dst, this is exemely simplisict.

What would be the best method of doing this is to use the Zone Info files from libc. These not only have all the dst splits, but also all the timezones, and would be the most acurate.

Have a look at Sources of Time Zone and Daylight Savings Data whcih has pointers to where all this information is.

Did you know that Australia has 21 different Timezones and DST areas.

If you could enter the same information from tzdata then this would be the altermate dst system

jhriggs’s picture

There has sure been a lot of talk about DST and time zones lately. All I ask is that we hold off on committing anything for a bit. I am currently working on a project that will hopefully fix this issue completely, correctly, and once and for all. :-)

I am in the process of writing a pure PHP library that will be released LGPL that uses the real zoneinfo data files to handle any date (not just Unix timestamps) in any time zone. With this, site admins and/or users will be able to select their real time zone (America/New_York vs. our current GMT -500) and DST will inherently be handled without users having to change their settings. Much of the grunt work is already done. I still have to put all of the pieces together and make sure everything is well documented. As soon as I have tested everything to my satisfaction, I will role a Drupal patch that we can hopefully get tested and committed ASAP. Of course I want to see this get into Drupal first! ;-)

I hope to have everything completed in the next couple of weeks, but based on the way things have been going lately, I can't make any promises. It will definitely be done before 4.7, though...whatever it takes!

gordon’s picture

+1

This sounds great, but I think we will still need a pure php version that will be able to be used on hosted web sites where we will not be able to get you library installed. It doesn't need to be as effient as the php extension, as sites that at that big will be self hosted and be able to install this library.

One question. Does you extension run on windows. If it does this is another reason to have a pure php version.

This is really the method that should be employed.

jhriggs’s picture

When I say "library," I just mean a collection of PHP files as opposed to a single file. There is nothing to be installed like an extension. This is pure PHP that will simply be part of the distribution (a handful of .php files placed inside includes/zoneinfo or something like that). As this is pure PHP, it will work anywhere PHP does. This will essentially give us the benefit and functionality of using the TZ variable on Unix without all of the problems associated with that (i.e. not knowing what zones are available and what there names are, not being available on Windows, etc.).

gordon’s picture

This sounds good. If you would like someone who has done alot of stuff with timezones and dst (mainly in the contestable electricity inductry in australia, which is required for billing), I would be quite happy to take a look.

The only problem that I see is that it sounds a little too messy for core. maybe this could be made into a contributed module that can be installed if people need the correct timezones.

However this is how the dst and timezones need to be done if we want to get them correct. As I said before and I couldn't believe it, Australia has 21 different timezones and dst areas. The timezone database needs to be maintained.

killes@www.drop.org’s picture

jhriggs, gordon: Please have a look at crunchywelch's dst implementation for event.module (in 4.6 and cvs). We intend to push this into core.

mfb’s picture

Great! event_timezones.inc definitely needs to be in core, as it would be used for both user and site configuration. But it should include everything from the unix zoneinfo libraries, e.g. timezone abbreviations used by strftime(), so you can display "PDT" or "PST" instead of "America/Los_Angeles".

so, a complete port of the zoneinfo libraries to PHP is probably what is needed. This would be used for site config, user config and events.

glen’s picture

one idea, maybe use mysql timezone tables that are available since mysql 4.1.3?
http://www.visionwebhosting.net/mysql-web-hosting/page0403.htm

not sure how it's possible with licensing, but original tzdata is GPL (at least according to Fedora Core 2 tzdata.spec) then mysql data table contents should be too GPL.

good side is that it should work for windows too http://dev.mysql.com/downloads/timezones.html

mfb’s picture

It would be great to use MySQL 4.1 as the solution but it probably isn't (yet) possible for Drupal to require MySQL 4.1 when many webhosts are still using MySQL 4.0. We'll have to use our own zoneinfo library. Which by the way will need an update if the U.S. changes daylight saving time.

praseodym’s picture

+1 for this feature, it's irritating to change timezones every time DST is set.

m3avrck’s picture

Any updates? Love to see this for 4.7, seems this has been simmering for quite sometime now.

Sage’s picture

This appears not to have been integrated into version 4.7 as indicated above, at least not as of beta 5. Is this still going to make it in?

Sage’s picture

This patch fails when applied to Drupal 4.7 rc2:

$ patch -p0 <dst_0.patch
patching file includes/common.inc
Hunk #1 succeeded at 844 (offset -116 lines).
Hunk #2 FAILED at 1630.
1 out of 2 hunks FAILED -- saving rejects to file includes/common.inc.rej
patching file modules/node.module
Hunk #1 FAILED at 1053.
1 out of 1 hunk FAILED -- saving rejects to file modules/node.module.rej
patching file modules/system.module
Hunk #1 FAILED at 216.
Hunk #2 FAILED at 249.
2 out of 2 hunks FAILED -- saving rejects to file modules/system.module.rej

It is my understanding that crunchywelch's attempts to move his DST implementation from the event module into Drupal core have been met with resistance. This leaves us with no DST solution for 4.7. Would it be at all possible to do this with a third-party module rather than a patch?

killes@www.drop.org’s picture

Status:Needs review» Needs work

Yeah, it didn't make it. We haven't been pushing hard enough. Somebody wanted to make a stand-alone module, not sure it will work, though.

Sage’s picture

What else can be done on this, then? I'm not much of a programmer, but I would like to help out; this is the one last big feature missing from Drupal for me. Should we start a project where we can better organize this effort? Let me know what I can do to help make this happen.

forngren’s picture

We could create a group for this :/

What else can be done on this, then? This is MUST have feature for 4.8...
gorgen’s picture

StatusFileSize
new4.94 KB

I needed this patch for a 4.7.3 site so I modified it and here it is if someone else needs it..

Jaza’s picture

New patch against Drupal 4.7.4. Exactly the same as the patch in #29, except that it includes the fix for the Australian region that I pointed out in #11. This patch is still very much needed, IMO.

andrewfn’s picture

I second that! Drupal not handling daylight savings is a constant headache for me and I am embarrassed each time I have to explain this fact to customers. Let's get the patch committed asap.

Jaza’s picture

StatusFileSize
new6.42 KB

A few more fixes for AU and NZ daylight savings in this patch.

bradlis7’s picture

I don't think the patch is implemented this way, but I think the user ought to be able to choose CST, or CDT (where C would be different for the timezone), and then it will automatically calculate whether it is DST currently or not. That would add almost twice as many timezones to the list, but it would make sense to me. Or, you could just do CST, and CST (Daylight Savings) just so the users know what's going on.

Or you could just do a checkbox...

Is there a chance this will go into 5.0, or is it too late?

AjK’s picture

Version:x.y.z» 4.7.4

Had fun trying to apply this patch till I read the patch and then saw #31, it's for 4.7 not CVS. Changing version

DaveNotik’s picture

Status:Needs review» Active

Hmmm... can this be turned into a feature request for 5.0 (or the next version if this is considered a major feature)?

Not cool having to go back and forth from -400 to -500 times whenever the clock changes.

--D

flaviovs’s picture

This is a fundamental feature, and I think that for 5.0 we should try to do the "right thing", that is, use a database that maps locations to time zone information.

In this scheme users should not select which time zone they're are on anymore, but select where they're located.

The time zone database can them be used to map user locations to their correct time zone plus DST settings.

We must not forget that DST support is not only about changing display time on specific date/time. On many countries DST start/end dates change every year. You may have a post made LASTYEAR-MONTH-DAY at 13pm that have been done on DST, but even if THISYEAR-MONTH-DAY is not on DST because DST start date has changed, last year's post should still be shown as been made at 13pm.

This mean that such database should not contains DST start/end dates for every location, but keep historical data about DST changes.

Fortunately, this is exactly how time zone handling is implemented on most modern OS (including Linux, *BSDs and Un*xes). Unfortunately there's no official support on PHP < 5 for changing timezones on-the-fly, though it looks like setting the TZ environment variable does the trick (see http://www.php.net/manual/en/function.putenv.php).

mfb’s picture

I've always used putenv on non-drupal sites.. e.g. putenv('TZ=America/Buenos_Aires'); then putenv('TZ'); when you're done to go back to the server default.

Now that timezones are better supported by PHP 5.1, how about rewriting format_date(), system_date_time_settings() etc. to take advantage of this functionality if it's available?

awgrover’s picture

At least on Unix, and php 5.1, wouldn't a combination of:
date_default_timezone_set($userTZ || $siteTZ);
date($formatString, $utcTimeStamp);
correctly format the time, including DST? This avoids writing your own timezone tables, etc.

Does this reliably work on Windows?

For earlier PHP, putenv('TZ=SomeValue') may work, instead of date_default_timezone_set(). I found it problematic (the timezone appeared to be cached between calls to date()).

Does this address and solve the timezone issues?

A backward compatible patch would operate as now for numeric 'configurable_timezones'/'date_default_timezone' values, but use the above mechanism otherwise.

awgrover’s picture

At least on linux, putenv('TZ=SomeValue') works fine: as long as the timezone-name is good (see the PHP appendix for supported timezone-names).

KarenS’s picture

Assigned:crunchywelch» KarenS
StatusFileSize
new9.3 KB

I have created a patch that would introduce this feature on systems that support the new timezone handling in php 5. It degrades to the current system for systems still using php 4. This I think is in line with recent discussions about starting to add in features that only work in php 5 so long as there is some decent fallback for php 4.

So this patch:

1) Adds a function drupal_handle_timezones() which will check if the system can use php timezone handling.

2) Adds a 'zonename' column to the users table where we can store the timezone name as well as the offset

3) On the system date/time settings page and user edit page, checks if php timezone handling is enabled. If so, it presents a list of timezone names to choose from instead of offsets.

4) Adds two functions to automatically calculate the site and user offsets from the timezone name and update the system variable and user records. These are used when the site and user timezone info is updated, and by cron.

5) Adds a cron function to update the timezone offsets once a day.

6) For environments where the php timezone functions won't work, the system behaves exactly as it always has, using offsets.

Since it uses the new php timezone handling, there is no need to load in any lists of timezone names, they can be called using a php function, so there is not a lot of new code needed to do it this way.

Boris Mann’s picture

Nice! Just wanted to mention there is also discussion on g.d.o about this: http://groups.drupal.org/node/3407

mfb’s picture

It doesn't seem too elegant to maintain the old timezone offsets as separate data entities when you're actually using the correct named timezones.

I was hoping there'd be some way to abstract this out and make the offset available via the php functions -- I didn't realize modules would directly use the timezone offset column, as opposed to $user->timezone, calling format_date(), etc.

The only way to literally simulate the offset column would be to use MySQL's timezone support -- but I'm guessing that's not an option for db compatibility ;)

Thanks for the patch, I will try it soon. Will be a huge step forward for radio, TV and other sites where precise times and timezones are important.

KarenS’s picture

My original thought was to get rid of the offset column, but didn't for two reasons:

1) Backwards compatibility. There is no way to continue to provide the existing behavior for sites using php4 without it.

2) It actually is sometimes useful to have it in there. As a part of the database you can use it in SQL queries, like Views. Plus there's not that much overhead for that one small column.

And the database compatibility problem is worse than you may realize since even if you ignore other databases, mysql's handling of timezones varies depending on the version you are using and the way it is configured.

killes@www.drop.org’s picture

You should make sure that the tz identifiers are translated on output.

KarenS’s picture

Actually, that just occurred to me, too. Not sure of the best way to do that since we're bringing in the list using php. Maybe iterate over the list and apply t() to each, then store it?? I tried to see if php does locale adjustments on it, but I don't think so.

KarenS’s picture

Does Drupal use setlocale()? If so, maybe we can do something using strftime's %z to output the translated name of the timezone.

mfb’s picture

I see how the timezone offset column needs to stay for now, in case a site is reverted to PHP4 environment.

But the overall idea here should be that the old-style timezone offset is not used by views and other modules, because the offset is incorrect half the time (if it's currently summer, a date/time in winter will be have the incorrect time displayed, and vice versa). Views needs access to the correct timezone-based time display.

mfb’s picture

strftime (on my system) returns timezone abbreviations like PDT, which don't vary by locale setting. Should be fine to just wrap in t() whenever a timezone string is being displayed.

KarenS’s picture

There is really nothing wrong with using the offset, so long as it is correct. This patch should correct it for systems using php5, and systems using php4 will be in the same position they are in now. You could go through the code and remove all references to the timezone column and find different ways to do it, but it will involve a lot of extra code to provide two alternative different ways to do everything, one that works in php4 and one that works in php5. That seems unnecessary when everything will work just fine as long as the offset is correct.

strftime has two strings for the timezone, %z and %Z. The exact thing they return varies depending on how the system was configured and the library that was used, but generally one returns the full name of the timezone and the other returns the abbreviation. If set_locale() has been used, they will return the translated values, so if you see no difference by locale, it appears set_locale() has not been set. If we're not already using set_locale() in Drupal, we should consider it -- there are a number of php functions that will automatically adjust to local formats/translations if it is set.

Anyway, you do NOT want to combine strftime() and t(), you need one or the other. t() expects the base value to be in English, but stftime is supposed to be returning a value in the local language, and we don't want that wrapped in t(). So we could use t() on the php timezone name, which is in English. If we do that, someone will need to provide translations for all those values. The ideal would be if we can use strftime and have it work correctly (i.e. if set_locale() has been used) since then the translations will be done by php.

mfb’s picture

Well the crux of this issue is that a generic offset is useless so I would say isn't worth using in a SQL query, it has to be calculated on the fly for each date -- this is what the new PHP5 timezone support provides. If my user account has an offset of -07:00 in summer this will be incorrect if I am actually trying to display a past or future date/time in winter. In other words it's incorrect half the time..

I was suggesting to use t() and not bother with strftime(), which does not (at least on FreeBSD 6.2) provide localized timezone names (it only localizes the day of the week) nor return full timezone names (only an abbreviation).

KarenS’s picture

I misunderstood you on strftime, I thought you were proposing to wrap the strftime result with t().

On the question of whether or not to drop the timezone column, it would be interesting to see exactly how many places in core it is actually used. If there are not many, maybe it is worth rewriting the code and getting rid of the column. Are you interested in digging into the code and figuring out exactly where that column is being used now?

I was also mulling over other options. One other approach would be to rewrite timezone functions a bit so they use wrapper functions (drupal_date(), drupal_time(), etc.) that will use the php5 functions if php5 is installed and include an .inc file that does some sort of workaround timezone handling for php4. Then we could get rid of the file completely once we can require php5. The workaround .inc file could have the timezone list and maybe do something using putenv(TZ) to get quasi timezone handling in php4. I tried using putenv() in early versions of the date module and it didn't work in all situations and actually created errors in some, so I need to dig back through my issues to try to figure out what the problems actually were.

KarenS’s picture

Status:Needs review» Needs work

I got curious and did some digging around and that timezone column is only used in a couple places in core, so maybe getting rid of it is easier than I thought. I have also been playing around with the idea of using wrapper functions for each of the new timezone functions and a php4 .inc file to make php4 mimic things that php5 does natively and I'm starting to think we could make that work and it might be a good solution.

Why don't we take this discussion back to http://groups.drupal.org/node/3407 and flesh out an approach that we can bring back here as a patch?

@killes, you've done lots of work on this already and I suspect we can use some of what you've already done in the php4 .inc files, so what do you think?

mfb’s picture

I finally installed drupal head so I can try out this patch. But, it no longer applies :(
Too many changes in system.module and system.install. Can you re-roll?
Also, it's preferable to make a patch with unix line endings (rather than DOS).

mfb’s picture

Btw, the main thing I'd like to add to this patch is, for format_date() in common.inc to use the zonename to do a correct offset calculation each time it's called. Of course if PHP timezone support is not present it would use the offset.

Xano’s picture

I think it would be a good idea to base the time Drupal uses on the timezone the server is in. Users can then override this setting for themselves by choosing their own DST/summertime options. Because lots (if not most) of servers are in the country they serve this would be a good start. Advanced DST controls could be made later if possible/necessary.

mfb’s picture

we could call date_default_timezone_get() (if the function exists) during drupal installation and use it to configure the site timezone. This function takes into account the TZ env variable (server timezone) in "guessing" the timezone. this actually isn't that useful for me personally since, all my servers are all in one timezone but most of the sites I install are for other timezones.

forngren’s picture

How does http://drupal.org/node/105039 affect this patch?

nlindley’s picture

Nice work, KarenS. I think you've got things going in the right direction, but there are a couple things that concern me about the patch. Please correct me if I'm wrong in my understanding of how it works.

This patch appears to not really implement a timezone, but merely apply a different offset based on timezone that gets updated everyday. For example, I live in Oklahoma and am currently in CDT, so my timezone would America/Chicago. Now, let's say I posted a comment back in January at 1:00 PM CST, the offset is -0600, but now that it's summer, it would say the comment was posted at 2:00 PM because my offset is currently -0500.

Based on that simple scenario, and the fact that daylight savings time doesn't always happen the same time of the year, it seems like we should store the timezone, and then give that information to whatever function formats the date.

Anyway, I've got more thoughts, but I'm putting the rest of my discussion at http://groups.drupal.org/node/3407

mfb’s picture

One thing I've been thinking about is that, having to chose "America/Chicago" or whatever from a dropdown may be too user un-friendly, for those who don't know their unix timezone. We may at least want a contrib module with the world map that operating systems use. of course, that may be pretty unwieldy to reproduce in a webpage ;)

KarenS’s picture

I just posted some more observations/comments in the gdo link, I'm trying to see if we can get a small patch into 6x before code freeze that will at least start moving us in this direction.

As to the timezone names -- there are actually multiple names for every zone, so Wisconsin could be 'America/Chicago' or 'US/Central'. I think most users would have no trouble understanding 'US/Central', for instance. The downside is that the 'US/...' zonenames are at the very bottom of the list, so you might not realize they're there.

Crell’s picture

Could the timezone be specified with an ajax autocomplete callback? Then we can stack in as many variants as we want and users can just type "US/C" and start getting zones.

Gábor Hojtsy’s picture

Indeed, a small patch for D6 would be great to get timezones moving properly (pun intended :).

KarenS’s picture

There's lots of discussion about this going on at http://groups.drupal.org/node/3407. At this time I'm leaning toward creating a patch to add a second column to the user table so we can track both a real timezone and the current offset. Some other things, like incorporating that into format_date() may be too complicated to get done before freeze. But at least creating a method to store accurate timezone data and setting up some basic timezone functionality would make it possible for contrib modules to take it further until we get another chance to make changes to core.

I'm working on a patch. Hope to get it posted soon.

killes@www.drop.org’s picture

KarenS: I think adding a second column to the users table is the way to go, I've chosen this appraoch for my event 5.2 rewrite.

KarenS’s picture

Status:Needs work» Needs review
StatusFileSize
new16.61 KB

In working on this, I thought it would be best to break it down into several patches. I'm attaching the first of them. You can see a discussion of the reasons behind this at http://groups.drupal.org/node/3407

The patches be:

#1) Patch core to rename user->timezone to user->offset and create a new user->timezone that will hold the timezone name; alter system and user timezone forms to accept a timezone name instead of an offset in systems using php 5.2+; add some hooks so other modules can jump in and do things with timezone info, but otherwise make no changes to timezone handling in core. In other words, mostly just create the necessary infrastructure to do more with timezones.

#2) Patch common.inc to restructure format_date so instead of format_date($timestamp, $type = 'medium', $format = '', $timezone = NULL, $langcode = NULL), it becomes format_date($timestamp, $type = 'medium', $params = array()), where an array of params can include things like 'timezone', 'langcode', 'offset', 'format', etc. Most core calls to format_date() set only the timestamp and type, so this would make it easier to add in new arguments to format_date without making very many changes in core.

#3) Once format_date has been restructured, patch it to properly handle either offsets or timezones.

Even if we don't get past the first patch, we'll start to get enough into core that we can do more with contrib modules.

In my patch I am renaming functions so they use the 'date' space, both to make it easier to find and use the date-related functions and as a precursor to moving them into their own include file, as in my proposed patch at http://drupal.org/node/154477.

So we have:

variable_get('date_default_offset'); -- the offset now stored in date_default_timezone
variable_get('date_default_timezone') -- the timezone name;
date_zone_names(); -- the timezone name array returned by DateTimeZone::listIdentifiers()
date_zone_offsets(); -- the array of offsets we now use
date_handle_timezones(); -- true or false, test whether php5 timezone handling is available on this system

we also will have user->offset for the offset value and user->timezone for the timezone name

Note that user->timezone will be empty on systems earlier than php5.2. user->offset may or may not have a correct value in it if user->timezone is being used. I'm updating it when user->timezone gets updated, but not trying to keep it current.

There is also a hook_timezone_alter() to allow you to alter the timezone info when the site timezone is changed, when the user timezone is changed, and to intervene when the list of available timezones is being constructed.

KarenS’s picture

mfb has posted a patch to handle the last two things on the list -- rework format_date() so we can add more parameters to it and add in new php5 timezone processes to the way it handles dates.

KarenS’s picture

Forgot the link to mfb's patch -- http://drupal.org/node/155442.

mfb’s picture

Parts #2 and #3 of this task are patched here: http://drupal.org/node/155442
Testing is needed! I will hopefully have some time for testing soon myself.

mfb’s picture

The functions that I used in my patch say they work with PHP 5 >= 5.1.0

So, shouldn't PHP 5.1.0 work? (aside from whatever bugs and vulnerabilities that version must have ;)

nlindley’s picture

The classes for DateTime and DateTimeZone do exists in PHP 5.1, but it seems like those are not compiled in by default. It might be a good idea to do something like:

return version_compare(PHP_VERSION, '5.1', '>=') && class_exists('DateTime') && class_exists('DateTimeZone');

KarenS’s picture

Some were broken in 5.1 and most everything I've read says don't count on it working correctly until 5.2. Plus there were several functions added between 5.1 and 5.2, can't remember which ones right now, though.

mfb’s picture

Line 251 of this patch should be

+ $zones[$zone] = format_date($timestamp, 'custom', array('format' => variable_get('date_format_long', 'l, F j, Y - H:i') .' O', 'offset' => $zone, 'timezone' => 'UTC'));

mfb’s picture

well ignore the last part of what I said, shouldn't be UTC ...

mfb’s picture

Ok I'm kindof in shock, proper timezone support in drupal core!

KarenS’s picture

proper timezone support in drupal core!

...only if we can get lots of attention on this issue so it gets pushed through. There are *LOTS* of open issues and only a day or two to go until freeze.

BTW, I can't make the change you suggest to my patch because my patch will have to go in before your patch and the new arguments aren't there yet until your patch. Otherwise we have to combine them into a single patch.

Anyway, I think this is a good approach to the problem, and I've spent LOTS and LOTS of time trying to find a nice clean way to get proper timezone support in core, so I feel pretty confident it's a good method. I hope others will dive in and help review and push it forward.

I stand ready to make whatever adjustments are needed to get it in. I'll clean it up, cut it up, stand on my head, whatever :-)

nlindley’s picture

Could we try merging the two patches? Whenever I try patching on my machine, whichever I apply second doesn't completely succeed.

mfb’s picture

StatusFileSize
new31.39 KB

OK here's a merged patch. Test away!

mfb’s picture

StatusFileSize
new31.39 KB

This one has 1 character difference, I didn't fully fix a bug that KarenS found.

nlindley’s picture

OK... Just some preliminary testing here. I did a clean install of drupal, applied the patch, and then ran the upgrade script. I don't know if this is a problem with this patch or something in form.inc, but I got the following error:

notice: Undefined index: id in /var/www/thisoneplace.com/drupal-cvs/drupal/includes/form.inc on line 1920.

mfb’s picture

I saw this warning a couple days ago when working on another patch, so I'd say it's unrelated.

nlindley’s picture

Yeah, I just saw it someplace else, too. Whenever an upgrade is done, no default timezone is selected. I guess this is fine, but should it default to something like "UTC"?

Other than having defaults set on an upgrade, this is working great for me with PHP 5.2.3. I noticed no problems with a clean install, and dates/times are displaying properly. Great work!

mfb’s picture

My vote would be defaults of 0 for date_default_offset and 'UTC' for date_default_timezone, just to make things identical to the old behavior.

KarenS’s picture

I debated the default and took it out to force you to pick a timezone. Perhaps that should be changed, though. It could either by 'UTC' or 'GMT', the difference is where on the list it puts you. 'GMT' puts you in the middle of the list, 'UTC' at the bottom.

Also, I just realized that we need to translate the timezone list. That was discussed earlier and then I forgot and left it out in my patch. We could also use the new $langcode variable, like:

<?php
function date_zone_names($langcode = NULL) {
...
}
?>

which, if I understand it right, would allow you to ask for a timezone list for a specific language.

mfb’s picture

On my system, which doesn't have a local timezone set, PHP defaults to UTC. So that seems like the "real" default..

Yes, format_date() allows for translating timezone names (and abbreviations) if you put them in your custom date format, so the select form should too.

asimmonds’s picture

With a fresh install, if I leave the timezone field select box not set to anything in the installer then php/apache core dumps. As well, if I select the empty timezone option in /admin/settings/date-time, then I get in screen full of notices.

The timezone select box fields should probably be made a required form field.

KarenS’s picture

If empty timezones are generating errors we definitely need to put a default in there and get rid of the blank option.

I tried to apply this patch and couldn't get it to work.

We need to:

1) Insert defaults instead of NULL for all the variables and values.
2) Get rid of the blank option in the array of zone names (which will make it a required field)
3) Add an optional $langcode variable to the zone names function
4) Add translations to the zone names array (probably by doing a foreach over the array to do t($value, array(), $langcode)).

I'll keep trying to get the patch to work or apply it manually, unless someone else gets these things done first...

mfb’s picture

One more thing, we should also set timezone to UTC in format_date() if someone mistakenly passes in an empty or null timezone.

I didn't have a problem applying the patch on a fresh checkout.

mfb’s picture

StatusFileSize
new31.39 KB

This version should resolve the above issues. Phew that is a lot of strings that need to be loaded from the database..

KarenS’s picture

I just sent an email to Gábor to see if he has any suggestions on how to handle translation of that array. It would be nice to cache it by $langcode or something so we don't keep regenerating that process over and over.

Gábor Hojtsy’s picture

Some comments while I am here :)

1. Why did you move $format to the parameters array? It seems to be used a lot in core.
2. There is also a user timezone setting switch (with jS), which could be complementary to provide some better functionality even if PHP 5.2 is not used. http://drupal.org/node/149092
3. Having date_* functions in system module does not work. That module should only have system_* functions. Either move generic date functions to a date.inc, or if you think they are not that generic, rename them to system_*_something.
4. userzone => user_timezone, sitezone => site_timezone (or even swap the words, timezone_site(), timezone_user())
5. Why is there a timezone_alter? I understand that when time is set *back*, that could be a problem, so you see content submitted in the future, you get notifications about it and so on. Is this supposed to be used for that case? How? Something else to use that for? Why should this alter be used when timezones are listed??? Form_alter() should be perfectly fine for that case!

Seems like the performance problems you mention could be in date_zone_names(). It would be nice to see a time zone listing which is handled in that function. From the php.net docs, it seems that it would be a better idea to split by '/' and translate the two parts separately, so there would not be a *huge* list of strings to translate in the database (this could be a real performance hog even if you don't list the timezones on a page, as all the timezone names would end up in the locale cache (being short strings) on all page views).

KarenS’s picture

1. Why did you move $format to the parameters array? It seems to be used a lot in core.

Because the list of possible arguments is getting so long and many of them are not always needed or not in a logical order. There is now a $timezone argument which is really an $offset argument and we also need an argument for the timezone name. Seems easier to use if you provide an array like 'timezone' => mytimezonename, 'offset' => myoffset. This will make it much easier to add other optional arguments in the future, if needed.

2. There is also a user timezone setting switch (with jS), which could be complementary to provide some better functionality even if PHP 5.2 is not used. http://drupal.org/node/149092

Yes, that would be nice but I was trying not to make this patch any larger than it already is. Also, that doesn't seem to work for automatically setting a timezone name, only an offset, which will be of limited use for the php5 stuff.

3. Having date_* functions in system module does not work. That module should only have system_* functions. Either move generic date functions to a date.inc, or if you think they are not that generic, rename them to system_*_something.

I want them in a separate file since there are potentially more date functions coming (see my patch at http://drupal.org/node/154477) I didn't think of doing that already in this patch, but would be easy enough to add.

4. userzone => user_timezone, sitezone => site_timezone (or even swap the words, timezone_site(), timezone_user())

You mean rename the functions?? Not quite sure what you're saying.

5. Why is there a timezone_alter? I understand that when time is set *back*, that could be a problem, so you see content submitted in the future, you get notifications about it and so on. Is this supposed to be used for that case? How? Something else to use that for? Why should this alter be used when timezones are listed??? Form_alter() should be perfectly fine for that case!

timezone_alter is to provide a hook so a contrib module could provide proper timezone support in systems that won't support native timezone handling (anything less than php5.2). It would be way too cumbersome to do that in core, but I wanted to provide ways for a contrib module to jump in and alter the timezone list itself, not just the form.

Seems like the performance problems you mention could be in date_zone_names(). It would be nice to see a time zone listing which is handled in that function. From the php.net docs, it seems that it would be a better idea to split by '/' and translate the two parts separately, so there would not be a *huge* list of strings to translate in the database (this could be a real performance hog even if you don't list the timezones on a page, as all the timezone names would end up in the locale cache (being short strings) on all page views).

Yes, the date_zone_names is where the problems are. I'm not familar enough with the way the translation system works to know exactly where and how we could do things like cache some of this stuff, so we're looking for ideas on how to provide translated timezone lists.

mfb’s picture

Splitting the timezones and translating each part separately wouldn't help, you still have just as many unique city names.

It wouldn't be hard to store the whole translated array in the cache table. I'm just not sure how to deal with the details of expiration, etc.

KarenS’s picture

Another though on bringing the zone names list under control, there are many things on the list that are duplicate ways of creating the same zone. Instead of using the php5 date functions to create the list, we could create our own, shortened list, then translate that. This would also make the list work in php4 (although the native date calculations still won't work there). This could be feasible because that list doesn't really ever change.

For instance, I think all the 'America/...' names are duplicated by country and could be removed (America/New York is the same as US/Eastern). We could also remove the generic zones (+500, etc). Doing that we could maybe cut the list in half (which would also make more user-friendly).

It will still be a long list and we need a way to cache it, if possible.

Gábor Hojtsy’s picture

Karen,

1. It could still be, that we keep often used optional arguments as simple arguments to simplify our lifes.
2. I did not mean folding that into this patch, just pointing out that others are working on making the offset handling better.
3. No way to add date_* functions into system.module
4. Yes, rename, make them obvious.
5. You never even call the function if there is no timezone support (but only offset support), so the alter would have no function there. I don't see the reason to have it in then.

mfb is right, that the city names will still be unique but splitting the time zones for translation could help a great deal for translators to not translate the same string prefixes all times. Like Africa in all the Africa/* timezones. The obvious problem with these short strings is that the locale cache loads them on *all* page loads, so this becomes a serious performance hit on every site using locale and proper timezones.

mfb’s picture

the US/* Canada/* Etc/GMT* and other oddities are old timezones, that are only in the timezone database for backwards compatibility reasons.

These are the currently valid timezones: http://en.wikipedia.org/wiki/List_of_tz_zones_by_name

The America/* timezones are the correct ones to use for the US.

mfb’s picture

Gábor, I just wasn't sure if translators would want to be locked in to the "Continent/City" format, they might want to translate "America/New_York" to "US Eastern".

KarenS’s picture

What about this as a proposal to be able to get this patch in before code freeze:

1) Remove the 'date' functions to their own date.inc file (which is going to become more and more necessary)
2) Remove the timezone_alter() hooks for now (I'm not sure there will be any way to provide a contrib module with timezone support that way, but that may just have to be the way it is)
3) Rename the functions you wanted renamed.
4) Roll back the changes to the way format_date() is structured, but leave the new date handling capability. It will just pick up an additional $offset parameter after $langcode (not my preference to do it that way, but it's not worth holding this patch up).

That still leaves the timezone array to be figured out. If we're going to remove the hooks in the function, I'd prefer to hard-code the array so it will be available on any system. Plus then we can remove the deprecated zones to make the array a bit smaller. Maybe we should even store it in the database so it's not held in memory all the time.

Still not sure what to do about translating it. Even if we cache the translated array, we'll still have all those translatable strings in memory.

KarenS’s picture

StatusFileSize
new33.29 KB

Here is a patch with those changes.

mfb’s picture

KarenS: format_date() is used with the offset in form.inc map_month() function, that instance will need to be tweaked.

mfb’s picture

To avoid code-bloat we could also use PHP's built-in list, but display only those zones beginning with Africa America Antarctica Arctic Asia Atlantic Australia Europe Indian Pacific, and UTC. Thus drupal only needs a much smaller array of continents and oceans.

nlindley’s picture

StatusFileSize
new32.78 KB

This is the same file as post 104, except I took out the carriage returns.

KarenS’s picture

I'm assuming this is not getting in to 6.x, so I'm in the process of trying to add this into the Date API as a way it can be used in both 5.x and 6.x.

@Gábor - I've been thinking about the problem of translating the timezone names (and this would apply to other long lists of infrequently used values like country, county, or city names, for instance), and I thought it might be good to start an issue to discuss how best to handle that in the next version. I opened an issue at http://drupal.org/node/155954 for this purpose.

webchick’s picture

Version:6.x-dev» 7.x-dev

Bumping to 7.x.

nlindley’s picture

Well, it looks like we can count on Drupal 7.x requiring PHP 5.2. I guess that means we don't have to worry about supporting offsets. Although some may want to for old contrib modules, I think it would be a better idea to just switch to true timezones only.

mfb’s picture

This is what I was working on during the code sprint at MIT. The patch we were working on for Drupal 6 can be vastly reduced -- removing all the PHP 4 support. But on the other hand, there is a lot more we can do: For example, add a full date class to Drupal 7, and some schema changes, like moving all integer timestamp fields to GMT native datetime fields (which allows us to store a much wider range of dates, and given that contrib modules use datetime fields, it makes sense to not have a mix of integer and datetime fields).

What I did so far was prepare some benchmarking scripts to compare integer and datetime fields, and I'm going to commit this to my sandbox soon. If anyone else is interested in this, I could use some queries or use cases to benchmark -- please send 'em my way.

P.S. regarding a date class, I was wondering if there are any agreed-upon practices for adding core classes? e.g. should they all be in a "Drupal" namespace.

Crell’s picture

So far there is no standard for class naming other than following PHP standard: ClassName::methodName().

I was doing a little toying with dates recently, and have a DateValue value object class that I can throw your way if you're interested. It's a bit nicer to work with than the built-in DateTime class. KarenS should have a copy as well. I'm in general +1 on using an actual DateTime or wrapper thereof for all of our date/time handling.

mfb’s picture

Sure, I'd be interested in checking it out.
Off-topic from timezones, but still date-related: I did some benchmarking on int vs. datetime fields, running queries on identical generated data. I tested "SELECT...WHERE..." and "SELECT...ORDER BY..." type queries, both using and not using indexes. On mysql 5.0.45, I found that queries on native datetime fields took 1% longer than queries on integer fields. On a untweaked postgresql 8.2 install I found a larger penalty. Note this was just testing raw db performance -- running thousands of non-real-world queries on tables containing nothing but dates, since I didn't yet create an actual drupal patch to benchmark. But still it does seem likely there would be at least some small performance penalty in switching from int to datetime.

hass’s picture

I worked with date and time fields outside of drupal in past and ported MySQL 4.x code to MsSQL2K. This was a very bad experience... an integer is for sure much much easier and should be really faster.

Crell’s picture

moshe weitzman’s picture

KarenS has a patch in the works which adds a date api to drupal. specifically, site and user timezones will be specified by name, not by offset. then the php5.2 date handling emits the date for the user considering his timezone and DST and so on. coming very soon.

KarenS’s picture

There's a huge amount of work that has been done in the 5.2 version of the Date API that I was hoping to present as a core patch very soon. Some of the things included in the Date API are:

1) PHP 5.2 date handling is used, which properly handles timezone conversions, including dst.

2) Site and user timezone settings store timezone names instead of offsets for use in date manipulations.

3) Built in jQuery date and time pickers are included.

4) Automatic, self-validating date form elements can be created by adding '#type' => 'date_select' or '#type' => 'date_popup' and a couple other parameters to forms.

5) Cross-database date SQL for comparing dates and extracting date parts out of timestamps, ISO dates or datetime fields -- created by analyzing MYSQL and POSTGRESQL datetime documentation to return the same results in both. This has been in use in the Date module for over a year now, so has undergone some testing.

6) A fairly complex iCal parser to convert ical files into organized arrays of event information to make it easier to both import and export date data using iCal rules.

7) Functionality to create repeating dates -- both a form element that will allow a user to select repeat rules and a function to take a rule and return an array of the dates that fit that rule. This part might be too much for core and could remain a contrib module.

8) And it all comes with a number of simpletests that test the API, the timezone conversions, the repeating date logic, etc.

In addition to the API, I hoped to convert all core date fields to native datetime fields to make date manipulation work better and get away with the awful cludges you have to do to convert timestamps to something the database can understand as a date. I'd be interested in benchmarking the performance of datetime fields vs integer timestamps, but it seems likely that dates in the native datetime format should perform fairly well and would be *far* easier to manipulate in a database-agnostic way if you need to do things like extract the month from a date (and they also have the advantage of being human-readable).

I proposed a session at DrupalCon to discuss this and it got bumped down to a BOF, which mostly got overlooked, but I've been trying hard to get some discussion going about this topic for the last year or so, both here and on g.d.o.

I did not create this as a date class, but as procedural functions, and I'm not sure if there is any advantage in dumping all that work (and all the testing of that code that has already been done in the last six months or so) and starting over to create this as a class.

mfb’s picture

Re: switch to datetime columns, any ideas on how much of a performance penalty is acceptable? So far in my raw tests of queries on 200,000 rows of dates, queries on datetime dates took 1% longer than queries on int dates. But hopefully there would be smaller impact in the context of a drupal site. I'd like to go ahead and try patching drupal core to use datetime and running benchmarks on things like /node, /tracker, etc. (if there's a decent chance this won't be wasted effort ;)

KarenS’s picture

One thing to consider in the evaluation of performance is what happens on a query like "SELECT MONTH(FROM_UNIXTIME(created))", where created is an integer that has to be cast as a datetime field, vs "SELECT MONTH(created)" where created is a datetime field that doesn't need to be converted. I'm assuming the first will be slower because of the FROM_UNIXTIME conversion. So it's not just simple selects that need to be analyzed, but also selects that are doing date manipulation.

In other words, maybe the cost of FROM_UNIXTIME offsets whatever performance cost there is in the simple selects of datetime fields over integers.

Crell’s picture

If 1% is the only performance cost of using native timestamps, what's the feature benefit? 1% is, I think, easy enough to swallow if there is a compelling reason for it (in terms of capability, cleanliness, stability, etc.) There are other places in core where we should be able to get a performance boost this time around to offset small losses like that, if they're with cause.

mfb’s picture

I didn't see a significant difference between SELECT MONTH(FROM_UNIXTIME(created)) and SELECT MONTH(created) when running these queries 180,000 times. But I also don't think this is a fair benchmark since they're not returning the same results. MONTH(FROM_UNIXTIME(created)) is giving me the month in the database's timezone. MONTH(created) is giving me the month in GMT. In real-life you want neither, you want the month in the user's timezone. This could be done in mysql with CONVERT_TZ() or on the php side.

drewish’s picture

1% over 200,000 queries isn't bad at all if you can store pre-1970 dates...

mfb’s picture

Date module doesn't already have a javascript timezone picker does it? because I figured out a way to make drupal core's timezone picker work with php timezone names. I can get the timezone abbreviation and offset from javascript and pass it in a ajax call to timezone_name_from_abbr(). patch coming soon (cross-browser testing..)

mfb’s picture

StatusFileSize
new11.45 KB
Failed: Failed to apply patch.
[ View ]

Although there's still lots to discuss/work on related to dates, I wanted to upload a patch to get timezones working on Drupal 7. This is just the bare minimum without a new class or datetime schema. I might even suggest to commit this patch and then do more work on another issue.

I tried to get javascript timezone auto-detection working as well as possible. On Windows clients (FF and IE6) it does not seem to detect all timezones, but it works at least most of the time there. Timezone detection should work better on Linux FF (which passes in via ajax a timezone abbreviation that is missing from the Windows platform).

mfb’s picture

StatusFileSize
new11.45 KB
Failed: Failed to apply patch.
[ View ]

Don't think it matters but one of the js variables wasn't declared in the first draft.

KarenS’s picture

Re: javascript detection of the timezone name -- I tried lots of things to get that working and dropped it because browsers are so inconsistent about how they report the timezone that I couldn't get it working. If you can get it working reliably that would be great :)

Re: CONVERT_TZ, it sadly turns out we cannot use it because many hosts do not have the timezone tables available by default. I tried using that in the 5.2 version of the Date module and immediately starting getting numerous reports of people who were on hosts that did not have the timezone info available in the database. Plus I found that there's no comparable function in POSTGRESQL -- well there is a function but it doesn't behave the same way at all. So at the moment I don't think we can do timezone conversion in the database that way and will have to do it by adding and subtracting offsets instead. See http://drupal.org/node/218479 and http://drupal.org/node/220663 for more details of the problems we ran into.

Re: your patch -- one thing you are doing is setting the default value for users to UTC which is a mistake I made early on in the Date module. That makes the timezone wildly wrong if they edit their user settings without actually selecting a timezone. What I did instead was offer an empty value and set that as the default. Then if the timezone name hasn't been set it will be empty, and you'll fall back to the site timezone name instead, a much better solution. You are also using the current user timezone field and I would instead use a new timezone_name field (for both user and site timezones). Anyone using the Date module will already have that set correctly and we can start it out with a NULL value, as noted above, to know when the user has actually set it.

mfb’s picture

Thanks for feedback! I'll roll another version. Re: timezone_name, Yes it should be supported, I'd forgotten it adds a column to user table. I do like to avoid long column names.. for aesthetic reasons I'd just copy data from it but maybe i'm just being nitpicky.

I'd like to say js timezone detection in the patch is "good enough" but it could use testing. It will be impossible to have it working perfectly without hitting limitations of browser/platform inconsistencies and of timezone_name_from_abbr() itself, well unless we want to test every scenario and write a crapload of javascript.

macgirvin’s picture

After reading through all the related issues, it has become apparent that there is no ideal solution short of a full Olsen implementation in core; which would put us out another 4 years. :-(

Here's the option list:

1. - Database support of timezones. As many have noted, it isn't possible to get full support by ISP's, as well as this option is unique to the database backend used.

2. - PHP 5.1.0+ timezone functions. These require a PECL module for updates - which may have ISP support issues, but appears to be supported in PHP core and cross platform. An additional issue is the PHP version requirement.

3. - Native Unix/Linux Olsen databases (/usr/share/zoneinfo). Not useful on Windows; which has its own version of Olsen-like tables, but would require different access mechanisms. For instance the easiest way to set the timezone in Linux is with putenv(TZ=....), which has issues in Windows implementations.

4. - JQuery the client. This has no major server drawbacks, but significant client issues.

Of all of these, number 2 seems that it would have the least amount of headaches; but still may lack universality. For this reason, it would make more sense to implement timezone corrections via plugin. Due the the many modules this would touch, it would require core support. The fallback can always be the offset we have now - but which is incorrect for half the year without manual intervention by every member who has a zone set (and lives in a DST region; a majority of the internet population).

It also seems ridiculous to store date/time as anything but date/time strings (YYYY-MM-DD HH:MM:SS) in UTC. Unix timestamps simply don't have the versatility and range. I had no idea timestamps were (still?) being used in Drupal. As others have pointed out in the related threads, there are a lot of places where performance can be adjusted to offset the sacrifice of having human readable time attached to our data.

The process (as many of you know) is that assuming all times are stored in UTC, there are a few simple functions which are indispensable. Call them whatever you wish, but I'll use these names for description:

enumerate_zones() to show what possibilities are available.
tz_set() to change the session to the user's zone, or the server zone if this cannot be determined.
to_utc() converts from localtime for any data storage (or date-based queries!)
from_utc() converts anything from the database for representation.

Providing math to calculate the offset of two arbitrary zones is largely irrelevant in an application like this, except for the two cases of UTC conversion. There's simply no general need to convert between Australia/Perth and America/Los_Angeles. Yet this is where most designers start.

Now in the Event module, it's not quite so cut and dry. That's outside the scope of this dicussion but suffice to say many people would be alarmed to have Christmas start at 3AM on Dec. 26th. Some events need a special case to denote that they are absolute no matter what timezone applies. A world-wide podcast would not fall in this special case. It needs to be adjusted for localtime or the visitors would tune in at the wrong time.

Anyway, my proposal for a long-term solution is for stub functions for these key functions, with module callbacks so that they can be over-ridden, and with default implementations which use the simple non-adjusted fixed offsets we have now. Then perhaps a few of us can come up with a good plugin or two comprising the most robust solutions available. This takes the pressure off of core to have a solution for everybody.

A solution for everybody is likely to be a bloody mess. But it might not take much more than a few hours to make a robust plugin. I've done the Linux /usr/share/zoneinfo for other community systems and it works well. This would work for anybody on Linux, regardless of PHP version. The PHP timezone functions will work for anybody with newer systems. Folks running on Windows/PHP4 servers would be stuck changing their offset twice a year; but I don't know any other way around that one.

I don't see a bright future in pursuing the MySQL function route, although it's arguably better than the fixed offsets we've got. The JQuery client check likewise is a very clever solution and better than what we've got now. At least if it goes wrong you can blame the client.

Personally I'll take any solution that works. I have Linux Olsen tables, I've got zones in MySQL and I've got PHP5.x. My client time is OK for JQuery, but I can't speak for my 'customers'. But none of these options seem like they're suitable for core, so let's not put them there. Let's just get the stubs/module-callbacks in so that folks who are able to use them can. All we need from core are the stubs, and a check to make sure that the appropriate functions are called in all the proper places.

Apologies for the long post.

mfb’s picture

macgirvin, could you try the latest version of this patch and let us know what you think? We already know that Drupal 7 will require PHP 5.2 (and MySQL 5.0, although not necessarily with timezone support loaded into the MySQL server). Re: the patch, note I do need to reroll it with a few fixes and upload here (soon as I find a moment).

hass’s picture

@mfb: You cannot use MySQL functions for this daylight part. There could be MsSQL, Oracle, Postgre and others... and PDO support came in... maybe this could make things easier in a way i don't know http://drupal.org/node/225450.

mfb’s picture

Although I was using this issue to do some date-related database benchmarking, it's not related to the patch (please take a look and test it), where I'm just using PHP timezone functions. Anything related to database probably belongs in some other issue (whether core or date module) since this one is getting pretty long ;)

macgirvin’s picture

Patch installed (to be fair I manually back ported it to 6.1, because my customers aren't going to wait for 7.x). So far it seems to work as advertised as far as setting zones. I've still got to run it through the paces to see if we actually get the correct time, but it's straightforward. The folks here in Australia spit the dummy (get upset) when you mess up or ignore their timezone, so I'll find out soon enough if it has any problems. We're going through a daylight time change in another week or two and I'll be able to really give it a whirl.

Crell’s picture

PHP 5.2 includes a full Olsen implementation. That's why we wanted to make that the minimum version for Drupal 7. :-)

PDO won't help in the timezone case. The main stumbling block is that MySQL has an optional look-aside table for doing timezone translation, but most hosts don't have it installed so we can't make use of it. PDO is just the interface to the database.

macgirvin’s picture

I've been able to verify this patch using the 2007 timezone data for Australia, which resulted in daylight savings changing this morning.

Note: In fact there were several changes to the daylight savings rules here in Australia this year and many service providers have not yet upgraded. So this is actually checking the old tables which cutover today, using my U.S. hosting provider. This timezone data is incorrect for 2008, however it proves the patch works fine.

I'll have another important data point next Sunday (Saturday back in the states) which is the cutover day under the new Aussie rules, and which will roll over on my Australian test system that has all the current timezone patches (installed in linux, php, and mysql).

c960657’s picture

Re: d7.timezone2.patch
As the default value, date_default_timezone_get() may be a better guess than UTC, at least for people who administer their own server or who use a local ISP (in their local timezone).

macgirvin’s picture

@mfb:

Do you have any more recent rolls on the patch? I notice one was promised a week or two ago.

I've spent the last week or two re-acquainting myself with all the Drupal date/time issues (I had abandoned Drupal a couple of years ago over these and similar bugs). The date/time code is still a mess, as it was back in late '05. It prevents me from recommending Drupal to many of my Australian customers who are quite sensitive about the ability to track this country's frequent DST policy changes. Using some form of updateable timezone tables is a huge step forward. We need to push this harder to get into 7.x though or it will slip out yet another two+ years (Drupal major release + a year for modules to catch up).

FYI: I noticed patching this into 6.x that I was still getting an errant call from somewhere (which I haven't yet tracked, but is on a contrib-free install) using numeric offsets which I hacked a quick fix for thusly:

-  if (!isset($timezones[$timezone])) {
+  if (!isset($timezones[$timezone]) && (! intval($timezone))) {
    $timezones[$timezone] = timezone_open($timezone);
  }

It isn't logically correct, and this isn't the target patch version; but pointed the potential need for some form of exception handling when hit with a numeric offset.

I'm also aghast at the potential performance issues related to the current format_date() function, which can get called a lot of times over the course of a page load. This needs a new issue, to avoid recursing on itself over and over (and ultimately falling into somewhat costly libc functions) for each character of the format string. Granted this is done to allow translation of the string elements returned by date, but is horribly costly. My thoughts are to just call date/gmdate once, with an altered format that specially prefixes the elements which need translating, then translate all the specially prefixed elements on the final result. Thoughts appreciated, but should be directed at a new issue (which hasn't yet been created as I'm writing this).

macgirvin’s picture

I managed to work out something which should be an improvement over the current way that format_date works for the string translations, as I mentioned in #142. I'm going to post the rough code here so that it doesn't get lost on my hard drive. If mfb would like to work this routine into the patch it would be a huge improvement. This is just a proof of concept at this point that will need a bit of work to integrate and review. It does away with a lot of the messiness of format_date. It could be even better if we didn't have the long-month-name to deal with, but we've got the hand we were dealt.

  // Replace entries in $format string which need to be translated with escaped string '+=X=+'
  // where X is a format specifier which requires translation. The 'F' format specifier requires
  // special handling because in English (for example) May is both a short and long month name.
  // We need to specify for the 'F' format that it is the long month name we will be translating.
  // These patterns specifically exclude any pattern specifier which has been prefixed with '\\'
  // (octal 134).

  $format = preg_replace('@(?<!\134)([AaeDlMt])@','+=$1=+',$format);
  $format = preg_replace('@(?<!\134)(F)@','+=!\l\o\n\g-\m\o\n\t\h-\n\a\m\e F=+',$format);

  // Make a single call to gmdate using our specially formatted string.

  $date = gmdate($format,$timestamp);

  // Now look for anything that we escaped, and translate it.

  if(preg_match_all('@\+=([-! a-zA-Z]*)=\+@',$date,$matches)) {
    $arr = $matches[1];
    foreach($arr as $a) {
      if(substr($a,0,16) == '!long-month-name')
        $date = str_replace("+=$a=+",t($a,array('!long-month-name' => ''),$langcode),$date);
      else
        $date = str_replace("+=$a=+",t($a,array(),$langcode),$date);
   }
  }

  // $date now contains the correct translation of the date format.

UPDATE: 06-APR-2008 This issue has now been split off into #243129: Refactor format_date() to avoid multiple calls to date_format() - but as it affects some of the same code as the daylight savings patch, you might wish to subscribe to the issue so as to avoid conflicting patches.

macgirvin’s picture

First Sunday in April 2008. Sydney, Australia - Daylight Savings Time changes. This event usually strikes fear into the heart of any software developer.

I change all the house clocks, the car clock, the sprinkler and heater clocks. Check all the PCs and phones. Most of my systems and web communities changed the time last weekend (wrong - that was the 2007 rule; but today they become right again).

Now it's time to login to every Drupal site I've ever visited and update the time there, because it is the only web service I'm connected to that isn't yet aware of Daylight Savings. But wait, my system has been patched (patch in #131). My development sites actually display the correct time. Yay! It works! That still leaves me with about 80 or 90 Drupal sites to change, but there's light shining through the darkness. Maybe someday this will no longer be necessary.

mfb’s picture

Excellent :)
I'm still workin on this patch but have been busy elsewhere lately. The main thing that needed testing in #131 was the javascript timezone detection which I only tested on a few systems (it is sensitive to platform issues on the client side -- different operating systems and browsers). You can test whether or not it chooses an approximately correct timezone both when installing a new site and when creating a new account (it can't be perfect, it won't know which South American city to choose for example).

mfb’s picture

StatusFileSize
new12.13 KB
Failed: Failed to apply patch.
[ View ]

I rerolled the patch, so it should once again apply to HEAD. I moved the user update function over to user.install, and copy data from timezone_name if the column exists. As someone suggested I used date_default_timezone_get() to guess the timezone during installation. Although, this form has javascript auto-detection so it will mostly come into play for something like an automated script which is installing drupal without javascript.

@KarenS: you wrote "one thing you are doing is setting the default value for users to UTC which is a mistake I made early on in the Date module. That makes the timezone wildly wrong if they edit their user settings without actually selecting a timezone. What I did instead was offer an empty value and set that as the default."

I think I had this correct before, but please clarify if I'm missing something: If user-configurable timezones are enabled I am defaulting users to the site-wide default timezone (both in hidden field at the time of registration, and on their edit page if they don't have a timezone set when editing). This is how it works now and seems (to me) to be the most user-friendly approach for most sites (for example if timezones are suddenly made user-configurable, it seems correct to pre-select the default rather than '' => 'Choose a timezone'). There is also javascript timezone detection during the registration process, so it would only be users who register without javascript who get the site-wide default.

mfb’s picture

StatusFileSize
new12.12 KB
Failed: Failed to apply patch.
[ View ]

Chasing HEAD.

mfb’s picture

StatusFileSize
new12.19 KB
Failed: Failed to apply patch.
[ View ]

Chasing HEAD.

mfb’s picture

StatusFileSize
new12.2 KB
Failed: Failed to apply patch.
[ View ]

Chasing HEAD.

mfb’s picture

StatusFileSize
new12.18 KB
Failed: Failed to apply patch.
[ View ]

Chasing HEAD.

Dries’s picture

Status:Needs review» Needs work

This would be an ideal patch to write test functions for. The tests could forward the time to see if the time adjust accordingly.

The code looks good, only the Javascript code is a bit puzzling. I'd love to see a bit more documentation around that. It's not quite clear what $dst is for. I think it is to indicate that we should use the offsets but I'm not quite sure.

Also, a lot of knowledge is captures in this issue, but very little of that actually flows to the patch. As this is such a hard problem to solve, I recommend that we add a bit more code comments to the patch.

I also think this warrants a CHANGELOG.txt entry.

mfb’s picture

StatusFileSize
new17.68 KB
Failed: Failed to apply patch.
[ View ]

OK I started on some tests. Any suggestions on other tests to add?
I also added some comments to the javascript and in a few other places.

mfb’s picture

Status:Needs work» Needs review
StatusFileSize
new20.92 KB
Failed: Failed to apply patch.
[ View ]

I also added a test for user-configurable time zones.

mfb’s picture

StatusFileSize
new21.96 KB
Failed: Failed to apply patch.
[ View ]

This version adds a CHANGELOG.txt entry.

Susurrus’s picture

@recidive: Stupid copy/paste...

#184733: Use named time zones is what I meant.

Dries’s picture

The code looks good. Thanks for writing tests!

One comment: instead of filling out the admin/settings/date-time page and submitting a form, you should be able to set these variables directly by using $this->drupalVariableSet().

If you can fix that, I'm ready to commit this patch.

As far as additional tests -- it would be great if we could test some of the trickier issues. Writing tests for older timezone related bugs could help. What matters most is that the functionality introduced/fixed by this patch is properly tested.

mfb’s picture

StatusFileSize
new21.48 KB
Failed: Failed to apply patch.
[ View ]

For the record there is no more drupalVariableSet(), it's now just variable_set(). I also had to fix the logic in another user.test, to deal with the fact that the site-wide time zone no longer defaults to 0.

mfb’s picture

I would like to point out that we now have access to the date('T') formatter, which is the time zone abbreviation: EST, MDT, etc. We *could* make use of 'T' in various places, for example the ready-made date format options and the time zone select form. But I have not yet made such aesthetic changes since I was making this patch as conservatively as possible (to get it in quicker ;)

In some cases of course the time zone abbreviation is not actually used in the local area, rather it was arbitrarily assigned by the (US-based) time zone database maintainer.

macgirvin’s picture

I would caution folks about using the date('T') format unless they are pretty certain of a US audience. I've currently got an issue open with a backup software company because they slap 'EST' onto their email log entries. That's correct, we're using EST, but this is *Australian* EST, currently GMT+10. Once it hits the mail system it gets translated to New York time, also known as EST, but GMT-5(?).

Use the long form instead.

mfb’s picture

StatusFileSize
new21.28 KB
Failed: Failed to apply patch.
[ View ]

Chasing HEAD.

mfb’s picture

StatusFileSize
new21.26 KB
Failed: Failed to apply patch.
[ View ]

Chasing HEAD.

Dries’s picture

The code looks good, but I'm not convinced that the tests are complete. I'd like to get your take on this and/or clarify/extend the test case.

There are actually 3 timezones at work here, not? (i) the timezone of the user submitting the content, (ii) the timezone of the server/database, and (iii) the timezone of the user visiting the content? As far as I can see, only (ii) and (iii) are being tested.

mfb’s picture

When you say "(i) the timezone of the user submitting the content" are you referring to the node form's "Authored on" field? I could add tests that the correct default value is displayed and that the posted value is stored correctly.

KarenS’s picture

This looks like a pretty good first step. A couple observations/comments:

1) The attempt to detect the timezone name is nice, but it's hard to tell how well it will work since it will be hard to test. It might help to leave a watchdog entry for each detected date string showing the complete date string and the zone it was interpreted into to have a way to debug and test how accurate this is. That could be removed later, once we are confident it is working reliably, or at least that it is coming up with logical fallbacks when there is not enough info. Can we test the date string interpretation using simpletests? If so, we need a variety of browser examples to test, which could be collected in watchdog entries.

2) I tried to created a javascript timezone detector in the Date module and found some browsers that don't have the timezone abbr in the date string but instead have confusing information like 'GMT-0500' which should not be interpreted as 'GMT', and three letter month and day names (which my regex tried to interpret as timezone abbreviations), so we need to be sure the code can handle those situations correctly.

3) I'd like to see us get rid of interpreting a timestamp in format_date() and use real datetime fields instead, but I guess that is a matter for a different patch.

I'm planning to add the javascript timezone detection and maybe some other parts of this patch to the Date API for 5.x and 6.x which will give it a wider audience to help test it.

KarenS’s picture

Oh, one more thing. We need translations of the timezone names. I've adopted a method that killes created in the Event module of listing all the possible timezones in the install file, just so the translator can find them. Since it's in the install file, it doesn't create much overhead. Would it be a good idea to do this in core? It looks like:

<?php
/**
 * These strings exist only for the extractor to pick them up and make them
 * available for translation. Putting them here keeps them from being parsed
 * on normal pages.
 *
 * Idea borrowed from Event module. Thanks killes!
 *
t('Africa/Algiers')
t('Africa/Asmera')
t('Africa/Bangui')
t('Africa/Blantyre')
t('Africa/Brazzaville')
t('Africa/Bujumbura')
....
?>
KarenS’s picture

A couple more ideas so I don't lose track of them:

1) When we change the site timezone name in the update, we should display a message saying something like, 'The site timezone name has been set to America/Chicago', just in case the wrong timezone got chosen. It's a tricky business trying to figure out what the timezone should be and it will sometimes fail, and a user message will give the admin a way to immediately see that the wrong choice was made.

2) Trying to update the user timezones all at once with no more info than their offset will be tricky and I suspect there will be a lot of cases where it is not correct. We could display a message to the user the first time they login after this change notifying them about what timezone their account has been set to, with a link to the place where they can fix it if it's wrong.

I can roll a patch with these changes if they sound good, but I thought I'd toss them out on the table first to see if there is interest.

mfb’s picture

1) I can add some watchdog() calls in the user_timezone callback. Part of your proposal will require passing the full date string via ajax, whereas I'm now only passing the abbreviation (if any) and the calculated parameters. But this seems worth it for debugging. I can't think of a way to meaningfully test the javascript time zone detection via simpletest since it does involve javascript execution; we'd have to use selenium.

2) I'm only looking for between 3 and 5 letters enclosed in parentheses, which in my testing was always a time zone. I should clarify this in the code comments.

3) I don't think I have the patience to do that in this patch but I'll help with a future patch :)

#170: I can add this, if desired. The timezone database is updated quite frequently, every month or so, so this could quickly fall out of date. PHP 5.2.6 has the most recent updates from March 24th: Saigon changed to Ho_Chi_Minh, Calcutta changed to Kolkata, among others. For anyone working on translations, a lot of the work has already been done; see the Unicode Common Locale Data Repository. And according to http://www.twinsun.com/tz/tz-link.htm "ICU contains a mechanism for using this data" although I'm not sure how one would extract all the time zone names in a particular language.

#171: Sounds good to me. Let me know if you're rolling a patch so we don't cross-post or duplicate efforts..

macgirvin’s picture

2) Trying to update the user timezones all at once with no more info than their offset will be tricky and I suspect there will be a lot of cases where it is not correct. We could display a message to the user the first time they login after this change notifying them about what timezone their account has been set to, with a link to the place where they can fix it if it's wrong.

We may be likely to have less complaints (overall) if we set all the users to the new site timezone and as you say let them know this has been done, or give the admin an upgrade option to let them set a default. At least for regional and local communities/sites the server zone will be right more of the time than not. We could also probe what's set for Date/Event if either of these are installed, but I'm aware of the issues we've been discussing elsewhere which might make this a bit tricky. It would get more things right when one of these is on the system. Global communities not using Date/Event are going to be messy pure and simple. It's basically a wild guess based around an approximate longitude and I figure more than 50% correct will be pure luck.

mfb’s picture

In the current version of the patch, the update script will at least give the user the same offset they previously had configured. To me this is "good enough." Even if it's the wrong geographic time zone, at least they have the correct local time.

Of course a human will do a better job bulk-updating the time zones, and we could even make a fancy UI to do it. But there isn't really any place to put this in the Drupal update scripts. So maybe it could be a contrib module?

Other ideas for related contrib modules would include a map-based time zone picker as seen on most operating systems.

mfb’s picture

StatusFileSize
new491 bytes

I haven't merged this in yet but for future reference, here's a build script to generate the time zone translation list.

mfb’s picture

StatusFileSize
new21.8 KB
Failed: Failed to apply patch.
[ View ]

This version logs a watchdog message each time the javascript tz detection runs. This is basically debug code that could be removed later.

KarenS’s picture

@mfb, if you have time to roll a patch for #171, go ahead and do it. #175 looks like a good method for handling the translations. The translations will need to be updated from time to time any way we do this.

@macgirvin, I agree we should try to capture anything we can from Date/Event because it will be far more accurate than any guessing we could do. The patch already takes advantage of Date timezone info if it exists, we need to update it to detect Event timezone info. If the Event module adds a function to return the site and user timezone names, we can use that, otherwise the event module uses a zid and has a table that tracks which zid is which zone name, so we'd need to check for the zid and query that table, and then do a str_replace(' ', '_', $zone) on the result to replace spaces with underscores.

On #173, I also thought about just defaulting everyone else to the site timezone. Although it will often be wrong, it will often be right and that's going to be the default value anyway. Using any timezone that has the same offset means someone in Brazil could end up with America/New_York, which is wrong enough to create problems.

The one other idea I have for defaults is check for Date/Event info and use it if available, then update to the site timezone if their offset is the same as the site, otherwise set the value to NULL and wait until they log in the first time to update their values. (This means we'll lose the original offset info, for better or worse.) At that point we'll have their browser info to help make an intelligent guess. Then use our javascript detection in the user edit form as well as the registration form to try to default them to the right value.

macgirvin’s picture

@KarenS: Perfect. Given the circumstances I don't think we can do much better. I'm in full agreement that as a last resort, we're better off with NULL and losing data (the old offset) than having the Kurds show up in Baghdad. The js should make everything right for most folks and leave a manageable number of edge cases to mop up afterwards. Since these will mostly be the tier 2 and 3 browsers, for better or worse they're already accustomed to being edge cases.

mfb’s picture

By the way, the only case where javascript time zone detection works much better than the update script is for Mac and Linux users, which pass in a time zone abbreviation. Windows users can still get assigned to another time zone with the same offset, because there's no abbreviation to work with.

mfb’s picture

I'll take a look at Event module. Although it does seem odd to incorporate so much contrib module functionality into core. Couldn't Event module have its own update script to migrate the zid data..

KarenS’s picture

Assigned:KarenS» Unassigned

#179 - the difference between doing this in the update and when the user logs in is that in the update we will just assign a timezone to them, right or wrong, and the user may not know or notice that it is wrong. If we do it when they log in, I'm assuming the method will *not* be to automatically assign anything, but to direct them to update their timezone with a link to where to do it, and then use the javascript to come up with a sane default for the timezone selector. Since they're on the timezone selection form, they can see what is selected and change it right there if it's wrong.

#180 - this is unusual, but this is an unusual case where we have two widely-used contrib modules that have much better information than we can get in any other way. We could expect both Date and Event to do their own updates of this info, but their updates won't happen until after the core updates finish and those modules are re-installed and updated, which is quite a bit later in the update process and it leaves the site and its users without correct timezone information until that gets done, and that is a critical bit of information for the proper operation of the site. Dries may or may not agree with this, but I think this is one of the few legitimate use cases for updating core from contrib info.

Just noticed that I'm still assigned to this issue, but mfb is doing all the work, so I'm unassigning myself :)

mfb’s picture

Assigned:Unassigned» mfb
Status:Needs review» Needs work

OK I was thinking of you as the "supervisor" on this :)
I can work the NULL + javascript method of migration into the next version. I guess I will assume that we always want javascript time zone detection to run if the user's time zone is NULL. I'm not yet sure how the logic to direct them to their edit page will work, perhaps compare timestamps of last login and when the update script ran.

KarenS’s picture

I think all we need is to display a message when the user logs in and the timezone is NULL. The message could contain a link to the user edit page where they can update the timezone or better yet use drupal_goto() to take them straight to the form and show them a message there.

I think my vote is to show them the message one time, the next time they log in after the site is updated. If they don't respond to that message by doing something with the timezone form, silently set it to the same value a new user would get (the site default) and don't show any more messages. That warns them that the timezone has changed, gives them a chance to do something with it if they care, and gives them a sane default value set if they don't.

mfb’s picture

If they don't click "Save" on the user edit page, their time zone will still be NULL. when you say "silently set it to the same value a new user would get" I guess you mean the ajax call should forcibly set the time zone? Maybe I'll figure this out when I actually start working on the patch..

mfb’s picture

Because the user login block passes in a destination, I can't use drupal_goto(). I have to do something like

<?php
 $_REQUEST
['destination'] = ('user/' . $user->uid . '/edit');
?>

Also there are various places where the message & redirect logic could go, e.g. system_user('login') hook already has time zone related stuff.

I'd still need to figure out this part: "If they don't respond to that message by doing something with the timezone form, silently set it to the same value a new user would get (the site default) and don't show any more messages." Currently I'm using a getJSON request, but I shouldn't use that to modify user data, I'd have to use a post and make sure I avoid XSRF.

mfb’s picture

Status:Needs work» Needs review
StatusFileSize
new23.05 KB
Failed: Failed to apply patch.
[ View ]

Do you want to try this out? It displays a message each time a user logs in with a NULL time zone, until they set it. Seems much simpler to implement this way. Also it means a user could ignore it and be reminded again later.

macgirvin’s picture

Dang - this is looking good. I'm seeing the deficiencies of timezone_name_from_abbr() which put my test account in Australia/Melbourne on a Windows box and Australia/ACT (Canberra) on Linux - both Firefox. Abbreviations were AEST and EST appropriately. Still quite impressive even though it was wrong in both cases.

I tested the upgrade path on a pretty much virgin HEAD system with no add on modules and the upgrade worked cleanly.

I'm just wondering about having the autodetect run at rego (oops registration) time vs. just leaving it NULL (or site_default) for new accounts until first login when there's some opportunity to interact and make it right. As it is, I created a new account and the autodetect was applied without user notice (though the event was logged), which means that since it has been set, logging in after registration doesn't tell you that you might want to check it.

If it is set to NULL on registration, first login would then always take you to the profile edit, which is common on many sites and I don't think is bad but some folks might have issue with. Giving new folks the site default wouldn't have this behavior but might be an improvement over starting them off with something that's still a guess, no matter how good it is.

Often the first thing most folks do after they sign up is to visit their profile so I can live with it no matter how it's done. But I can also foresee future issues about new accounts being given something that is quite possibly wrong by default, so this is as good a time as any to figure out the most desirable behavior.

mfb’s picture

I don't feel too strongly either way. We could even make it a configurable option -- silently guess the time zone during registration vs. nag the user.

KarenS’s picture

StatusFileSize
new25.16 KB
Failed: Failed to apply patch.
[ View ]

I've added some more to this and re-rolled it. Here's what I changed:

The original patch checked for empty timezone names in the system module and set a message if it wasn't. I removed that code and moved the test to the user module in the login submit function where I have access to the $form_state so I can alter the redirection.

When an existing user logs in, we check if we have configurable timezones and if the timezone is set. If not, they get re-directed to the user edit page where they see a message that they need to set their timezone. Our existing users will see this until they set their timezones and then no more.

When a new user registers, they get their login info in the mail and they can go to the site and login. There is no timezone set yet, so they immediately get redirected to the user edit page with a message. (This also lets them see the other things on the user edit page that they can set).

There is nothing in the code yet that will default new users to any value, it will be empty, so they will always get sent to the edit page to select a timezone when they first login. If anyone feels strongly that that is not the right procedure, we could add a configuration option about whether new users will get set to the site timezone or asked to set their own timezones.

I also added in some code to try to get timezone info from the Event tables, if available. I don't have Event installed to try that part out, so that needs to be tested, but I'm pretty sure it's going to produce the right results.

macgirvin’s picture

@KarenS - Hate to be the bearer of bad news, but #189 patch didn't work as advertised. I had exactly the same results as #186 (autodetected timezone on new registration and prior to login that was wrong [although close, same results] both times). The only difference on my end between this and the last report (#187) is that this was a fresh install on HEAD so I did not check any upgrade paths - although I should note that user/1 was redirected to set the timezone as soon as the installation completed.

New users still get the autodetect which is set during the registration process (I checked the DB before attempting a login with the new account) - so in fact it isn't empty when they login the first time as you described.

[I'm also not receiving any registration emails and I've tried a few different addrs. I thought I should flag it in case it is indicative of a regression either in #186,#189, or current HEAD. Haven't had time to fully investigate this yet.]

mfb’s picture

heh it did take some working around re: login destination ;) I'll look at this some more in the morning..

KarenS’s picture

Ah! One problem is that I left some garbage in the patch from when I was playing around with different options. I'll roll another and post it shortly.

Not receiving mail is odd. There is nothing here that should interfere with email.

Also, macgirvin, you use Event, don't you, can you test the new code there to be sure it picks up your Event timezone info?

KarenS’s picture

StatusFileSize
new25.89 KB
Failed: Failed to apply patch.
[ View ]

Here's another stab at this. The more I thought about the auto redirect the more I got worried that it may have unintended consequences for things like remote logins, so I went back to the original method of just displaying a message to the user and not trying to do any redirection.

We have some timezone handling in the user module and some in the system module, and there's no real consistency (this is a problem in the original code, not this patch). For instance timezone info is added to the user registration form by the user module but added to the user edit form by the system module, and the system module, which defines what the timezones are, was using user javascript to detect the system timezone.

It looks to me like the system module is the logical 'owner' of timezone info, so I consolidated all timezone handling there. I also added an option in the date-time admin form to set a default time zone value for new registrations to be the site timezone, empty, or a user selection, then altered the registration form to match that choice.

I also did a bit of cleanup on the messages which were made up of a bunch of fragments that might be hard to translate and reworked them into complete sentences.

I have not tested the upgrade path or the attempt to get timezone info from the Event module.

Jaza’s picture

I have several suggestions regarding the update process for an individual user's timezone setting:

  1. With the patch in #189, am I right in saying that unless the upgrade script manages to retrieve reliable timezone information from the Date or Event modules, then it will set the user's timezone to NULL? Please confirm that this is the case. All users should be prompted to verify their timezone after logging in, whether or not the auto-detect JS succeeds in guessing it. And we should not actually be saving the suggestion that the JS provides, we should only be setting it to the default form selection value. There's no point in more than a mere suggestion, for data as unreliable as that.
  2. Personally, my vote is for the timezone detection JS to ignore everything except for proper timezone information, in the form 'Country/Region' or 'Country/City'. The abbreviations are extremely unreliable (as has been pointed out by other people, with the example of 'EST' meaning both US and Australian 'Eastern Standard Time', among others). Offset information would be better off ignored, as each offset corresponds to numerous actual time zones, and what's the chance that we'll randomly guess the right one for a given user? Even with DST info tacked on (which is itself also an unpredictable guess), the odds are against us. If we're ignoring the user's existing time offset setting, then it seems silly to then go and use the offset that their browser client has provided. What I'm saying is, we should either (a) guess the user's timezone if we've managed to get very reliable-looking client-side data, or (b) not guess it at all. As the saying goes, GIGO ('Garbage In, Garbage Out'). And GIGO is not the Drupal way.
  3. Also, the watchdog message being logged in user_timezone() should be recording the user to whom it applies.
Jaza’s picture

About the issue of translating the timezones. The build script posted in #175 looks great. However, we'll have to document that the static list of translatable timezones should only be rebuilt for each major Drupal release, and not at any other time. Translatable strings should not be changed after a string freeze, nor once they're in a stable branch. We can't have translatable strings that are getting pumped straight into Drupal's codebase from a third party (i.e. from the PHP library), without a mechanism for controlling exactly when that pumping does and does not happen.

So, for example: if we declare a string freeze for Drupal 7 in (say) 6 months' time, and the PHP team decide to update their timezone list in 7 months' time, then the updated list will just have to wait until Drupal 8.

KarenS’s picture

StatusFileSize
new36.71 KB
Failed: Failed to apply patch.
[ View ]

#194 - yes the latest patch will only use the js detection on a timezone selection form to help make an intelligent guess for a default value, it will not update that value anywhere else. The current process is to try to get the user timezone from Date/Event, otherwise set it to NULL.

We have to use the timezone abbreviation for the detection because 1) as far as I know, no browser puts the timezone name you're looking for into its date string -- it will either be the abbreviation or something like 'Central Daylight Savings Time', and 2) the abbreviation is what the timezone_name_from_abbr() function uses and there is no other way in PHP 5.2 to search for a match. This is good enough to set a default value, but as we said above, not good enough to use as an automatic selection.

#195 - good point, so we might as well put the hard-coded values in there instead of using the function so we can see what they are. I've updated the patch to add that in, along with a TODO to confirm those values right before string freeze.

I also added in an option whether or not you want users with empty timezones to be reminded on login. It will default to turn that option on in the update if we set user time zones to NULL, but this way the admin can control that behavior. And I added a message to the update to tell the admin that there are now new timezone options they can set.

Here's another patch, with those changes and a couple small bug-fixes.

mfb’s picture

Status:Needs review» Needs work

The idea of the build script was to use it to regenerate the list of time zones in future drupal releases. It could live in the scripts directory or we could hunt it down here.

One problem I found so far: In your Event module update script, the name column is ambiguous, you need to specify event_timezones name.

mfb’s picture

Also you used the t() function in the update script. I didn't use it because none of the other update scripts seem to use it when calling drupal_set_message(). I'm not sure what the deal is, as I didn't manage to find any documentation on using the t() function in hook_update_N().

mfb’s picture

I tried out the patch and it looks like system.js is not loaded on the user/edit page.

hass’s picture

t() in update makes no sense, while the po strings are imported afterwards.

mfb’s picture

Status:Needs work» Needs review
StatusFileSize
new38.01 KB
Failed: Failed to apply patch.
[ View ]

I resolved the above issues and made some other tweaks including: time zone strings in system.install were apparently not from the latest version of PHP so they were already out of date; on installation admins were presented with a nice "Please set your user account time zone" so the empty time zone message now defaults to off; date_timezone() renamed to system_timezone().

I wouldn't say I am completely happy with the new setup; on my sites I would still like to convert user's time zones on upgrade and use js time zone detection on registration, rather than forcing users to set their time zones in both cases. But I won't argue on these minor points if others think it's working well.

mfb’s picture

StatusFileSize
new38.08 KB
Failed: Failed to apply patch.
[ View ]

Chasing head.

catch’s picture

Still applies with offset. All tests pass.

I noticed a slight delay when viewing the timezone on user/1/edit - it showed Africa/Abidjan initially, then switched to London very quickly (yay!). Was this because it was the first time visiting my account settings? If so it might be nice to add this to the user/1 settings on site installation.

It's a shame about the huge list of country strings, but iirc (without reading through the 200 previous posts), there's no way around this right?

Other than that, the patch looks good to me, although no time for an in-depth review.

mfb’s picture

@catch: this is how it started working in recent versions of the patch. In previous versions there was no javascript timezone detection on the user-edit page, only the user-register and user-create pages. I don't mind the "old" way at all. I am basically going with whatever folks here suggest for these "UI details" so as to get the guts of patch committed.

It would be possible to filter some more obsolete timezones from the list, if we want to maintain a list of obsolete timezones in the code. For example, in recent versions of the timezone database, Asia/Kolkata replaced Asia/Calcutta. PHP timezone_identifiers_list() lists all the timezones, old and new, for backwards compatibility. My thought was this backwards compatibility will also be useful for drupal sites, so the selection shows up if a user had previously specified "Asia/Calcutta" according to the old database.

Freso’s picture

I don't think "old" timezones are needed in Drupal. Drupal 7 will (hopefully :)) be the first release with proper timezones in it, so it won't need to be aware of timezones predating its release date. (I'd also like to think that Drupal 8 and onwards would be able to omit timezones obsoleted by their release, and have the update script change the obsoleted time zones to their proper, current (at the time) ones.)

But perhaps it would be a good thing if it was possible to extend the list of timezones through a contrib module or otherwise? That way, if someone really needs the obsolete timezones, they can still add them. And likewise, if someone needs a timezone that's being introduced after, e.g., the release of Drupal 7 but before Drupal 8, they can still include it on their site. (Not that this necessarily has to be part of this patch, just something that might be nice to keep in mind. :))

catch’s picture

I had to register on a vbulletin board the other day, and it had the normal timezone options, then 'automatically detect daylight saving' as a checkbox. That'd be a nicer UI IMO, but no reason to hold this patch up with it - since it'd be either an easy followup patch or contrib option.

mfb’s picture

@Freso: In that case this patch also has a reason to update timezones, as it migrates timezones from existing installations of the date or event modules. There are 107 obsolete timezones in the tzdata "backward" file which could be used to create an update script.

Excluding old timezones would require adding an array of ~46 obsolete timezones to this patch. With this additional filtering, _system_zonelist() would currently list 401 timezones instead of 447.

mfb’s picture

@catch: I'm not sure I understand what the vbulletin UI is doing. If a user is selecting their daylight saving status rather than the site figuring it out for them, that would imply that we don't really know their timezone. The goal here is for details like daylight saving time to be taken care of automatically by feeding the user's geographic timezone to PHP.

As far as UI the main improvement I could think of would be just to copy the clickable map used by most operating systems.

mfb’s picture

About translation of timezone names... I finally downloaded the XML files from unicode.org (the Unicode Common Locale Data Repository that I mentioned earlier in this thread) to take a look at how it works.

In an XML file for each language, e.g. es.xml, each timezone is mapped to a translated form of the exemplar city, e.g. <zone type="Africa/Johannesburg"><exemplarCity>Johannesburgo</exemplarCity></zone>

There is also supplemental data that maps each timezone to a metazone, for example America/New_York and America/Toronto map to "America_Eastern". And these metazones are also translated in the language files, e.g. in es.xml as "Hora oriental". However metazone mappings change over time, e.g. <timezone type="America/Santo_Domingo"><usesMetazone to="1974-10-27 05:00" mzone="Dominican"/><usesMetazone to="2000-10-29 06:00" from="1974-10-27 05:00" mzone="Atlantic"/><usesMetazone to="2000-12-03 06:00" from="2000-10-29 06:00" mzone="America_Eastern"/><usesMetazone from="2000-12-03 06:00" mzone="Atlantic"/></timezone> so these aren't such a good idea to use.

lilou’s picture

Status:Needs review» Needs work

Patch cannot be applied to HEAD.

mfb’s picture

Status:Needs work» Needs review
StatusFileSize
new38.07 KB
Failed: Failed to apply patch.
[ View ]

Chasing HEAD.

Btw would it be possible for testing.drupal.org to automatically inform me when patches no longer apply..?

lilou’s picture

@mfb : http://drupal.org/project/issues/subscribe-mail/drupal : "Receive notification of failed patch tests."

... but last patch of active issue need to be test frequently.

mfb’s picture

@lilou I've never received a failed patch test. Maybe it only tests once as soon as a patch is uploaded.

catch’s picture

mfb: it doesn't quite work yet - http://groups.drupal.org/node/13990

mfb’s picture

StatusFileSize
new37.97 KB
Failed: Failed to apply patch.
[ View ]

Chasing HEAD.

mfb’s picture

Version:5.2» 7.x-dev
Assigned:KarenS» mfb
StatusFileSize
new37.99 KB
Failed: Failed to apply patch.
[ View ]

Chasing HEAD.

mfb’s picture

StatusFileSize
new38.16 KB
Failed: Failed to apply patch.
[ View ]

Chasing HEAD.

mfb’s picture

StatusFileSize
new38.15 KB
Failed: Failed to apply patch.
[ View ]

Chasing HEAD (you people and your time() REQUEST_TIME craziness ;)

lilou’s picture

User time zone tests : 26 passes, 0 fails, 0 exceptions

maartenvg’s picture

Status:Needs review» Needs work

Patch applies but update user_update_7002() can not be run. I get
An error occurred. http://www.example.nl/update.php?id=2&op=do <br /> <b>Fatal error</b>: Call to undefined method DatabaseSchema_mysql::escapeTable() in <b>/var/www/home/d7/includes/database/mysql/schema.inc</b> on line <b>22</b><br />
This was on a clean HEAD install.

Besides that, this module should use the new DBTNG database calls: see http://drupal.org/node/224333#dbtng

mfb’s picture

Status:Needs work» Needs review

Actually that's a bug in the db layer, I filed a new issue here: #310607: Fatal error: undefined method DatabaseSchema_mysql::escapeTable()

I agree that the database calls in the .install files should get an overhaul (e.g. using foreach to iterate over the results) but I *think* this is at least working and it would be good to get further reviews. I will revisit when the above issue is fixed.

mfb’s picture

StatusFileSize
new38.16 KB
Failed: Failed to apply patch.
[ View ]

Chasing HEAD, and a couple tweaks related to new db layer.

webchick’s picture

I'm going to chime in here and say I'd really like to see this in D7.

I'm also going to say that unfortunately I don't have time to review it now, but it'd be great to get some eyes on this so that when I do have time (probably next week) it was all nice and nitpicked and ready to commit. ;)

KarenS’s picture

I would like to test this, but can't get to it immediately. Marking it so I remember to get back to it :)

mfb’s picture

StatusFileSize
new38.25 KB
Failed: Failed to apply patch.
[ View ]

Chasing HEAD -- revamp system_user_*() hooks.

keith.smith’s picture

Incredibly minor, but I notice that most of the code comments refer to "time zone" and "time zones", while some refer to "timezone" and "timezones".

mfb’s picture

StatusFileSize
new39.66 KB
Failed: Failed to apply patch.
[ View ]

OK I changed ~6 cases to "time zone", let me know if I missed any.

drewish’s picture

we should put this in the patch spotlight: http://drupal.org/node/132970

it'll need a description written up and a list of things to test.

mfb’s picture

Here's a super-abbreviated start. If I or someone has more time it would be possible to come up with a very long list of detailed test scenarios.

#11077: Introduce Daylight Saving Time for Drupal revamps Drupal's support for system default and user-configured time zones to use PHP 5's time zone subsystem. With this patch, time zones will be identified by geographic names such as "Europe/London" rather than numeric offsets from GMT. This is the same method that virtually all operating systems use to identify time zones and perform date/time calculations involving non-GMT time. The offsets that Drupal has used in the past are wrong literally half the time due to daylight saving (summer) time and have to be manually updated like clocks -- but even when set correctly, times cannot be calculated correctly six months in the past or future.

For more on date/time in PHP 5, see http://www.php.net/manual/en/book.datetime.php

The current version of the patch includes some changes and additions beyond the time zone revamp (see the issue comments for discussion on these items):

  • Some time zone-related code is moved from user module to system module;
  • The user time zone can now be set automatically via javascript on the registration page or the user edit page, and can now appear as a visible field on the registration page so that it can be manually verified (due to difficulty in 100% accurate detection of the user's time zone in some environments like Internet Explorer);
  • The automatic javascript time zone detection is logged for debugging purposes;
  • Time zones configured by the Date or Event modules on Drupal 6 are automatically migrated to Drupal 7 by update scripts in system.install and user.install;
  • All user-configured time zone settings will be wiped out by the update script and will have to be reconfigured by users, because it is impossible to migrate time zone offsets to time zone names accurately (dozens of time zones may share the same offset at various times of year);
  • Time zone-related tests have been added to system.test and user.test; and
  • A list of time zone names is included in system.install for purposes of seeding the translatable string database.

Things to review:

  • Upgrade from Drupal 6 to Drupal 7: Check the messages displayed during the update script, test the attempt to migrate the system default time zone, and review the potential disruption of user time zones being wiped out;
  • Upgrade from Drupal 6 to Drupal 7 with Date or Event modules installed: Test the migration of user-configured time zones;
  • Test all the combinations of time zone configurations at admin/settings/date-time;
  • Test functionality of format_date(), i.e. all display of dates and times;
  • Test user time zone functionality at registration, user login and user edit;
  • Review the removal of the option of having both a time zone hidden field and automatic time zone detection at registration (with this patch it is possible to have EITHER a hidden time zone field OR automatic javascript time zone detection but NOT BOTH -- some may want to restore this option);
  • Review the new time zone-related System and User tests, and suggest any ideas for additional tests;
  • Review the UI for configuring time zones, including a very long select form, and consider potential for improving the UI and/or ensuring that add-on modules are able to improve the UI (e.g., would it be useful to be able to drupal_alter() the list of time zones); and
  • Review feasibility of translating time zone identifiers, ideally using pre-existing translations in the Unicode Common Locale Data Repository.

[Edited to clarify the present inability to have BOTH a *hidden* time zone field and automatic javascript time zone detection on the register page -- admins have to choose one or the other].

mfb’s picture

I just looked at admin/settings/date-time and I'm not sure the fieldsets (Locale and Formatting) are totally logical. Maybe the time zone options should go in to a separate "User-configurable time zones" fieldset. The difference between "locale" and "formatting" is not entirely clear to me either.

mfb’s picture

StatusFileSize
new41.53 KB
Failed: Failed to apply patch.
[ View ]

Chasing HEAD and fix a regression in #226: some code needs to be shared between system_user_form() and system_user_register(). Per above I added an extra fieldset for "User-configurable time zones".

webchick’s picture

@mfb: Just so you're aware, we're in #drupal prepping for UNSTABLE-2 which should be tagged either later tonight or tomorrow morning so patches are getting committed every few mins. So I'd hold off doing more re-rolling until tomorrow for your own sanity. :)

mfb’s picture

StatusFileSize
new42.15 KB
Failed: Failed to apply patch.
[ View ]

Chasing HEAD, and finally fixed a bug I reported way back in #105.

jjkd’s picture

Installed a clean version of Drupal 7 from HEAD.
Patch still applies against HEAD, with a few offsets.
The need for a schema update was properly detected, and two were applied successfully.
My local timezone was correctly identified as the suggested value for the system default time zone.
The reminder prompt for a user without a confirmed time zone setting to set their own time zone worked.
The user time zone detection in the self-update context worked.
The options around user timezone handling for new registrations worked as described.

One issue I noticed is that when an admin views a new user registration that has an empty timezone assignment, the timezone is not shown as 'not selected' but instead shows as (I believe) the detected timezone of the admin. So, the person administering the new user can't distinguish between users with no time zone and users whose time zone is set to their own time zone. I believe that means changing anything else on the user and saving the result will silently change the user's time zone from none to the admin's.

I think it would be preferable if the admin could distinguish between a user whose time zone is set from one whose time zone is not set, and default to admin edits not changing a null time zone.
--
Joe Kyle
--jjkd--

mfb’s picture

Thanks for catching that. so we need to allow the form to preserve a NULL timezone, and only do javascript time zone detection when the user is editing her/his own account. I think it would be ok to add an empty-string option to the form, allowing also non-admin users to set their time zone to undefined.

mfb’s picture

StatusFileSize
new42.4 KB
Failed: Failed to apply patch.
[ View ]

OK to address #244, I added a $blank param to _system_zonelist(), which prepends a "None selected" option to the zonelist. One other thing I noticed, since javascript auto-detection was removed from the hidden timezone field, I changed this hidden field to use #value rather than #default_value.

jjkd’s picture

Ok, went through the process again, editing a user from the admin context is much improved.

The only remaining wrinkle in this category I noticed is when a user is editing their own time zone, and selects the 'none' option, auto-detection runs again when the page reloads, and so the 'none' option is not shown as the user's current setting. The value in the db is correctly empty at this point.
--
Joe Kyle
--jjkd--

mfb’s picture

I wouldn't consider this a problem. Do we need to make it easy for users to maintain an empty time zone setting? Previously I don't think Drupal has allowed users to set an empty time zone. And we would have to differentiate somehow between a missing time zone setting and an intentionally empty time zone setting.

jjkd’s picture

Yes, I think you are right. I agree, allowing the user to change to an empty time zone setting is probably not desireable. So, if we can eliminate the "none selected" option only in the self-edit context, I think that would help.

In working through this in my head, I think what was going on there may be a consideration here for a potential usability issue. It appears to me that we are 'overloading' the time zone field, and really using it for two different purposes when displaying a value:

1) If the user's time zone is set in the database, display the current database setting
2) If the user's time zone is not set in the database, display the results of the time zone detect code

In the case of #1, the user can safely hit either 'save' or navigate away from the page, and whatever they saw as their 'current' time zone setting is retained in either case. If they change the time zone value, the need to hit 'save' is a reasonable expectation.

In the case of #2, navigating away from the page will discard the time zone value displayed, and the empty database value will remain. There really isn't an indication to the user that the value shown is a 'recommended setting', and they need to hit 'save' to retain it. The user may interpret the display as their 'current' time zone setting, and not realize that it must be saved at that point.

An example use case would be: System is configured to create new users with empty time zone values, and to prompt users for time zone entry. User logs in, sees the prompt, clicks it to go (or otherwise eventually navigates) to their user profile. They see a time zone displayed, and thinks their time zone is set, so they navigate away, This repeats each time they log in, and they get the impression that this is something unreliable in the system. Eventually change something else in their profile, and now the time zone setting does persist.

I would suggest that in the case of #2, we provide some indication to the user that the value displayed is a recommended time zone value, and they need to save it.
--
Joe Kyle
--jjkd--

alexanderpas’s picture

solution for option #2:
drupal_set_message('Timezone will be automatically detected if possible, please edit it if found incorrect, hit save button to confirm selection.');

mfb’s picture

There's a problem with using drupal_set_message(): apparently the system_user_form() hook fires when the form is posted, but the submitted form values are not passed thru in $edit. So, after a user sets a time zone successfully, she/he will see a message that was set during form submission (not displayed at that time because it was a POST request) about needing to set a time zone.

Maybe the only way I can do this cleanly in system_user_form() is to render the message in the form itself (like in the field description).

mfb’s picture

StatusFileSize
new42.58 KB
Failed: Failed to apply patch.
[ View ]

OK to address #249/#250 I added a message in the description of the timezone fieldset.

jjkd’s picture

Great, thanks, I think that pretty much covers #249/#250.

The only other thing I've noted is that the field description rendered in both the admin and self-edit contexts is the same, and is currently:

"Select your current local time. Dates and times throughout this site will be displayed using this time zone."

I would suggest that there be different versions for the self-edit and admin contexts, perhaps:

"Select your current local time. Dates and times throughout this site will be displayed to you using this time zone."

and

"Select the desired time zone for this user. Dates and times throughout this site will be displayed to the user using this time zone."

mfb’s picture

Has anyone complained about this issue before? Because it's been like this for ages, and I don't think this is the only place in core where site admins see a "you" that actually refers to the user in question, see also: "A valid e-mail address. All e-mails from the system will be sent to this address. The e-mail address is not made public and will only be used if you wish to receive a new password or wish to receive certain news or notifications by e-mail." If this is in fact considered a usability problem then I would suggest to change it to some neutral wording rather than adding extra strings.

jjkd’s picture

Sorry, if that's the norm, I would agree that it isn't the responsibility of this patch to change that 'standard'. We can probably leave it to those concentrating on usability to decide if a core-wide issue should be opened on that.

How about the fairly neutral:

"Select the desired local time and time zone. Dates and times throughout this site will be displayed using this time zone."

alexanderpas’s picture

+1 to neutral statement!

jjkd’s picture

StatusFileSize
new42.59 KB
Failed: Failed to apply patch.
[ View ]

Here is a version of patch 29 with the field description changed to the more neutral version. Full credit to Mark, all I did was change the one line.

alexanderpas’s picture

Shaman: since we upped the requirements for D7 to PHP5.2 we now have the tools to pull this off.

jjkd’s picture

Ok, looking back at #230 to see what else might need some attention, the last bullet caught my eye. I did some exploring over at Unicode Common Locale Data Repository site regarding existing translations of time zone names, and based on how they structure their data, I saw no easy way to incorporate what they have there into what we do here. I did find what seems to be a reasonably well maintained source of PO files here:

http://translate.fedoraproject.org/module/system-config-date

1) Would this be an acceptable secondary source?
2) Regardless of source, do we want to include this work in this current patch effort, or should this be deferred?

mfb’s picture

Thanks for researching this. So the translations already exist, example link: http://translate.fedoraproject.org/POT/system-config-date.tip/timezones.... Hopefully it's easy for translators to import these existing translations when they are creating a new translation? Is there anything else this patch would need to do?

It seems we still need to hackishly include the time zones in a code comment since drupal does not ship .pot files?

jjkd’s picture

While I don't think I know enough about translation workflow to be sure, I think that the process (or a variation on the process) described here:

http://drupal.org/node/11311

would be able to do the job. I believe that you are correct, we will need to include the time zone strings (and as you note, update them just prior to string freeze) so that they can be picked up by the .pot file generation process. Hopefully someone who is more involved with translation can confirm. A documentation update to cover this would probably be a good idea.

alexanderpas’s picture

AFAIK, as long as we have the untranslated strings in before string freeze, ( t('Europe/Amsterdam') for example) the translations can be added later.

also, drupal core should not carry the translations, just the translatable strings (however, we should point translators to the already availble translations of those strings.)

hgmichna’s picture

Component:usability» base system

How does this relate to the fact that Drupal 6 already detects the user's time zone automatically when the user registers?

If Drupal already has this ability, why can it not also be used when the user travels or when daylight saving time begins and ends?

The Auto Time Zone module did all that, but only for Drupal before 6. If Auto Time Zone could do it so easily, why do you have to teach Drupal any time zone knowledge? What's so difficult about reading the user's time, comparing it to the server's time and setting the time difference accordingly?

What's happening here? Things don't seem to fall in place, although everything is already there somewhere.

Hans-Georg

mfb’s picture

StatusFileSize
new42.47 KB
Failed: Failed to apply patch.
[ View ]

chasing HEAD.

catch’s picture

Component:base system» usability

This looks good to me. It's a shame about the strings in system.install but I don't have an alternative either. Tried the timezone options and it worked fine (if testing the patch, don't forget to run update.php). I'm a little unsure we need quite as much granularity of options on the date/timezone settings page - seems like we could just set default timezone, whether users are allowed to set their own timezones, whether this happens at registration - and be done with it. Incredibly minor but there's some trailing whitespace on several lines in the patch.

So the only questions remaining for me are - 1. is there a way we could load those strings without specifying them all in system.install, or somehow offload the responsibility for that from core? And what happens if there are daylight saving changes during the 2 years or so a release will be supported? 2. Do we need all those options exposed on date time settings.

Since the timezone offsets are a real pain as they stand, I'm moving this to the usability component, and hopefully someone from the UX team can weigh in here as well.

Tests all pass.

jjkd’s picture

Component:base system» usability

@hgmichna: To recap the prior discussion in this issue, the situation being addressed here is that In prior implementations, ala Drupal 6, the time zone offset was stored as a static numeric value that was added or subtracted from the time in question. That cannot properly deal with Daylight Saving Time changes, as the offset from GMT is constantly changing, with both "when" and "how much" dependent upon the user's location, plus being dependent upon the exact value of the timestamp itself (which will determine if DST applies or not in that location for the referenced date/time). I don't think there's any question that effectively addressing this will require the implementation of this kind of mechanism, the questions are about how many options and how much UI to implement, along with how well migration works.

@catch: Re: your point 1, isn't this a general issue with any part of Drupal that needs to allow for the translation of strings that aren't 'there'? I agree that there should probably be a better way of handling this, but is that something that should be fixed as part of this patch?

Re: DST changes, there are three things that can change:

  1. the list of locations
  2. translations for the list of locations
  3. the time zone rules/database

The first and second items don't change that often, but yes, they will need to be maintained. The third item changes as well, and a bit more often (I believe eight so far this year). This is mostly up to PHP, and there's a PECL update process for this, but there's a downstream impact on the 'translation' table here used for migration. I believe that announcement list(s) are available to track changes to this data, with the primary source being the tz mailing list (see: http://www.twinsun.com/tz/tz-link.htm).

Re: your point 2, I think the options do a good job of allowing someone to determine how they want their Drual site to behave, and are based on real-world workflow. Admins creating accounts may not know the preferred time zone of the user they are creating. For self-created accounts, removing the option to allow the time zone to be selected later might be ok, but I'm not sure how much that saves us.

hgmichna’s picture

jjkd, thanks for the comprehensive info!

Why can Drupal not work like the erstwhile Auto Time Zone module that existed for Drupal 5? I believe the module simply compared the user's time with the server's time and set the user's time difference accordingly. It did that often, so a travelling user wouldn't have to worry.

This sounds like a very much simpler and fully automatic solution.

jjkd’s picture

Yes, you are overlooking something. Please read the entire thread here, please pay special attention to those leading up to #64 and the link there to: http://groups.drupal.org/node/3407, read that entire thread as well, along with this one: http://groups.drupal.org/node/4979. Then come back here.

The issue here is not to merely display timestamps on nodes and comments, that is not 'all the user wants' --- Indeed, that is probably the least interesting application of this functionality. Start thinking about events, think about any arbitrary node type that needs to use a date/time value that represents a date/time that occurs in the future. Think about CCK and dates, the Event module, Views, Calendar, ...

The issue is to have a date/time handling system in core that correctly handles dates and times so as to allow any date/time value to be correctly displayed in the user's time zone. The calculation required to do that needs to be able to calculate the offset between the user's time zone at the user's location and GMT, specifically the offset in effect at the time value to be represented, not using the offset that happened to be in effect when the value was stored, nor using the offset that happened to be in effect when the value is/was displayed, nor using a static value for the offset. For example, we switched from DST to 'normal' time here at 2 am this morning. Using your proposal, if yesterday, I had created a node with a date/time value pointing to today at 1 PM, the display of that date/time would now be wrong when displayed today, by one hour. If someone was supposed to meet me somewhere, either physically or online, that meeting wouldn't have been successful.

That is the problem to be solved. This is an issue that programmers have been struggling with since the first program that stored and retrieved a date/time. There is one clean and simple solution to implement: do everything and display everything in GMT, and have the user do the conversion. The other, more complex but generally accepted solution is the 'libc' or 'tz/Gordon' solution, which is what is proposed here (as it is included in PHP5). As far as I know, every system that handles date/time values correctly is either based directly on this solution, is derived from the 'tz/Gordon' solution, or has been re-implemented from the same principles as this solution. I don't believe that there is any simpler solution that meets the requirement listed previously. If there were, everyone would be using it instead of the 'tz/Gordon' solution.

The larger decision here is 'do we implement a version of this solution or do nothing?' If we don't implement a solution based on 'tz/Gordon' we might as well do nothing, as it would not be fundamentally any more correct or reliable than the status quo. There would be little value in implementing a lesser solution that doesn't solve the problem. Better to put that effort into other parts of D7.

If we are to implement a version of this solution, the call to action is to focus on the specific points raised in messages #230 and #267.

sun’s picture

Please STOP derailing this issue. If you are no date/time as well as PHP expert, do NOT follow-up here. Thank you.

hgmichna’s picture

jjkd, thanks for your patient explanation and the links. I wasn't aware that there is a need to go that far, because I currently need nothing of this, and I don't know any module that makes use of time values.

In my Drupal installations your example of the appointment across the time switch would work, because the appointment time would merely be some text in a message. But I see your point, and I agree that, if there are other functions that need proper time handling, then it should be done properly.

The sad thing is that we had the Auto Time Zone module in Drupal 5, which was good enough for me, we will hopefully have perfect time handling in Drupal 7, but we have a painful situation in Drupal 6, where 90% of my users are now in the wrong time zone.

Oh well, always look forward. It seems that there is no current solution, unless somebody writes a stopgap measure. How about a button for the administrator that leads to a repeat of the single time zone determination that Drupal 6 does once for each new user? The admin would press that button after the DST switch. Does anybody know of any hack, like deleting something in the database to force redetection?

alexanderpas’s picture

the problem with that is, there are different rules for timezones.

hgmichna’s picture

alexanderpas, with serious time handling this is no problem, as these two different times can be distinguished. Time systems have an extra flag for that.

alexanderpas’s picture

I'm not worried abou the storage and display system, as that is the easiest part.... I'm worried about the entry system for the user.

also no need for an extra flag (besides during entry) since we luckily still have DST-less UTC

hgmichna’s picture

sun, it was unavoidable that people like me would drop in here, because Drupal operators running Drupal 6 have an acute problem and are looking for a solution. (No Auto Time Zone module for Drupal 6.)

Most of them, including myself before now, do not know that serious time handling is needed for Drupal and why we cannot have a quick fix, and so some will come here and ask stupid questions. (Actually I'd love to have a quick fix right now. I need no serious time handling yet. May appreciate it in the future though.)

Let's hope that the exchange of the last few messages is sufficient to explain the issues. If not and if more people come in here asking stupid questions, you may have to write a comprehensive explanation for non-programmers. I guess merely telling people to stay away may not work. (:-)

Damien Tournoud’s picture

The good way to get strings that can't be extracted cleanly into po files is to submit an issue to potx. I've just done so (#329264: Add timezone identifiers to potx), please remove the timezone identifiers from system.install.

Damien Tournoud’s picture

Status:Needs review» Needs work

That's such a great patch. I so want this in core.

Here is a visual review:

<?php
 
function map_month($month) {
-  return
format_date(gmmktime(0, 0, 0, $month, 2, 1970), 'custom', 'M', 0);
+  return
format_date(gmmktime(0, 0, 0, $month, 2, 1970), 'custom', 'M', 'UTC');
 }
?>

This is sad, but out of scope for that patch. I opened another issue for this (#329273: map_month() is sad).

<?php
+    '#options' => array(t('Disabled'), t('Enabled')),
+   
'#options' => array(t('New users will be set to the default time zone at registration.'), t('New users will get an empty time zone at registration.'), t('New users will select their own time zone at registration.')),
+   
'#options' => array(t('Ignore empty user time zones.'), t('Remind users at login if their time zone is not set.')),
?>

Those three '#options' arrays should be explicitly keyed, and for the last two explicit constants should be defined. I know the original code didn't, but it's a bad habit we are trying to fight against.

<?php
// If the contributed Event module has set a default site time zone
// use that information.
+  elseif ($timezone_id = variable_get('date_default_timezone_id', 0)) {
+   
$timezone = db_result(db_query('SELECT name FROM {event_timezone} t WHERE t.timezone = :timezone_id', array(':timezone_id' => $timezone_id)));
+   
$timezone = str_replace(' ', '_', $timezone);
+   
variable_set('date_default_timezone', $timezone);
+  }
?>

This can fail a lot of different ways. Especially if the event_timezone table is not defined or if its schema don't fit what you are expecting. Please use a try/catch block around this.

<?php
+  else {
+   
variable_set('date_default_timezone', 'UTC');
+  }
?>

Hum. What about also using date_default_timezone_get(), as it is done above in the patch?

<?php
drupal_set_message('The default time zone has been set to <em>' . check_plain(variable_get('date_default_timezone', 'UTC')) . '</em>. Please check the ' . l('date and time configuration page', 'admin/settings/date-time') . ' to configure it correctly.', 'warning');
?>

The default argument to that variable_get() is not the same as the previous one.

<?php
+function system_user_register(&$edit, &$user, $category = NULL) {
+  if (
variable_get('configurable_timezones', 1)) {
+   
$form = array();
+    if (
variable_get('user_default_timezone', 0) < 2) {
?>

See comment above (what's the meaning of that 2?).

No other comments for the rest of the patch (but note: I can't - and don't want to - read javascript :p).

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new31.61 KB
Failed: Failed to apply patch.
[ View ]

- Fixed some missing t() calls.
- Fixed inline JS.
- Fixed minor inline documentation stuff.

Remaining minor issue: Variable name "empty_timezone_message" should probably be prefixed with "date_".

sun’s picture

Status:Needs review» Needs work
mfb’s picture

StatusFileSize
new32.1 KB
Failed: Failed to apply patch.
[ View ]

I thought there should be a way but I'd not even seen http://drupal.org/project/potx before :) Rerolled to remove tz identifiers and trailing whitespace. Still other issues at #282 to look at.

I am disregarding #283 for now. If you added t() calls to the .install file, please note that t() doesn't work in update scripts and other update scripts do not use them. It would be easier if you just give me the bugs to fix since I prefer not to merge patches together :)

hass’s picture

This is not correct way with t():

<?php
drupal_set_message('The default time zone has been set to <em>' . check_plain(variable_get('date_default_timezone', 'UTC')) . '</em>. Please check the ' . l('date and time configuration page', 'admin/settings/date-time') . ' to configure it correctly.', 'warning');
?>

Correct (untested):

<?php
drupal_set_message(t('The default time zone has been set to %date-default-timezone. Please check the <a href="@settings-date-time">date and time configuration page</a> to configure it correctly.', array('%date-default-timezone' => variable_get('date_default_timezone', 'UTC'), '@settings-date-time' => url('admin/settings/date-time'))), 'warning');
?>

EDIT: There seems to be a similar string in the last patch...

sun’s picture

StatusFileSize
new12.79 KB
Failed: Failed to apply patch.
[ View ]

Diff between patches in #285 and #283; left out wrong t() changes.

drewish’s picture

based solely on the file size i'm guessing that sun's patch in #287 is missing some thing...

mfb’s picture

Uhg, a huge patch diff. I will try to incorporate what I can pick out of this.

Why did you change change @user-edit to !user-edit? If you want to use !user-edit then you need to run it thru check_url() to correctly encode any ampersands in the URL.

<?php
-+    drupal_set_message(t('Please configure your <a href="@user-edit">account time zone setting</a>.', array('@user-edit' => url("user/$user->uid/edit", array('query' => drupal_get_destination(), 'fragment' => 'edit-timezone')))));
++   
drupal_set_message(t('Please configure your <a href="!user-edit">account time zone setting</a>.', array('!user-edit' => url("user/$user->uid/edit", array('query' => drupal_get_destination(), 'fragment' => 'edit-timezone')))));
?>
mfb’s picture

@sun: Another question from the patch diff:

<?php
-+    drupal_add_js('if (Drupal.jsEnabled) {
-+                     $(document).ready(function() {
-+                       Drupal.setDefaultTimezone();
-+                     });
-+                   }'
, 'inline');
++   
drupal_add_js('$(Drupal.setDefaultTimezone);', 'inline');
?>

I certainly like your version better, but can you explain why it's now OK to remove the if (Drupal.jsEnabled) logic? I'm just shuffling around the existing code and don't want to remove it if it was there to resolve some other issue.

mfb’s picture

Remaining minor issue: Variable name "empty_timezone_message" should probably be prefixed with "date_".

I would like to use a prefix, but I'm curious what is it that makes "date_" the correct prefix? After all this is not the Date module.

mfb’s picture

Status:Needs work» Needs review
StatusFileSize
new33.41 KB
Failed: Failed to apply patch.
[ View ]
<?php
+    '#options' => array(t('Disabled'), t('Enabled')),
+   
'#options' => array(t('New users will be set to the default time zone at registration.'), t('New users will get an empty time zone at registration.'), t('New users will select their own time zone at registration.')),
+   
'#options' => array(t('Ignore empty user time zones.'), t('Remind users at login if their time zone is not set.')),
?>

Those three '#options' arrays should be explicitly keyed, and for the last two explicit constants should be defined. I know the original code didn't, but it's a bad habit we are trying to fight against.

OK constants defined in system.module.

<?php
// If the contributed Event module has set a default site time zone
// use that information.
+  elseif ($timezone_id = variable_get('date_default_timezone_id', 0)) {
+   
$timezone = db_result(db_query('SELECT name FROM {event_timezone} t WHERE t.timezone = :timezone_id', array(':timezone_id' => $timezone_id)));
+   
$timezone = str_replace(' ', '_', $timezone);
+   
variable_set('date_default_timezone', $timezone);
+  }
?>

This can fail a lot of different ways. Especially if the event_timezone table is not defined or if its schema don't fit what you are expecting. Please use a try/catch block around this.

Done.

<?php
+  else {
+   
variable_set('date_default_timezone', 'UTC');
+  }
?>

Hum. What about also using date_default_timezone_get(), as it is done above in the patch?

This is handling the case of date_default_timezone == 0, which is UTC. I tweaked the code comment to try to explain better.

<?php
drupal_set_message('The default time zone has been set to <em>' . check_plain(variable_get('date_default_timezone', 'UTC')) . '</em>. Please check the ' . l('date and time configuration page', 'admin/settings/date-time') . ' to configure it correctly.', 'warning');
?>

The default argument to that variable_get() is not the same as the previous one.

This was intentional; the previous instance is testing to see if date_default_timezone is set to a non-zero number, so I supply an empty default value, otherwise if it was not set it would evaluate to the boolean true default value.

<?php
+function system_user_register(&$edit, &$user, $category = NULL) {
+  if (
variable_get('configurable_timezones', 1)) {
+   
$form = array();
+    if (
variable_get('user_default_timezone', 0) < 2) {
?>

See comment above (what's the meaning of that 2?).

Now using the constants.

One other change: I was testing the patch on another system and noticed that for some reason the PHP time zones were sorted differently, with continents in ascending alphabetical order and cities in descending alphabetical order. So I am now running the array of zones thru asort(). This was also needed anyways to present the translated time zones in alphabetical order.

Damien Tournoud’s picture

The logic in system_update_7012() is still a little off.

It could be made more robust this way:

<?php
$timezone
= NULL;

// Use an existing timezone if set.
if (is_valid_timezone($existing_timezone = variable_get('date_default_timezone', NULL))) {
 
$timezone = $existing_timezone;
}

// Else, use contributed module timezone if set.
if (!$timezone && is_valid_timezone($date_timezone = variable_get('date_default_timezone_name', NULL))) {
 
$timezone = $date_timezone;
}

// Else, use the timezone from the Event contrib module if installed.
if (!$timezone && $timezone_id = variable_get('date_default_timezone_id', 0)) {
 
// ...
  // grab the timezone from event.
  // ...
 
if (is_valid_timezone($event_timezone)) {
   
$timezone = $event_timezone;
  }
}

// Else, use the existing timezone from the site.
if (!$timezone && $numeric_timezone = variable_get('date_default_timezone', 0)) {
 
$timezone = timezone_name_from_abbr('', intval($timezone), intval(date('I'))));
}

// If everything else failed, default to UTC.
if (!$timezone) {
 
$timezone = 'UTC';
}

variable_set('date_default_timezone', $timezone);
?>

Where is_valid_timezone() validate that the timezone string is in _system_zonelist().

Damien Tournoud’s picture

There are still two uses of variable_get('date_default_timezone') with inconsistent defaults:

  • in system_user_register(): variable_get('date_default_timezone', '')
  • in system_user_timezone(): variable_get('date_default_timezone', '')
  • in UserRegistrationTestCase: variable_get('date_default_timezone', '')
  • in system_date_time_settings(): variable_get('date_default_timezone', date_default_timezone_get())

All those should probably default to 'UTC' instead.

mfb’s picture

I'm aware that these default values are inconsistent but I don't think it's a problem (personally). There were specific requests in this thread to use date_default_timezone_get() so I am using that in certain places. And the uses of variable_get('date_default_timezone', '') simply result in an empty user time zone if the system default time zone has not been defined for some reason.

jjkd’s picture

No longer applies against HEAD:

...
patching file includes/form.inc
Hunk #1 FAILED at 1716.
1 out of 1 hunk FAILED -- saving rejects to file includes/form.inc.rej
...

jjkd’s picture

I would still like a wording change along the lines of that in #255 go in.

mfb’s picture

StatusFileSize
new32.9 KB
Failed: Failed to apply patch.
[ View ]

OK this addresses #293, #296 and #297.

sun’s picture

Reconsidered:

+Drupal.setDefaultTimezone = function() {

should be a regular JS behavior, making the mess with inline JavaScript (#290) obsolete. To do this, the form should implement a unique CSS class on the target form field, for example system-user-timezone, which will make this code more reliable/stable:

+  $.getJSON(Drupal.settings.basePath, { q: path, date: dateString }, function (data) {
+    if (data) {
+      $("#edit-date-default-timezone, #edit-timezone").val(data);
+    }
+  });

i.e.

+  $.getJSON(Drupal.settings.basePath, {q: path, date: dateString}, function (data) {
+    if (data) {
+      $("select.system-user-timezone").val(data);
+    }
+  });

Note that there should be no space after the opening and before the closing curly brace in the options argument to $.getJSON().

system_timezone() should complete the request using exit():

+function system_timezone($abbreviation = '', $offset = -1, $is_daylight_saving_time = NULL) {
...
+  drupal_json($timezone);
+}

Also, I'm very unsure whether we really want to log each timezone detection, since this is a JS callback that can be accessed *many* times:

+  // Log a debug message.
+  watchdog('timezone', 'Detected time zone: %timezone; client date: %date; abbreviation: %abbreviation; offset: %offset; daylight saving time: %is_daylight_saving_time.', array('%timezone' => $timezone, '%date' => $date, '%abbreviation' => $abbreviation, '%offset' => $offset, '%is_daylight_saving_time' => $is_daylight_saving_time), WATCHDOG_DEBUG);

Please remove this unnecessary change from system_update_7007():

@@ -3033,7 +3033,6 @@ function system_update_7007() {
   return $ret;
}

-
/**
  * Use the poll_choice primary key to record votes in poll_votes rather than
  * the choice order. Rename chorder to weight.

isset() should be left as is here:

function format_date($timestamp, $type = 'medium', $format = '', $timezone = NULL, $langcode = NULL) {
-  if (!isset($timezone)) {
+  static $timezones = array();
+  if (!$timezone) {

I would have rerolled the patch, but it seems you want to scratch your own itch.

sun’s picture

Note that there should be no space after the opening and before the closing curly brace in the options argument to $.getJSON().

Sorry, scratch that. Core IS using spaces at the bounds of JavaScript objects - for whatever reason.

mfb’s picture

should be a regular JS behavior, making the mess with inline JavaScript (#290) obsolete. To do this, the form should implement a unique CSS class on the target form field, for example system-user-timezone, which will make this code more reliable/stable:

Thanks, I am reading up on Drupal.behaviors now so hopefully I can figure this out for the next reroll.

Note that there should be no space after the opening and before the closing curly brace in the options argument to $.getJSON().

I copied my apparently wrong use of spaces inside the { } from various places in Drupal core js files. Could we add this rule to the coding standards at http://drupal.org/node/172169 ?

system_timezone() should complete the request using exit():

Often when drupal_json() is used in core it is not followed by exit(), e.g. user_autocomplete(). Are we sure it's a required or recommended practice? If so we should document it.

Also, I'm very unsure whether we really want to log each timezone detection, since this is a JS callback that can be accessed *many* times:

See #169 -- It's supposed to be a temporary debug message so we can test how well it's working. I think it may prove helpful to leave it in for now but we should add a @todo message to make clear it's temporary.

Please remove this unnecessary change from system_update_7007():

OK removing extra lines elsewhere is out of scope..

isset() should be left as is here:

This change was intended to avoid PHP warnings if format_date() is called with an empty but not NULL timezone. But, I'd agree, it's best to be strict, and demand either a valid time zone name or NULL.

mfb’s picture

StatusFileSize
new33.31 KB
Failed: Failed to apply patch.
[ View ]

I re-implemented time zone detection as a JS behavior.

But one thing I noticed is that the clean URLs check (admin/settings/clean-urls/check) runs on every page where system.js is loaded. Which now would include the user-edit page if the user has an empty time zone. Is this a bug in system.js?

hass’s picture

Status:Needs review» Needs work

1. Incorrect context sensitive string and invalid use of t(). See http://api.drupal.org/api/function/t/7:

$date .= trim(t('!long-month-name ' . date_format($date_time, $c), array('!long-month-name' => ''), $langcode));
+  drupal_set_message('The default time zone has been set to <em>' . check_plain($timezone) . '</em>. Please check the ' . l('date and time configuration page', 'admin/settings/date-time') . ' to configure it correctly.', 'warning');
+    drupal_set_message('User time zones have been emptied and need to be set to the correct values. Use the new ' . l('time zone options', 'admin/settings/date-time') . ' to choose whether to remind users at login to set the correct time zone.', 'warning');

2. Invalid code style:

'#options' => array(0 => t('Disabled'), 1 => t('Enabled')),

Correct:

'#options' => array(
  0 => t('Disabled'),
  1 => t('Enabled')
),

3. Semicolon should be a period.
setting for a new registration; users will be able to change their time zone

4. The simple test's contain many un-t'ified strings like 'Date should be identical, with GMT offset of -10 hours.'. I'm not sure if this is ok.

5. I think it's not a good idea to save a setting as named value like variable_set('date_default_timezone', 'America/Los_Angeles');. I'm the fan of unique integer values as names could change and will change after translation. As an example the name of this time zone will become 'Amerika/Los Angeles' in German. I could also give more examples in Germany like "Germany/Cologne" would be translated to German as "Deutschland/Köln" - however Cologne isn't a time zone city or not... here are potential issues. And not to mention about typo's... maybe we fix 'America/Las Angeles' in a later release and then all people loose their setting because of the typo correction.

jjkd’s picture

Re: point 5: The values to be stored in the Drupal database actually come from an upstream source, tzinfo, which contains standardized values for these. In effect, they are already foreign keys, just not numeric. The value is typically translated for display/selection only. The German translation is...

#: timezones.h:140
msgid "America/Los_Angeles"
msgstr "Amerika/Los_Angeles"

... already established, as are those for many other languages. 'Germany/Cologne' would not be offered as an option, that doesn't exist as a tzinfo choice/option.

While it would be possible to introduce a numeric foreign key notation for this, that would mean an additional db table we don't have now as all the run-time support for this is inside PHP. This would also require a methodology for syncing our table with PHP's internalized data.

mfb’s picture

StatusFileSize
new33.34 KB
Failed: Failed to apply patch.
[ View ]

@hass:

1. Incorrect context sensitive string and invalid use of t(). See http://api.drupal.org/api/function/t/7:

$date .= trim(t('!long-month-name ' . date_format($date_time, $c), array('!long-month-name' => ''), $langcode));

+  drupal_set_message('The default time zone has been set to <em>' . check_plain($timezone) . '</em>. Please check the ' . l('date and time configuration page', 'admin/settings/date-time') . ' to configure it correctly.', 'warning');

+    drupal_set_message('User time zones have been emptied and need to be set to the correct values. Use the new ' . l('time zone options', 'admin/settings/date-time') . ' to choose whether to remind users at login to set the correct time zone.', 'warning');

For the first line, can you point out what is the problem in $date .= trim(t('!long-month-name ' . date_format($date_time, $c), array('!long-month-name' => ''), $langcode));? This line is mostly copied with a slight modification.

For the next two lines, can you confirm for us that t() should be used in hook_update_N implementations? It is not used in other update scripts. See for example system_update_7003(). You said in #200 "t() in update makes no sense, while the po strings are imported afterwards."

2. Invalid code style:
'#options' => array(0 => t('Disabled'), 1 => t('Enabled')),

Correct:
'#options' => array(
0 => t('Disabled'),
1 => t('Enabled')
),

What I have is valid code style, see the rest of system.admin.inc and also the code style guide ("if the line spans longer than 80 characters...")

3. Semicolon should be a period.
setting for a new registration; users will be able to change their time zone

OK.

4. The simple test's contain many un-t'ified strings like 'Date should be identical, with GMT offset of -10 hours.'. I'm not sure if this is ok.

Fixed.

5. I think it's not a good idea to save a setting as named value like variable_set('date_default_timezone', 'America/Los_Angeles');. I'm the fan of unique integer values as names could change and will change after translation. As an example the name of this time zone will become 'Amerika/Los Angeles' in German. I could also give more examples in Germany like "Germany/Cologne" would be translated to German as "Deutschland/Köln" - however Cologne isn't a time zone city or not... here are potential issues. And not to mention about typo's... maybe we fix 'America/Las Angeles' in a later release and then all people loose their setting because of the typo correction.

When a time zone name changes (e.g. Asia/Calcutta was recently replaced with Asia/Kolkata), PHP preserves the old time zone for backwards compatibility. The time zone database has been compiled into PHP, which is about as optimized as it gets in drupal. My feeling is that creating a mapping of numeric IDs to time zones (in code or database) would not be worth the extra overhead.

hass’s picture

You can use t() in update scripts. No problem. We may find a way to load this strings earlier in D7... :-). Also see http://drupal.org/node/322731.

(t('!long-month-name ' . date_format($date_time, $c), array('!long-month-name' => '')
If this is an old string we need to find out first before shooting... I remember there was some discussion about using !long-month-name for a special case, but not sure what the problem was. Normally you should use t('!long-month-name @date_time', array('!long-month-name' => '', '@date_time' => date_format($date_time, $c))) to have a context sensitive string.

As I know - arrays should have one value pair per line. This makes it easier to compare changes via CVS. I've never heard about a 80 chars exception.

sun’s picture

I hope you recognized #300. Your JS object notation is consistent with Drupal core. Sorry for derailing.

To prevent Drupal.behaviors.setTimezone from running on all pages where system.js is loaded, it should be moved into a separate misc/timezone.js.

After reading #169, please remove the watchdog message. This is what our testing framework is for. Hopefully, we'll soon have JS tests, too.

hass' points 1) and 2) are wrong. He even refers to http://drupal.org/node/322731, which explicitly states that t() should not be used in update functions.

mfb’s picture

StatusFileSize
new33.97 KB
Failed: Failed to apply patch.
[ View ]

I moved the timezone js into its own file.

In next reroll I can remove the watchdog message.

Maybe in the meantime someone can help with testing on various browsers and timezones (IE 8, Opera, iPhone, etc.)

drewish’s picture

hass in #306, your link in to [#322731] seems to contradict your comment. it indicates the update messages should NOT be run through t().

jjkd’s picture

Tested on Opera 9.27 for Windows XP, time zone detection works.

Tested on Opera 9.62 for Windows XP, time zone detection works.

Tested on Opera 8.65 for Windows Mobile running on Windows Mobile 6, time zone detection works.

Tested on IE Mobile running on Windows Mobile 6, time zone detection works, though the field width is strange. Changing the various options for how the browser maps the web page to the device screen seems to change the behavior, so I think this issue is due to the browser and probably not significant.

jjkd’s picture

Tested on IE 8 beta 2, time zone detection works.

mfb’s picture

@jjkd: Could you test with your local environment set to a time zone with a non-hour offset from GMT, like India? I had some trouble with that previously and never did get to the bottom of it. If it's still a problem it might be something we could work around in javascript or on PHP side.

hass’s picture

@sun: 1) is correct.

Also see http://drupal.org/node/194962#comment-1065964. We are currently using t() in .install files. There are a few exceptions like documented in http://drupal.org/node/322731. Nevertheless it is totally wrong to have a string without using placeholders and it doesn't matter if you are using $t or t(). So you can use $t in install, but bad constructs like 'The default time zone has been set to <em>' . check_plain($timezone) . '</em>. Please check the ' . l('date and time configuration page', 'admin/settings/date-time') . ' to configure it correctly.' are not allowed.

EDIT: http://drupal.org/node/322774 jump to $DO_NOT_DO_THIS

hass’s picture

About #2, see the coding rules at http://drupal.org/coding-standards what says Note that if the line spans longer than 80 characters (often the case with form and menu declarations), each element should be broken into its own line, and indented one level. You can also see the Schema documented in http://drupal.org/node/322731. It break every array value ( = each element) into one line nevertheless there is some space left to 80 chars. Something wrong with this!?

mfb’s picture

I wouldn't mind making this code cleaner but can you all have a debate about t() in update functions somewhere else, like the comments on http://drupal.org/node/322731 or devel list or g.d.o.? Let me know if/when the API docs have been updated and I'll modify the patch. For now I am copying similar constructs like locale_update_6005() system_update_6045() etc.

sun’s picture

Status:Needs work» Needs review

@hass: Please stop derailing this issue. Gábor is explicitly referring to hook_schema(). http://drupal.org/node/322731 is also explicitly referring to hook_schema(). And it explicitly states that t() should NOT be used in module update functions. Please read that stuff again. The strings you are referring to do not use t() at all, which is the current documented practice. Even if there were some changes in the planning stage, this patch must not implement those changes, as long they are not committed. Also, this patch should not change

$date .= trim(t('!long-month-name ' . date_format($date_time, $c), array('!long-month-name' => ''), $langcode));

since the rest of the function uses the same method to localize date parts. If you think that this is wrong, file a separate issue. Thanks.

@mfb: Patch looks good now. If SimpleTests pass, we should gain some core maintainer attention.

mfb’s picture

Well there's one little thing I noticed, I added a try/catch block in system update and so I should do the same in user update. so more work on this later tonight while i watch election news..

drewish’s picture

asking hass not to derail an issue is like asking a bird not to fly--it's just what he does.

hass’s picture

@mfb: core is not bug free regarding context sensitive strings... I've also found a few strings in near past and I'm sure not many developers care about the update stuff - if it seems working fine from logic side and it wasn't yet multilingual... that could be all.

@drewish: That's not true. I *care* about translatability what many/most developers don't do! This is what waisted weeks of my time within the last months and could be prevented by simply following the long time written rules. I really hoped that Gabor's docs gave us something we can simply point to if developers don't care about the docs. It seems like we need to extend the examples to stop people from guessing and interpreting. Aside - as image module maintainer you should take a look to my big patch in #320293: Translatable strings review. It have some good examples why the docs have been written or extended.

drewish’s picture

@hass, whether you like it or not you've got a reputation for derailing issues that spans several issue queues. i think it's great to be concerned about i18n issues but you manage to bring up these issues in the worst way possible. as i'd pointed out the changes you'd proposed contradicted the documents you'd linked to supporting them--that's not helpful.

mfb’s picture

StatusFileSize
new34.14 KB
Failed: Failed to apply patch.
[ View ]

I added another try/catch block. Also, I noticed the event_timezones table name was spelled wrong in one case so I'm guessing no one has tested upgrading from drupal-6 event module.

hass’s picture

I'm sorry that I've said to use t() in install process that was wrong, but $t() is correct. I apologize not to have read the patch such carefully in all details and I flown over the code only to see if there are potential i18n issues and I found some. The below strings are proven to be wrong - if there are other examples in core or not. Why is it derailing to show bugs in attached patches? I would also be happy to get this patch in, but with such bugs it looks not core worthy to me.

<?php
drupal_set_message
('The default time zone has been set to <em>' . check_plain($timezone) . '</em>. Please check the ' . l('date and time configuration page', 'admin/settings/date-time') . ' to configure it correctly.', 'warning');

drupal_set_message('User time zones have been emptied and need to be set to the correct values. Use the new ' . l('time zone options', 'admin/settings/date-time') . ' to choose whether to remind users at login to set the correct time zone.', 'warning');
?>
jjkd’s picture

The patch from #308 tested with Google Chrome on Windows XP, time zone detection works.

The patch from #321 applies against HEAD with some fuzz.

Re: #312, some time zones not being selected properly, I did duplicate this symptom. Looks like the problem is with timezone_name_from_abbr() in system.module, specifically the system_timezone menu callback, as everything is fine up until the call to timezone_name_from_abbr(). A bug report is open against PHP that I believe is the same issue: http://bugs.php.net/bug.php?id=44780. Might be a good idea to replace this function with our own code.

mfb’s picture

StatusFileSize
new34.15 KB
Failed: Failed to apply patch.
[ View ]

I added a wrapper function for timezone_name_from_abbr() so we have a place to work around any issues. Namely, for certain buggy offsets if the abbreviation is missing, we supply a working abbreviation and pass all the parameters thru to timezone_name_from_abbr().

jjkd’s picture

For PHP 5.2.6, I believe that this is the list of buggy offsets:

-34200, ckhst
-16200, ant
-12600, negt
-7200, addt
12600, irst
16200, aft
19800, ist
20700, npt
21600, aktst
23400, burt
28800, bnt
34200, cast
35100, cwst
37800, cst
39600, anat
41400, lhst
46800, anast
49500, chadt
50400, anast
mfb’s picture

Ok that's more than I expected :/
Thanks for posting to bugs.php.net - everybody should vote on the issue over there, see link in #323 :)
There's also the possibility of mapping Windows time zones to zoneinfo time zones but that would require even more bytes of code than this mapping of offsets to abbreviations.

jjkd’s picture

Yes, I agree that's quite a few entries. I'm not sure about putting all those in. After some consideration, I think we might be better off overall to drop the wrapper function and return to letting the code default to having the user select their timezone manually in these cases. I am not convinced that the results of extending this code would be much of a net improvement given the other uncertainties in the time zone detection process.

As for mapping Windows time zones, I agree that there is some potential there, but after a decision on the previous point, I'd suggest that we call the time zone detection 'done' for now, I think it works quite well as it is. We can come back to it after the rest of the patch is done and committed. If anyone has an issue with the current level of functionality, I'm sure they will speak up.

mfb’s picture

Please try running this sample code:

<?php
$bad
= array(
  -
34200 => 'ckhst',
  -
16200 => 'ant',
  -
12600 => 'negt',
   -
7200 => 'addt',
  
12600 => 'irst',
  
16200 => 'aft',
  
19800 => 'ist',
  
20700 => 'npt',
  
21600 => 'aktst',
  
23400 => 'burt',
  
28800 => 'bnt',
  
34200 => 'cast',
  
35100 => 'cwst',
  
37800 => 'cst',
  
39600 => 'anat',
  
41400 => 'lhst',
  
46800 => 'anast',
  
49500 => 'chadt',
  
50400 => 'anast',
);
foreach (
$bad as $offset => $abbr) {
 
$timezone = timezone_name_from_abbr($abbr, $offset, NULL);
 
date_default_timezone_set($timezone);
 
$mismatch = (date('Z') == $offset) ? '' : 'MISMATCH';
  print
str_pad($offset, 6, ' ', STR_PAD_LEFT)
    .
" " . str_pad($abbr, 5) . ": " . str_pad($timezone, 19)
    .
" " . str_pad(date('Z'), 6, ' ', STR_PAD_LEFT)
    .
" " . str_pad(date('T'), 5) . " $mismatch\n";
}
?>

It would appear that 11 of the 19 abbrevation/offset mappings you have are for obsolete time zones, because the abbreviation/offset maps to a time zone which in turn maps to a different abbreviation and offset. So we might be able to have a small wrapper function.

mfb’s picture

Well the more I dig the more I will find time zones, like BURT is obsolete but has been replaced with MMT.

mfb’s picture

I think for now I agree with jjkd, get rid of the wrapper function to keep Drupal core lean and mean. Some javascript detection is not quite working on Windows for a few time zones, but hopefully PHP will eventually fix its offset=>timezone mapping bug.

jjkd’s picture

Sorry, the mapping to obsolete time zones is probably my fault, the code I used to do that didn't make any attempt to avoid obsolete time zones. It merely brute-forced its way through the list of time zones, checking every offset it found. Then, for any time zone offset that didn't get a result from timezone_name_from_abbr() when only offset was supplied, it picked up the 'first' abbreviation match that contained a mapping to the same offset. Note also that under Windows, I don't believe we typically get the abbreviation back from JavaScript at all, all we have to go on is the offset.

mfb’s picture

StatusFileSize
new33.61 KB
Failed: Failed to apply patch.
[ View ]

Rerolled without the wrapper function. @haas, I looked at your last comment and I think you're talking about get_t() but that just seems like a roundabout way of using t().

Pages