Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:73986 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 75961 invoked from network); 6 May 2014 23:33:29 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 6 May 2014 23:33:29 -0000 Authentication-Results: pb1.pair.com smtp.mail=tjerk.meesters@gmail.com; spf=pass; sender-id=pass Authentication-Results: pb1.pair.com header.from=tjerk.meesters@gmail.com; sender-id=pass Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.220.174 as permitted sender) X-PHP-List-Original-Sender: tjerk.meesters@gmail.com X-Host-Fingerprint: 209.85.220.174 mail-vc0-f174.google.com Received: from [209.85.220.174] ([209.85.220.174:36026] helo=mail-vc0-f174.google.com) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id FD/70-07044-84179635 for ; Tue, 06 May 2014 19:33:29 -0400 Received: by mail-vc0-f174.google.com with SMTP id ib6so259530vcb.33 for ; Tue, 06 May 2014 16:33:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=e81Ipp7CakTHVdP7QfCy+8T4GAc9XK/TFrA3tjWjgks=; b=eQxvP+9Jw45mPfs1HE4OyiD1CmjgpK7dizoFPH5CFzqUiaJDS2u2V8wVZv+wCtQXL8 3vbqHz60kcfbXuKQYSdaCpSo8RXYrnbCMbh0IEcHrnp/HLIFW89J4Ssq1tznT97VQYH5 XtdhMNsuRrTtI3DbVGQw2nktau26bHI7ciQ50HJZr4gd43DjpnNnm3KfL85QZ4ebk04Z SfOxZfQGFwzOBg3Vl1+r92VVGjwyYlb0xhCpLC1b/12Ez1mGIREA5FIDXMpMYED8dXRE db6EM44H3YVwGduwmbH9P5jwmMRcKIaoyJfiNz5TKPizoDzXgEc4vpNrG1VQO0p3TnFv wGrw== MIME-Version: 1.0 X-Received: by 10.52.116.101 with SMTP id jv5mr29844378vdb.11.1399419206272; Tue, 06 May 2014 16:33:26 -0700 (PDT) Sender: tjerk.meesters@gmail.com Received: by 10.58.133.84 with HTTP; Tue, 6 May 2014 16:33:26 -0700 (PDT) In-Reply-To: References: Date: Wed, 7 May 2014 07:33:26 +0800 X-Google-Sender-Auth: 1fpXBdcjK_qmIO4xd9n5vVD9Sp0 Message-ID: To: Dmitry Stogov Cc: Julien Pauli , PHP Internals Content-Type: multipart/alternative; boundary=bcaec548a9b7ae8ce904f8c3ace8 Subject: Re: 5dee3c11 break From: datibbaw@php.net (Tjerk Anne Meesters) --bcaec548a9b7ae8ce904f8c3ace8 Content-Type: text/plain; charset=UTF-8 Hi, On Tue, May 6, 2014 at 8:43 PM, Dmitry Stogov wrote: > zend_std_has_dimension() doesn't know what (check_empty == 2) means. > > check_empty == 0 - ISSET => we don't need to call offsetGet() and relay on > offsetExists() return value. > check_empty == 1 - ISEMPTY => we should call offsetGet() after > offsetExists(). > > NULL values should be handled by offsetExists(). > Okay, I suppose it's fair to make its behaviour the same as for zend_std :) I'll make the necessary changes by tonight for 5.6 and master. > > Thanks. Dmitry. > > > On Tue, May 6, 2014 at 4:36 PM, Tjerk Anne Meesters wrote: > >> >> >> On Tue, May 6, 2014 at 7:50 PM, Dmitry Stogov wrote: >> >>> Ah, I didn't get what did you mean by (check_empty == 2), but they >>> probably should be changed into (check_empty != 1) >>> >> >> When I said mostly, perhaps I should have mentioned that the isset() >> behaviour was deliberately updated as well; in other words, the following >> works as expected: >> >> $x['foo'] = null; >> isset($x['foo']); // false >> >> After taking a closer look at zend_std_has_dimension() it seems that both >> isset() and empty() behave as if empty() was always used; of course, if >> this behaviour was translated into spl_array.c, the infinite recursion >> would still be an issue. So, at this point I'm unsure what the proper >> action point should be. >> >> >>> >>> >>> It fixes the problem. >>> Could you please verify and commit into all relevant branches. >>> >>> Thanks. Dmitry. >>> >>> --- a/ext/spl/spl_array.c >>> +++ b/ext/spl/spl_array.c >>> @@ -603,7 +603,7 @@ static int spl_array_has_dimension_ex(int >>> check_inherited, zval *object, zval *o >>> >>> if (rv && zend_is_true(rv TSRMLS_CC)) { >>> zval_ptr_dtor(&rv); >>> - if (check_empty == 2) { >>> + if (check_empty != 1) { >>> return 1; >>> } else if (intern->fptr_offset_get) { >>> value = spl_array_read_dimension_ex(1, >>> object, offset, BP_VAR_R TSRMLS_CC); >>> @@ -622,7 +622,7 @@ static int spl_array_has_dimension_ex(int >>> check_inherited, zval *object, zval *o >>> switch(Z_TYPE_P(offset)) { >>> case IS_STRING: >>> if (zend_symtable_find(ht, >>> Z_STRVAL_P(offset), Z_STRLEN_P(offset)+1, (void **) &tmp) != FAILURE) { >>> - if (check_empty == 2) { >>> + if (check_empty != 1) { >>> return 1; >>> } >>> } else { >>> @@ -639,7 +639,7 @@ static int spl_array_has_dimension_ex(int >>> check_inherited, zval *object, zval *o >>> index = Z_LVAL_P(offset); >>> } >>> if (zend_hash_index_find(ht, index, >>> (void **)&tmp) != FAILURE) { >>> - if (check_empty == 2) { >>> + if (check_empty != 1) { >>> return 1; >>> } >>> } else { >>> >>> >>> >>> On Tue, May 6, 2014 at 3:29 PM, Tjerk Anne Meesters wrote: >>> >>>> Hi, >>>> >>>> >>>> On Tue, May 6, 2014 at 7:25 PM, Dmitry Stogov wrote: >>>> >>>>> I didn't review you patch careful, but why do you need to call >>>>> offsetGet() when offsetExists() must be enough? >>>>> is it for empty()? >>>>> >>>> >>>> Mostly for empty(), yes. It fixes the expected empty() behaviour for >>>> `$a['foo'] == 0` when using ArrayObject. This is further explained in this >>>> bug report: https://bugs.php.net/bug.php?id=66834 >>>> >>>> >>>>> Then you probably should implement it a way similar to >>>>> zend_std_has_dimension() in Zend/zend_object_handlers.c. >>>>> call offsetExists() and then offsetGet() if necessary. >>>>> >>>> >>>> That's already the case :) >>>> >>>> http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_object_handlers.c#719 >>>> >>>> >>>>> >>>>> Thanks. Dmitry. >>>>> >>>>> >>>>> On Tue, May 6, 2014 at 2:31 PM, Tjerk Anne Meesters wrote: >>>>> >>>>>> Hi Dmitry, >>>>>> >>>>>> On Tue, May 6, 2014 at 4:11 PM, Dmitry Stogov wrote: >>>>>> >>>>>>> Hi Tjerk, >>>>>>> >>>>>>> your commit broke the code that worked fine before (still works in >>>>>>> 5.5 but broken in 5.6 and above). >>>>>>> It leads into infinity recursion until stack overflow. >>>>>>> >>>>>>> It must be fixed or reverted. >>>>>>> >>>>>> >>>>>> This has been mentioned by Jakub before and a fix to ZF2 has already >>>>>> been merged: >>>>>> >>>>>> https://github.com/zendframework/zf2/pull/6096 >>>>>> >>>>>> The previous code and my patch basically cannot coexist; it used to >>>>>> work in 5.6 before, but only by the "virtue" of an unfortunate >>>>>> implementation. >>>>>> >>>>>> I believe this is not the only 5.6 issue that ZF2 is dealing with, >>>>>> but if you feel that this breaks too many things for a 5.x release I >>>>>> suppose we can revert it in PHP-5.6 and keep it for PHP-6? >>>>>> >>>>>> Let me know. >>>>>> >>>>>> >>>>>>> Thanks. Dmitry. >>>>>>> >>>>>>> >>>>>> class Parameters extends ArrayObject { >>>>>>> public function __construct(array $values = null) { >>>>>>> if (null === $values) { >>>>>>> $values = array(); >>>>>>> } >>>>>>> parent::__construct($values, ArrayObject::ARRAY_AS_PROPS); >>>>>>> } >>>>>>> public function offsetGet($name) { >>>>>>> if (isset($this[$name])) { >>>>>>> return parent::offsetGet($name); >>>>>>> } >>>>>>> return null; >>>>>>> } >>>>>>> } >>>>>>> $x = new Parameters(); >>>>>>> var_dump($x['foo']); >>>>>>> $x['foo'] = 'bar'; >>>>>>> var_dump($x['foo']); >>>>>>> ?> >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> -- >>>>>> Tjerk >>>>>> >>>>> >>>>> >>>> >>>> >>>> -- >>>> -- >>>> Tjerk >>>> >>> >>> >> >> >> -- >> -- >> Tjerk >> > > -- -- Tjerk --bcaec548a9b7ae8ce904f8c3ace8--