[aur-dev] [PATCH v6 1/3] Add user set timezones
Lukas Fleischer
lfleischer at archlinux.org
Thu Jan 19 20:52:29 UTC 2017
On Thu, 19 Jan 2017 at 04:23:37, Mark Weiman wrote:
> Currently, aurweb displays all dates and times in UTC time. This patch
> adds a capability for each logged in user to set their preferred
> timezone.
>
> Signed-off-by: Mark Weiman <mark.weiman at markzz.com>
> ---
> [...]
Thanks, we're almost there. There are several minor issues left, though.
See below.
> diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php
> index 9015ae8..8a0ed2b 100644
> --- a/web/lib/aur.inc.php
> +++ b/web/lib/aur.inc.php
> [...]
> @@ -356,7 +359,7 @@ function uid_from_sid($sid="") {
> function html_header($title="", $details=array()) {
> global $LANG;
> global $SUPPORTED_LANGS;
> -
> +
Unrelated whitespace change?
> include('header.php');
> return;
> }
> [...]
> diff --git a/web/lib/timezone.inc.php b/web/lib/timezone.inc.php
> new file mode 100644
> index 0000000..f3b4c71
> --- /dev/null
> +++ b/web/lib/timezone.inc.php
> @@ -0,0 +1,65 @@
> [...]
> +/**
> + * Set the $TZ global variable.
The comment needs to be updated to reflect the changes in this reroll of
the patch series.
> + *
> + * @return null
> + */
> +function set_tz() {
> + $timezones = generate_timezone_list();
> + $update_cookie = false;
> +
> + if (isset($_COOKIE["AURTZ"])) {
> + $TZ = $_COOKIE["AURTZ"];
It is quite confusing to use an upper case variable name here enough
though this no longer is a global variable. Something like $timezone
might be more appropriate.
> + } elseif (isset($_COOKIE["AURSID"])) {
> + $dbh = DB::connect();
> + $q = "SELECT Timezone FROM Users, Sessions ";
> + $q .= "WHERE Users.ID = Sessions.UsersID ";
> + $q .= "AND Sessions.SessionID = ";
> + $q .= $dbh->quote($_COOKIE["AURSID"]);
> + $result = $dbh->query($q);
> +
> + if ($result) {
> + $row = $result->fetchAll();
> + $TZ = $row[0][0];
As I mentioned in an earlier review, this can be simplified as
$timezone = $result->fetchColumn(0);
already assuming that $TZ becomes $timezone.
> + } else {
> + $TZ = config_get("options", "default_timezone");
This else-branch is not needed. The default case is already handled
below.
> + }
> +
> + $update_cookie = true;
> + }
> +
> + if (isset($TZ) && array_key_exists($TZ, $timezones)) {
> + date_default_timezone_set($TZ);
> + } else {
> + $TZ = config_get("options", "default_timezone");
> + date_default_timezone_set($TZ);
> + }
As I also mentioned in an earlier review, this can be simplified as
follows (also assuming that $TZ is replaced by $timezone):
if (!isset($timezone) || !array_key_exists($timezone, $timezones)) {
$timezone = config_get("options", "default_timezone");
}
date_default_timezone_set($timezone);
> +
> + if ($update_cookie) {
> + $timeout = intval(config_get("options", "persistent_cookie_timeout"));
> + $cookie_time = time() + $timeout;
> + setcookie("AURTZ", $TZ, $cookie_time, "/");
> + }
> +}
> diff --git a/web/template/account_edit_form.php b/web/template/account_edit_form.php
> index 19821a0..8b207e7 100644
> --- a/web/template/account_edit_form.php
> +++ b/web/template/account_edit_form.php
> @@ -129,6 +129,24 @@
> ?>
> </select>
> </p>
> + <p>
> + <label for="id_timezone"><?= __("Timezone") ?></label>
> + <select name="TZ" id="id_timezone">
> +<?php
> + $timezones = generate_timezone_list();
> + while (list($key, $val) = each($timezones)) {
> + if ($TZ == "") {
> + $TZ = config_get("options", "default_timezone");
> + }
Broken indentation. I also think this if-block belongs to
display_account_form() and should be executed before
account_edit_form.php is included.
> + if ($TZ == $key) {
> + print "<option value=\"".$key."\" selected=\"selected\"> ".$val."</option>\n";
> + } else {
> + print "<option value=\"".$key."\"> ".$val."</option>\n";
> + }
> + }
> +?>
> + </select>
> + </p>
> </fieldset>
>
> <fieldset>
> [...]
Looks good otherwise!
More information about the aur-dev
mailing list