[CC'ing internals.]
On 01/16/2013 03:15 PM, Paul Taulborg wrote:
On Wed, Jan 16, 2013 at 4:21 PM, Nuno Lopes <nlop...@php.net> wrote:
On Thu, Jan 10, 2013 at 4:59 PM, Nuno Lopes <nlop...@php.net> wrote:
On 01/07/2013 07:27 AM, Paul Taulborg wrote:
I would love to write this patch, I'm all in favor of simplifying
this. :) We should probably get with derick (cc'd) on this as well, to
make sure something else obvious isn't being missed.
Let me know how you want me to proceed. Thanks :)
Paul,
For small changes submit bugs (and/or github pull requests) like you
did before.
For bigger changes, start an RFC at https://wiki.php.net/rfc
This will help focus the change, and provides a source for any doc
updates.
Since it is really just you and Derick looking at this area, be
prepared
to be a leader.
Don't forget to CC internals.
Chris
PS Note that Derick tends to ignore top-posted email.
Bug/patch submitted:
https://bugs.php.net/bug.php?id=63941
Will post on internals momentarily. Turns out removing
default_timezone entirely won't work due to the callback for ini_set,
but this isn't a major problem. We could probably remove it entirely
with a future patch, but it would require some other internal variable
somewhere along the line anyway.
I have another suggestion that avoids BC break. Pseudo-code:
guess_timezone():
if (timezone) return timezone
if (default_timezone) return default_timezone
return "UTC
OnUpdate ini option:
if (valid) {
free(default_timezone)
default_timezone = new
}
PHP's date_default_timezone_set():
if (valid) {
free(timezone)
timezone = new
} else {
timezone = NULL
}
I belive this causes no major BC break and enables efficient code.
Note that according to the manual, date_default_timezone_set() takes
precedence over the ini setting, and therefore we cannot set the
'timezone'
var in the ini handler (as I previously wrongly suggested).
Nuno
Nuno,
Unless I'm misunderstanding or missing something here, this is already
what I did in the patch submitted 4 days ago:
https://bugs.php.net/bug.php?id=63941
I'm still waiting for feedback and for the patch to be applied.
Thanks
Sorry for the delay, but I've been quite busy..
So, to answer your question: I don't think your patch implements the
pseudo-code I suggested above. In particular your patch change the
documented behaviour for intermixed ini_set/date_set_default_tz calls.
Nuno
I would recommend this be changed, and it would not conflict with the
documentation. First, the ini_set() does not say "this will not
override date_set_default_timezone()", even though that is the
functionality. A user could easily presume (wrongly), that both do the
same thing (and they should, for consistency). I see no logical reason
to have two values in the core, that do the same thing.
For instance, I could use date_set_default_timezone() in one spot,
then later ini_set, expecting it to override, but it doesn't. This is
an extra nuance for no gain. If the argument is "but I might want to
revert back to the ini value!", well, you can't currently without a
literal ini_get() and then another date_set_default_timezone() call.
This is no different than what my patch does, you would have to grab
the original value with ini_get, and then set it via EITHER method.
This simplifies the internal code handling and makes it more
consistent and makes the code a bit easier to follow with regards to
this particular subset of the date section.
If we're going to get in here and start optimizing code, we should do
so, not continue to throw band-aids on that do nothing in terms of
performance gains or code readability/manageability. That is the
reason I tackled this head-on in the way I did, because having both
values internally checked is not necessary. I cannot see any reason to
have two values internally to represent essentially the same thing. If
there is something I'm missing here regarding that, please let me
know.
Thanks
--
christopher.jo...@oracle.com http://twitter.com/ghrd
Newly updated, free PHP & Oracle book:
http://www.oracle.com/technetwork/topics/php/underground-php-oracle-manual-098250.html
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
