set default time zone for date #154

das7pad · 2016-10-04T16:07:42Z

@pi-hole/dashboard

this prevents basic error messages from php(-cgi) for not setting the
timezone and then using UTC as default

HOTFIX RELEASE 1.4.2

this prevents basic error messages from php(-cgi) for not setting the timezone and then using UTC as default

PromoFaux · 2016-10-04T17:49:36Z

Not an error I've come across before, but I will test this and see if it breaks anything!

PromoFaux · 2016-10-04T18:02:12Z

Approved

Not causing issues for me, but others may have a better insight. I'm not really a PHP guy!

AzureMarker · 2016-10-08T16:52:13Z

I'm testing this now, to make sure that it doesn't force the timezone to be UTC if the server timezone is different.

AzureMarker · 2016-10-08T17:43:14Z

While it does not affect data.php (because it reads in the dates from dnsmasq, which uses the system timezone), it does change how errors are logged from auth.php. Because of this, I would recommend that you make php use the system timezone.

this will imply the system time zone. command date and the given format are supported by the majority of linux distros

trim against leading space from %k (range of ' 0' to '23') and strtolower for AM/PM that %p outputs as uppercase, while 'a' (-> am/pm) was used before

das7pad · 2016-10-09T04:08:45Z

@Mcat12 So far I was not able to find a single solution to get the correct system time zone on most of the common linux distros within php. In addition there are incompatibilities between the output of the system and php. CEST for example can't be used by php and it's date_default_timezone_set.
So I came up with the system command date, which can be used for the actual time and also a given time as string. The basic format should work on most of the linux distros.

AzureMarker · 2016-10-09T12:21:16Z

Looks great! Just one more thing.

AzureMarker · 2016-10-09T12:22:42Z

data.php

