Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:25343
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Message-ID: <010a01c6bf80$0e0b2b90$0201a8c0@pc1>
To: <internals@lists.php.net>, "Pierre" <pierre.php@gmail.com>
References: <005301c6bec7$783e5c80$0201a8c0@pc1> <fe05d1540608130508n73b36be8qefc087f56898448c@mail.gmail.com> <006d01c6bedd$c86c7a00$0201a8c0@pc1> <fe05d1540608130715h4acd0758t4a690deb34fec3c6@mail.gmail.com>
Date: Mon, 14 Aug 2006 04:00:13 -0500
MIME-Version: 1.0
Content-Type: text/plain;
	charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] [PATCH] array_count_values bug: mishandling of numbers with leading whitespace
From: php_lists@realplain.com ("Matt W")

Hi Pierre,

*sigh*  I'm thinking I better go through the official channels (Bug report)
then and see what happens in case nobody else really sees this here.  (Since
that's what a regular user would do. :-)  Although after seeing the repeated
disregard for Bug #37846, I don't know.)  Have to see now that it's
Monday...  More below.

----- Original Message -----
From: "Pierre"
Sent: Sunday, August 13, 2006


> Hi,
>
> On 8/13/06, Matt W <php_lists@realplain.com> wrote:
>
> > Not reasonable or safe?  Then why were the other bugs fixed instead of
being
> > marked "Bogus" if array_count_values() was *supposed* to behave
differently
> > in 5?
>
> Safe as it will not break working php5 code. Changing this behavior
> now can break many things out there (for php 5 users, php4 users are
> out of my worries for this problem). What I mean is that we should
> first check the impact and see why this change has been done (I
> remember something like a valid reason, but did not get the time to
> verify).
>
> In short, it is just what I said in the previous discussion, such
> changes must be done carefully and with a huge amount of test cases
> (and stick to them). I'm not choosing one solution or another, only
> saying that the decision should be taken carefully and generally.

This "breaking things for PHP 5 users" concern doesn't make sense to me -- 
for normal changes, certainly, but for *bug fixes*?  I didn't know there was
so much concern when fixing bugs that it may break things for someone
relying on a *bug*. :-/  And you're also saying that you don't care if code
from PHP 4 no longer works the same?  Again, I point to the other bugs that
were fixed, *which were _accidentally_ caused by PHP 5's internal function
changes*.  I seriously doubt there was a valid reason for the behavior
change (after checking CVS changes; see below), and it should be documented
in the manual if there was (could've been forgotten, but still).

After a quick check of
http://cvs.php.net/viewvc.cgi/php-src/ext/standard/array.c?view=log I found
many examples (with other changes) to support my idea that this is all a
mistake of not doing a global search and replace of zend_hash_[find|update]
with zend_symtable_* when the "symtable" functions were added.  I'm shocked
that with such concern over not breaking things that *every* use of the
"hash" functions wasn't carefully checked to see what would break by
removing the HANDLE_NUMERIC() macro and moving it to the "symtable"
functions.  Oh, actually I don't think that would've been needed with a
simple search and replace in *all* files. :-)  I guess somebody didn't
realize the differences that I did when I first looked at them a couple
months ago...

Now, let's look at what I found in the changelog for array.c:

*) 1.231 (Jun 5 '03): "fix array_key_exists() from HANDLE_NUMERIC() changes"
(Came up with a "hack," zend_is_numeric_key(), before eventually changing to
the correct symtable function); and "need to go through this file and find
any stuff that relies on this change" (Obviously we know at least one place
that's still been missed until now ;-))

*) 1.235 (Jul 22 '03): "Use the new infrastructure of zend_symtable_*()"
(Oh, there's the correct array_key_exists() fix!)

*) 1.237 (Aug 4 '03): "Fix bug #24652 - Sterling, do you begin to think that
maybe it wasn't such a good idea?"  (Another switch to the correct
"symtable" function for array_flip().  And no, the idea was fine, just
should've done a search and replace.)

*) 1.273 (Aug 26 '04): "Fixed bug #29808 (array_count_values() breaks with
numeric strings)" (First of the wrong fixes (is_numeric_string()) that were
unfortunately not realized like the others (that eventually used
"symtable").  This was for 5.0.2/5.1, so must not have been intentional
(changing in 5.0) or with good reason.)

*) 1.297 (Apr 12 '05): "fix #30833 (array_count_values modifying input
array)" (More unnecessary code added as a result of not fixing the root of
the problem ("hash" instead of "symtable"), which is the majority of what my
patch removes.)

*) 1.327 (Oct 4 '05): "fix #34723 (array_count_values() strips leading
zeroes)" (The last of the mess.  And still didn't fix the problem when
leading zeros are preceded by whitespace.)

Seems pretty obvious...

> --Pierre

Matt