Re: [PATCH] array_count_values bug: mishandling of numbers with leading whitespace

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