@@ -125,7 +125,7 @@ function getAllQueries() {
        $hostname = trim(file_get_contents("/etc/hostname"), "\x00..\x1F");

        foreach ($dns_queries as $query) {
-            $time = date_create(substr($query, 0, 16));
+            $time = exec("date --date='" . trim(substr($query, 0, 16)) . "' +'%Y-%m-%d\%Z%H:%M:%S'");


Can this use the same format as before? Y-m-d\TH:i:s

sure! I thought the Timezone (T in php date) is used here and forgot about the function of the \ ^^

just output a 'T' insted of the Timezone

AzureMarker · 2016-10-09T21:26:32Z

Approved

PromoFaux · 2016-10-18T15:07:43Z

@das7pad had to revert this back out of the release, as it was causing issues. Happy to revisit, but we need to work out what it is doing to slow down the query page!

akoebbe · 2016-10-18T15:12:06Z

After looking over this change, I've got some concerns with making exec commands in the script, especially within iterators. In general it's a bad idea and I think we might be able to achieve the same result without needing to use exec. I'm happy to refactor this if there's no objection.

PromoFaux · 2016-10-18T15:12:53Z

Suggestions and contributions always welcome!

PromoFaux · 2016-10-18T15:13:04Z

Unless they're rubbish... ;)

akoebbe · 2016-10-18T15:14:29Z

Well forget it then. My default setting is "rubbish". :-)

das7pad · 2016-10-19T11:49:45Z

ok, I can understand the performance issue here.

However not setting the timezone will spam the logfiles.

What about setting the timezone on a first run?
Check for existence and the then content of a file that will contain the timezone. If one fails, show the list of DateTimeZone::listIdentifiers() with a search functionality ( like https://silviomoreto.github.io/bootstrap-select/examples/#live-search )
In addition I would add a button to the top right menu to change the timezone later.

akoebbe · 2016-10-19T13:52:12Z

So there are a couple solutions to this problem.

Check to see if a timezone is set and, if it's not, inform the user to update the php.ini file and set the timezone. Not every OS will set that property or has a patch for it. I guess this might be checked and fixed in the install script.
Allow the user to update a setting by either updating a Pi Hole config file (setupVars.conf?) or a UI interface and then set the tz with date_default_timezone_set() at the beginning of the script. This would allow the user to specify a different tz for the admin vs another script they have running on the box. As far as I'm aware the only config file available is setupVars.conf. Maybe that can be populated during install based on the the OS's tz or add a step for the user to select it.

I don't think the TZ should be stored in the Admin code base since it's based on the tz of the log file the backend is generating. I'm not sure that putting the effort in to a Admin UI setting is worth it since it will only be set once and should probably be done during install.

das7pad · 2016-10-19T15:57:27Z

@akoebbe
How would you check for the timezone? Using date_defaul_timzone_get() will spam the logfile.
The php.ini will be the default version in most cases and as you have said, this will be global - not the best idea.
From the OS we can't get a propper timezone that can be used in php date_default_timezone_set(). And printing the options from php during the installation is quite the same as entering a search query/scrolling down the list at the first run of the admin page.

The option in the dashbord later on would be just a call on the 'firstrun page' - which verifys the input and prints the timezone into another file, the admin scripts are then calling. Printing into the code of data.php or auth.php would break the ability to update I think.

akoebbe · 2016-10-19T16:24:02Z

@das7pad Where would you suggest the value selected be stored?

akoebbe · 2016-10-19T16:26:27Z

@das7pad We could check for the timezone with the getter function you mentioned and use the '@' to suppress warning messages on that call.

das7pad · 2016-10-19T18:21:32Z

@akoebbe How about the php directory in a new file .timezone?

I am running Centos 7 and my Timezone is set to Europe/Berlin in timedatectl.
date +'%Z' outputs CEST
php -r 'echo @date_default_timezone_get();' outputs UTC as CEST is not a valid identifier in php for a timezone

PromoFaux and others added 2 commits October 1, 2016 16:24

Merge pull request #152 from pi-hole/devel

f112893

HOTFIX RELEASE 1.4.2

set default time zone for date

b73d6e0

this prevents basic error messages from php(-cgi) for not setting the timezone and then using UTC as default

das7pad added 4 commits October 9, 2016 03:04

Merge remote-tracking branch 'refs/remotes/pi-hole/master' into devel

4028a3f

use output of command date as datestring

9cd0f4b

this will imply the system time zone. command date and the given format are supported by the majority of linux distros

run date command right before log event

fb99587

use the date command for the other dates as well

0197ca7

trim against leading space from %k (range of ' 0' to '23') and strtolower for AM/PM that %p outputs as uppercase, while 'a' (-> am/pm) was used before

AzureMarker suggested changes Oct 9, 2016

View reviewed changes

keep recent format of date

91683a6

just output a 'T' insted of the Timezone

AzureMarker approved these changes Oct 9, 2016

View reviewed changes

AzureMarker added Bugfix Ready for Check and Merge labels Oct 9, 2016

AzureMarker merged commit eeb367f into pi-hole:devel Oct 13, 2016

This was referenced Oct 18, 2016

Revert "set default time zone for date" #170

Merged

Query log broken in 2.9.4 #169

Closed

[HOTFIX RELEASE] 1.4.3.1 #171

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set default time zone for date #154

set default time zone for date #154

das7pad commented Oct 4, 2016

PromoFaux commented Oct 4, 2016

PromoFaux commented Oct 4, 2016 •

edited by dschaper

AzureMarker commented Oct 8, 2016

AzureMarker commented Oct 8, 2016

das7pad commented Oct 9, 2016

AzureMarker commented Oct 9, 2016

AzureMarker Oct 9, 2016

das7pad Oct 9, 2016

AzureMarker commented Oct 9, 2016 •

edited by dschaper

PromoFaux commented Oct 18, 2016

akoebbe commented Oct 18, 2016

PromoFaux commented Oct 18, 2016

PromoFaux commented Oct 18, 2016

akoebbe commented Oct 18, 2016

das7pad commented Oct 19, 2016

akoebbe commented Oct 19, 2016

das7pad commented Oct 19, 2016

akoebbe commented Oct 19, 2016

akoebbe commented Oct 19, 2016

das7pad commented Oct 19, 2016

set default time zone for date #154

set default time zone for date #154

Conversation

das7pad commented Oct 4, 2016

PromoFaux commented Oct 4, 2016

PromoFaux commented Oct 4, 2016 • edited by dschaper

AzureMarker commented Oct 8, 2016

AzureMarker commented Oct 8, 2016

das7pad commented Oct 9, 2016

AzureMarker commented Oct 9, 2016

AzureMarker Oct 9, 2016

Choose a reason for hiding this comment

das7pad Oct 9, 2016

Choose a reason for hiding this comment

AzureMarker commented Oct 9, 2016 • edited by dschaper

PromoFaux commented Oct 18, 2016

akoebbe commented Oct 18, 2016

PromoFaux commented Oct 18, 2016

PromoFaux commented Oct 18, 2016

akoebbe commented Oct 18, 2016

das7pad commented Oct 19, 2016

akoebbe commented Oct 19, 2016

das7pad commented Oct 19, 2016

akoebbe commented Oct 19, 2016

akoebbe commented Oct 19, 2016

das7pad commented Oct 19, 2016

PromoFaux commented Oct 4, 2016 •

edited by dschaper

AzureMarker commented Oct 9, 2016 •

edited by dschaper