[aur-dev] [PATCH v2] Add user set timezones

[Mark Weiman]

On Tue, 2016-11-15 at 07:42 +0100, Lukas Fleischer wrote:
> 
> First of all, thanks for taking the time to implement all this! I added
> some inline comments below.
> 
> > +deafult_timezone = UTC
> 
> Typo?
> 

Yes.

> 
> Out of curiosity: How did you choose the length of 50 characters? Is it
> large enough according to some standard?
> 

Yeah, I chose something that was most likely long enough to hold anything that I
needed to put in there. It looks like Johannes found it to be a maximum of 33
characters, and after looking, the current maximum length is 30, so should I use
33 or 30?

> 
> This is really nitpicky but we usually add colons after these lines.
> 

Okay, I will fix this.

> 
> Okay. Looks a bit weird but I agree that doing this is better than
> reformatting the whole list in every patch that adds or removes a
> parameter. Maybe we should change the formatting to "one parameter per
> line" here. Not in the scope of this patch, though.
> 

Yeah, I really wasn't sure how to do this properly.

> 
> Should this refer to the default timezone from the configuration file
> instead?
> 

Probably a good idea.

> 
> Huh? Leftover debug statements? I think these should go...
> 

Yikes! I would have to agree.

> 
> I do not know about timezone handling in PHP, so this question might be
> naive: Is there any reason you chose this implementation instead of
> calling something like date_default_timezone_set() in set_tz() and using
> the default date/time functions without any wrappers?
> 

When reading the timestamp in gmdate(), PHP doesn't care what timezone the
default is set to and it just displays what the time is based on the how many
seconds past the Unix epoch. Because of this, you have to get the timezone's
offset (which is in UTC from the database) and do the math to make sure it's
correct. I have verified this in a PHP shell. Changing the default doesn't make
sense to me also because the application is only displaying other timezones, not
working in them, only UTC.

> Regards,
> Lukas
> 
> P.S.: Not a big issue but if you resubmit a patch several times, it is
> usually a good idea to add a comment on what changed between the "---"
> marker and the diff stat. Just to make it easier for reviewers to pick a
> version to review and to know about previously addressed stuff.

Mark Weiman