The format_time_zones() function creates a list of time zones appropriate for use in a select list (for instance this is used in the final screen of the installer.) It prints this list in a format of "[timezone]: [formatted current time in that timezone]". The list of timezones has 415 entries, which means this calls format_date() 415 times.

The date formatting doesn't really seem to add anything to the drop down (assumedly users know what their own time zone is.) It is also causing enormous slowdowns in the installer.

Solution: Just display the time zone.

Patch in a minute.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gdd’s picture

Status: Active » Needs review
FileSize
840 bytes

Easy enough

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks good if the bot comes back green. This fixes currently broken HEAD by removing the 1200 config() calls from format_date().

xjm’s picture

Priority: Normal » Major

Wowza. The slowdown is very noticeable.

catch’s picture

Just to clarify on #2, it obviously doesn't remove any config() calls from format_date(), but it removes the 400+ format_date() calls from the last page of the installer (and user account pages iirc).

To fix the actual performance issue, since we do 600 format_date() calls when viewing 300 comments, we'd need to sort out #1187726: Add caching for configuration / rework config object loading (Was: Memory usage and i/o from config objects).

webchick’s picture

Status: Reviewed & tested by the community » Fixed
FileSize
31.25 KB
37.25 KB

Before:

Date format reads America/Los Angeles November 23, 2012 17:23

After:

Just America/Los Angeles

I think users have many other ways to check the current date/time in their timezone than the Drupal installer needing to provide this. Speeding the testbot runs up by an hour is also a nice bonus. ;P

Committed and pushed to 8.x. Thanks!

Eric_A’s picture

Status: Fixed » Needs review
FileSize
685 bytes

(EDITED)

I'm all for Smallest Possible Change but since @zone: @date has become @zone the call to t() now looks silly. So the next change would be to use format_string() ((EDIT: erhm, even better:check_plain()), but in fact that @zone placeholder has been taken care of already in the earlier call to t(). So unless we were relying on some ugly t() / double sanitizing magic, we should completely ditch the last transformation.

webchick’s picture

Priority: Major » Normal

Hm. One could also argue that it might be nice for this string to be translatable so someone could introduce their own formatting is so desired.

But in any case, this isn't "major" anymore.

xjm’s picture

Priority: Normal » Major
Status: Needs review » Fixed

Can we open a followup issue to discuss that?

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.