Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:73974 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 83347 invoked from network); 6 May 2014 12:43:06 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 6 May 2014 12:43:06 -0000 Authentication-Results: pb1.pair.com smtp.mail=dmitry@zend.com; spf=permerror; sender-id=unknown Authentication-Results: pb1.pair.com header.from=dmitry@zend.com; sender-id=unknown Received-SPF: error (pb1.pair.com: domain zend.com from 209.85.212.177 cause and error) X-PHP-List-Original-Sender: dmitry@zend.com X-Host-Fingerprint: 209.85.212.177 mail-wi0-f177.google.com Received: from [209.85.212.177] ([209.85.212.177:54962] helo=mail-wi0-f177.google.com) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id 1A/04-53252-7D8D8635 for ; Tue, 06 May 2014 08:43:04 -0400 Received: by mail-wi0-f177.google.com with SMTP id f8so838905wiw.10 for ; Tue, 06 May 2014 05:43:00 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=ji9nJy3oth4Rh+Vfu9no02RyDgam6AmGeK6WeFv+E8I=; b=NIac+C/tzBqtfSJ09PWAO29D2CXr5v0215811Y7P7vQ/p4acPQ/3/njd6vS7jSt1Sq 845+n/h1P9eJAHnrwUuVxKvSAxCo5lVUupUfUZBXaiO8V/asXDgohCBgHFpzdtQNfj3Y fAe3EqvU7qmnCX509UqmPNdySdFLokJG0qXBu2MFZS5cJKEwFzdOdMlxVUpXPi8lk6PL pyI8Yt8uVADBYG7nPktRaNDM0E+R0RF47uLTTMa5wV2UKxnWQLI3xlLLpoiEIMdZ4PLx SIpYn6sc434jpCkqH6cSNstXAtD3fF8k3ReyvDWgokzM9bj2Gy8xSSJBudZst56EW2G/ Ig4Q== X-Gm-Message-State: ALoCoQmmmaTlInSPrXKl+twwiQq5RsKYlo19WJF2AoiSqQJmFjKhD4amPq8SoodXkT+H6kSypRcdzpU8TQswAr1xHM5SRE/R0mYdkPEqE5n+lnPE59NAFq3QhsqYtA2+y51TurNU7T1R MIME-Version: 1.0 X-Received: by 10.194.24.194 with SMTP id w2mr32933502wjf.25.1399380180474; Tue, 06 May 2014 05:43:00 -0700 (PDT) Received: by 10.227.234.6 with HTTP; Tue, 6 May 2014 05:43:00 -0700 (PDT) In-Reply-To: References: Date: Tue, 6 May 2014 16:43:00 +0400 Message-ID: To: Tjerk Anne Meesters Cc: Julien Pauli , PHP Internals Content-Type: multipart/alternative; boundary=047d7b450c5690302004f8ba968c Subject: Re: 5dee3c11 break From: dmitry@zend.com (Dmitry Stogov) --047d7b450c5690302004f8ba968c Content-Type: text/plain; charset=UTF-8 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(). 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 > --047d7b450c5690302004f8ba968c--