Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:34620
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: unknown (pb1.pair.com: domain php.net does not designate 82.94.239.7 as permitted sender)
Date: Wed, 9 Jan 2008 21:03:02 +0100 (CET)
To: Joe Orton <jorton@redhat.com>
cc: PHP Developers Mailing List <internals@lists.php.net>
In-Reply-To: <20080109141541.GA14571@redhat.com>
Message-ID: <alpine.DEB.0.98.0801092102560.4679@kossu.ez.no>
References: <20080109141541.GA14571@redhat.com>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=UTF-8
Subject: Re: [PHP-DEV] [PATCH] date/timelib: use system timezone database
From: derick@php.net (Derick Rethans)

On Wed, 9 Jan 2008, Joe Orton wrote:

> It's a bit of a maintenance headache for distributions when 
> packages include their own copy of the timezone database, since this 
> needs to be updated frequently.

There is a PECL extension to provide those updates:
http://pecl.php.net/package/timezonedb
It is a drop-in replacement to update the rules.

> I've prepared a patch to allow the timelib code to use the system 
> timezone database directly; would something like this be acceptable, any 
> thoughts?

Quite a few actually. 

> Notes:
> 
> 1) I've not implemented timelib_timezone_builtin_identifiers_list() here 
> since it doesn't seem to be used, is it there for third-party 
> extensions?  It could be implemented by iterating through the directory.

It's not used (anymore). However, there is a PHP function 
timezone_identifiers_list that uses the index information to enumerate 
all entries. Your patch prevents this function from working as both 
struct elements index_size and index are set to 0/null.

> 2) there's no general way that I can find to obtain the database 
> version, so I just invented a string here.

Not having the version creates another problem, as the PECL timezonedb 
extension will then *always* override the builtin database. The 
extension uses the version number to see if the extension provides a 
later rules set. With your patch, this extra check will not work 
correctly anymore if PHP's built-in version is newer (which would happen 
if PHP got upgraded).

Some other issues:

- This patchs allows accessing any file on the filesystem (because the 
  path is not sanitized).
- Opening a file is less quick than having the data in memory.
- The system's format of tzfiles does not necessarily have to be the 
  same as the one that PHP reads. On my (Debian) system the tz provided 
  data is already 64bit read. Once I update the extension to use this 
  extra data, reading the system tz files won't work anymore.
- It would be possible to use a "wrong set" of tzdata, resulting in bugs 
  like http://bugs.php.net/bug.php?id=35958

The reason to bundle the database is to have a operating system 
independent source of tzdata, which is something that this patch 
destroys. I am because of this, and all the other issues, quite against 
adding this patch - especially because there is already a solution for 
this.

regards,
Derick

-- 
Derick Rethans
http://derickrethans.nl | http://ezcomponents.org | http://xdebug.org