All of the identified issues can be resolved and none of them
represent a major challenge to address. However, if there is no
consensus to put this in the near future (which at this point is 5.3),
I have hard time justifying spending further time on this. The
original patch that was posted, that did break BC was far simpler and
featureless, the changes since (which took a fair amount of work) were
specifically made to address some of the main concerns that were on
the list. I feel what is on the table right now is pretty close to
what a final product could be, to have a vote on it. If decision is
made to proceed within a practical release schedule, then suffice to
say that I'd be more then happy to put further time to address the
minor issues indicated.
On 7-Jul-09, at 7:07 PM, Paul Biggar wrote:
2009/7/7 Johannes Schlüter <johan...@php.net>:
On Mon, 2009-07-06 at 20:52 -0400, Ilia Alshanetsky wrote:
Last week or so there was a fairly detailed discussion on the
internals list regarding type hinting based on my original patch.
Having an "old" 5.3 extension with a typehint expecting an array
arg_info.array_type_hint will be set to 1.
The newly compiled engine with this patch will then do
+ /* existing type already matches the hint or forced
type */
+ if (Z_TYPE_P(arg) == cur_arg_info->array_type_hint
|| Z_TYPE_P(arg) == (cur_arg_info->array_type_hint ^ (1<<7))) {
as it's main type check, but Z_TYPE_P(arg) will be IS_ARRAY (5) which
doesn't match the 1 provided by the old extension, other checks in
there
will fail too, so the valid param will be rejected whereas an integer
(IS_LONG 1) will be accepted where the extension expects an array.
I raised this in my review, to which Ilia replied "It should be fine"
(http://news.php.net/php.internals/44707). I would not have thought it
would be fine.
I had been thinking that Ilia would have to hack it to make 1 mean
array in this case, which would be ugly, but workable. Based on the
arguments in this thread, I believe it shouldn't go into 5.3 at all.
Are we allowed break the ABI for 5.4 (I would think so, but amn't
sure).
Overall, I'm very disappointed with the way this has been conducted.
When reviews were posted they are not replied to (Stas posted
http://news.php.net/php.internals/44710, I posted
http://news.php.net/php.internals/44706, and I dont see any replies
except a cursory response to mine). Furthermore:
- the RFC process has been wilfully ignored (despite multiple
requests)
- a vote was asked for when Lukas was still trying to discuss his
proposal
- the vote was take it or leave it
- there has been a general attitude of "throwing the toys out of
the pram"
I am mostly for the patch, and I 100% support the idea. However, I
feel I have to vote against it, and urge others to do the same, until
the entire mess is rectified.
Ilia, I respect the work you have put into this, but I would ask you
to withdraw the patch and the vote until these things have been sorted
out.
-1
Thanks,
Paul
--
Paul Biggar
paul.big...@gmail.com
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
