search

ualibraries / Guide-on-the-Side

The University of Arizona Libraries will no longer provide support for Guide on the Side. The code will remain openly available; however, UAL can no longer provide code fixes or upgrades.
https://ualibraries.github.io/Guide-on-the-Side/about.html
Other
66 stars 48 forks source link

Set a default timezone #103

Closed jaronkk closed 8 years ago

jaronkk commented 9 years ago

Fixes #97

As described in issue 97, GotS encountered errors serving assets because I didn't have a timezone set in my local PHP setup. (CakePHP was attempting to send the Last-Modified header for the file).

This is fixed by calling date_default_timezone_set('UTC') in app/Config/core.php if date.timezone is not set via the PHP ini.

I refrained from adding a configuration option since I think most people can get it configured via their PHP settings. If you would like the configuration option for timezone, I can include this commit: https://github.com/ndlib/Guide-on-the-Side/commit/74a910d45c5f95f6e54d790636e2c8022b478177

simpsonw commented 9 years ago

Hi @jaronkk ,

Thanks for the pull request! I definitely think that CakePHP's behavior regarding time zones is a bit wonky, but I'm not 100% sure about this particular fix. The problem is that ini_get seems to only be checking if something is set in the php.ini file, not if the setting is accounted for in general. To illustrate, here's a block of code I tested locally:

if (!ini_get('date.timezone')) {
    // Set the default timezone to UTC if it was not set via php.ini
    date_default_timezone_set('UTC');
}

if (!ini_get('date.timezone')) {
    die("here");
}

When I ran it, I got to the die statement, which seems a bit odd to me. I do like the configuration option you mentioned in https://github.com/ndlib/Guide-on-the-Side/commit/74a910d45c5f95f6e54d790636e2c8022b478177 because it seems to be a bit more explicit.

Just my two cents, though!

michaelhagedon commented 9 years ago

I agree with @wsimpson's assessment that the optional config parameter seems better, though it does clutter the config file some. @jaronkk, could we have that in this or a new PR? Thanks!

michaelhagedon commented 8 years ago

Hi @jaronkk, are you still interested in this? Sorry, we've taken a while to get back to you!

I'd like to address #97... for the reason Will mentions, I think we'd prefer the config option version. Would you like to send that over as a PR? I think there are some people, particularly those using shared hosting, who could benefit from that. Thanks either way! I'm going to close this in the meantime, but feel free to update this PR or send a new one.