Hi,
I come across the following bug: https://bugs.php.net/bug.php?id=66834
With an extended ArrayObject instance, the behaviour of isset() and
empty() is as follows:
- Call
static::offsetExists() - If return value is truthy, then
a. isset() returns true
b. empty() returns false
c. spl_array_has_property() returns true
However, if the ArrayObject instance is not overridden:
- Fetch value (either via string or long key)
- If key is found, then:
a. isset() returns false if value isnull, true otherwise.
b. empty() returns true if value is truty, false otherwise.
c. spl_array_has_property() returns true
That's counterintuitive, so my patch addresses this by also calling
offsetGet() if applicable. I wanted to bring this to the attention of the
group first, because it may affect performance to achieve this goal:
https://github.com/php/php-src/pull/614
I've tried to contact Marcus Boerger, but I was told that he's been less
active lately. Let me know your thoughts. The patch targets 5.4 onwards.
--
Tjerk
On Fri, Mar 7, 2014 at 11:14 AM, Tjerk Meesters
tjerk.meesters@gmail.com wrote:
Hi,
I come across the following bug: https://bugs.php.net/bug.php?id=66834
With an extended ArrayObject instance, the behaviour of
isset()and
empty()is as follows:
- Call
static::offsetExists()- If return value is truthy, then
a. isset() returns true
b. empty() returns false
c. spl_array_has_property() returns trueHowever, if the ArrayObject instance is not overridden:
- Fetch value (either via string or long key)
- If key is found, then:
a. isset() returns false if value isnull, true otherwise.
b. empty() returns true if value is truty, false otherwise.
c. spl_array_has_property() returns trueThat's counterintuitive, so my patch addresses this by also calling
offsetGet()if applicable. I wanted to bring this to the attention of the
group first, because it may affect performance to achieve this goal:https://github.com/php/php-src/pull/614
I've tried to contact Marcus Boerger, but I was told that he's been less
active lately. Let me know your thoughts. The patch targets 5.4 onwards.
You should try to contact Etienne Kneuss, he'll be able to help.
He is often available on IRC.
Julien
On Fri, Mar 7, 2014 at 11:14 AM, Tjerk Meesters
tjerk.meesters@gmail.com wrote:Hi,
I come across the following bug: https://bugs.php.net/bug.php?id=66834
With an extended ArrayObject instance, the behaviour of
isset()and
empty()is as follows:
- Call
static::offsetExists()- If return value is truthy, then
a. isset() returns true
b. empty() returns false
c. spl_array_has_property() returns trueHowever, if the ArrayObject instance is not overridden:
- Fetch value (either via string or long key)
- If key is found, then:
a. isset() returns false if value isnull, true otherwise.
b. empty() returns true if value is truty, false otherwise.
c. spl_array_has_property() returns trueThat's counterintuitive, so my patch addresses this by also calling
offsetGet()if applicable. I wanted to bring this to the attention of
the
group first, because it may affect performance to achieve this goal:
I have always been annoyed by this dichotomy between offsetExists() which
is functionning like array_key_exists() and isset() which is returning
false on defined, null values. Especially since isset() calls
offsetExists() only.
I would argue that the correct way to fix it is to have isset()/empty()
call offsetGet() as well if it exists. So in that sense I feel like your
patch is not addressing the right issue. In a sense ArrayObject does a hack
to circumvent that in the first place, and you are trying to fix the hack
to make it look more like what is supposed to happen.
--
Etienne Kneuss
On Fri, Mar 7, 2014 at 11:14 AM, Tjerk Meesters
tjerk.meesters@gmail.com wrote:Hi,
I come across the following bug: https://bugs.php.net/bug.php?id=66834
With an extended ArrayObject instance, the behaviour of
isset()and
empty()is as follows:
- Call
static::offsetExists()- If return value is truthy, then
a. isset() returns true
b. empty() returns false
c. spl_array_has_property() returns trueHowever, if the ArrayObject instance is not overridden:
- Fetch value (either via string or long key)
- If key is found, then:
a. isset() returns false if value isnull, true otherwise.
b. empty() returns true if value is truty, false otherwise.
c. spl_array_has_property() returns trueThat's counterintuitive, so my patch addresses this by also calling
offsetGet()if applicable. I wanted to bring this to the attention of
the
group first, because it may affect performance to achieve this goal:I have always been annoyed by this dichotomy between offsetExists() which
is functionning likearray_key_exists()and isset() which is returning
false on defined, null values. Especially since isset() calls
offsetExists() only.I would argue that the correct way to fix it is to have isset()/empty()
call offsetGet() as well if it exists. So in that sense I feel like your
patch is not addressing the right issue. In a sense ArrayObject does a hack
to circumvent that in the first place, and you are trying to fix the hack
to make it look more like what is supposed to happen.
Apart from this underlying "design" issue, it is true an overridden version
of ArrayObject is not functionally equivalent to the original class. So it
seems worth fixing independently from this hack.
Etienne Kneuss
Hi Etienne,
Thanks for your input, much appreciated.
I have moved the decision up the chain as you suggested; the results can be
found here:
https://github.com/datibbaw/php-src/compare/php:PHP-5.4...bug66834a
It's suffering from a few memory leaks which can be fixed, but would this
be acceptable otherwise?
On Fri, Mar 7, 2014 at 11:14 AM, Tjerk Meesters
tjerk.meesters@gmail.com wrote:Hi,
I come across the following bug: https://bugs.php.net/bug.php?id=66834
With an extended ArrayObject instance, the behaviour of
isset()and
empty()is as follows:
- Call
static::offsetExists()- If return value is truthy, then
a. isset() returns true
b. empty() returns false
c. spl_array_has_property() returns trueHowever, if the ArrayObject instance is not overridden:
- Fetch value (either via string or long key)
- If key is found, then:
a. isset() returns false if value isnull, true otherwise.
b. empty() returns true if value is truty, false otherwise.
c. spl_array_has_property() returns trueThat's counterintuitive, so my patch addresses this by also calling
offsetGet()if applicable. I wanted to bring this to the attention
of the
group first, because it may affect performance to achieve this goal:I have always been annoyed by this dichotomy between offsetExists() which
is functionning likearray_key_exists()and isset() which is returning
false on defined, null values. Especially since isset() calls
offsetExists() only.I would argue that the correct way to fix it is to have isset()/empty()
call offsetGet() as well if it exists. So in that sense I feel like your
patch is not addressing the right issue. In a sense ArrayObject does a hack
to circumvent that in the first place, and you are trying to fix the hack
to make it look more like what is supposed to happen.Apart from this underlying "design" issue, it is true an overridden
version of ArrayObject is not functionally equivalent to the original
class. So it seems worth fixing independently from this hack.Etienne Kneuss
--
Tjerk
On Sat, Mar 8, 2014 at 8:27 AM, Tjerk Meesters tjerk.meesters@gmail.comwrote:
Hi Etienne,
Thanks for your input, much appreciated.
I have moved the decision up the chain as you suggested; the results can
be found here:https://github.com/datibbaw/php-src/compare/php:PHP-5.4...bug66834a
It's suffering from a few memory leaks which can be fixed,
They are fixed now and a new PR has been opened:
https://github.com/php/php-src/pull/616
but would this be acceptable otherwise?
On Fri, Mar 7, 2014 at 11:14 AM, Tjerk Meesters
tjerk.meesters@gmail.com wrote:Hi,
I come across the following bug:
https://bugs.php.net/bug.php?id=66834With an extended ArrayObject instance, the behaviour of
isset()and
empty()is as follows:
- Call
static::offsetExists()- If return value is truthy, then
a. isset() returns true
b. empty() returns false
c. spl_array_has_property() returns trueHowever, if the ArrayObject instance is not overridden:
- Fetch value (either via string or long key)
- If key is found, then:
a. isset() returns false if value isnull, true otherwise.
b. empty() returns true if value is truty, false otherwise.
c. spl_array_has_property() returns trueThat's counterintuitive, so my patch addresses this by also calling
offsetGet()if applicable. I wanted to bring this to the attention
of the
group first, because it may affect performance to achieve this goal:I have always been annoyed by this dichotomy between offsetExists()
which is functionning likearray_key_exists()and isset() which is
returning false on defined, null values. Especially since isset() calls
offsetExists() only.I would argue that the correct way to fix it is to have isset()/empty()
call offsetGet() as well if it exists. So in that sense I feel like your
patch is not addressing the right issue. In a sense ArrayObject does a hack
to circumvent that in the first place, and you are trying to fix the hack
to make it look more like what is supposed to happen.Apart from this underlying "design" issue, it is true an overridden
version of ArrayObject is not functionally equivalent to the original
class. So it seems worth fixing independently from this hack.Etienne Kneuss
--
Tjerk
--
Tjerk
On Sat, Mar 8, 2014 at 4:56 AM, Tjerk Meesters tjerk.meesters@gmail.comwrote:
On Sat, Mar 8, 2014 at 8:27 AM, Tjerk Meesters <tjerk.meesters@gmail.com
wrote:
Hi Etienne,
Thanks for your input, much appreciated.
I have moved the decision up the chain as you suggested; the results can
be found here:https://github.com/datibbaw/php-src/compare/php:PHP-5.4...bug66834a
It's suffering from a few memory leaks which can be fixed,
They are fixed now and a new PR has been opened:
https://github.com/php/php-src/pull/616
but would this be acceptable otherwise?
On Fri, Mar 7, 2014 at 11:14 AM, Tjerk Meesters
tjerk.meesters@gmail.com wrote:Hi,
I come across the following bug:
https://bugs.php.net/bug.php?id=66834With an extended ArrayObject instance, the behaviour of
isset()
and
empty()is as follows:
- Call
static::offsetExists()- If return value is truthy, then
a. isset() returns true
b. empty() returns false
c. spl_array_has_property() returns trueHowever, if the ArrayObject instance is not overridden:
- Fetch value (either via string or long key)
- If key is found, then:
a. isset() returns false if value isnull, true otherwise.
b. empty() returns true if value is truty, false otherwise.
c. spl_array_has_property() returns trueThat's counterintuitive, so my patch addresses this by also calling
offsetGet()if applicable. I wanted to bring this to the attention
of the
group first, because it may affect performance to achieve this goal:I have always been annoyed by this dichotomy between offsetExists()
which is functionning likearray_key_exists()and isset() which is
returning false on defined, null values. Especially since isset() calls
offsetExists() only.I would argue that the correct way to fix it is to have isset()/empty()
call offsetGet() as well if it exists. So in that sense I feel like
your
patch is not addressing the right issue. In a sense ArrayObject does a
hack
to circumvent that in the first place, and you are trying to fix the
hack
to make it look more like what is supposed to happen.Apart from this underlying "design" issue, it is true an overridden
version of ArrayObject is not functionally equivalent to the original
class. So it seems worth fixing independently from this hack.Etienne Kneuss
--
Tjerk
--
Tjerk
Could you actually make that into a sane macro or would that just be more
trouble than it's worth?
On Sat, Mar 8, 2014 at 10:56 AM, Tjerk Meesters tjerk.meesters@gmail.comwrote:
On Sat, Mar 8, 2014 at 8:27 AM, Tjerk Meesters <tjerk.meesters@gmail.com
wrote:
Hi Etienne,
Thanks for your input, much appreciated.
I have moved the decision up the chain as you suggested; the results can
be found here:https://github.com/datibbaw/php-src/compare/php:PHP-5.4...bug66834a
It's suffering from a few memory leaks which can be fixed,
They are fixed now and a new PR has been opened:
Hi Etienne,
Are you fine with the changes in the PR?
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
On Sat, Mar 8, 2014 at 10:56 AM, Tjerk Meesters tjerk.meesters@gmail.comwrote:
On Sat, Mar 8, 2014 at 8:27 AM, Tjerk Meesters <tjerk.meesters@gmail.com
wrote:
Hi Etienne,
Thanks for your input, much appreciated.
I have moved the decision up the chain as you suggested; the results can
be found here:https://github.com/datibbaw/php-src/compare/php:PHP-5.4...bug66834a
It's suffering from a few memory leaks which can be fixed,
They are fixed now and a new PR has been opened:
Hi Etienne,
Are you fine with the changes in the PR?
Two things:
-
Where is this planned to be merged? Is it really 5.4 ? As is the PR is
no longer a simple bug fix and while I agree it should be changed that way,
it might introduce hard-to-track BC breaks. -
Seems to me that with this PR, part of the hack present in ArrayObject
in the has_dimension handler is no longer necessary, as it might be doing
to value-check twice now.
Best,
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
--
Etienne Kneuss
http://www.colder.ch
Hi Etienne,
On Sat, Mar 8, 2014 at 10:56 AM, Tjerk Meesters <tjerk.meesters@gmail.com
wrote:
On Sat, Mar 8, 2014 at 8:27 AM, Tjerk Meesters <tjerk.meesters@gmail.com
wrote:
Hi Etienne,
Thanks for your input, much appreciated.
I have moved the decision up the chain as you suggested; the results
can
be found here:https://github.com/datibbaw/php-src/compare/php:PHP-5.4...bug66834a
It's suffering from a few memory leaks which can be fixed,
They are fixed now and a new PR has been opened:
Hi Etienne,
Are you fine with the changes in the PR?
Two things:
- Where is this planned to be merged? Is it really 5.4 ? As is the PR is
no longer a simple bug fix and while I agree it should be changed that way,
it might introduce hard-to-track BC breaks.
After a short discussion on IRC I'm moving the branch to target 5.6 instead.
- Seems to me that with this PR, part of the hack present in ArrayObject
in the has_dimension handler is no longer necessary, as it might be doing
to value-check twice now.
I'll start looking into this now. Thanks for the heads up :)
Best,
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu--
Etienne Kneuss
http://www.colder.ch
--
Tjerk
On Mon, Mar 10, 2014 at 9:58 PM, Tjerk Meesters tjerk.meesters@gmail.comwrote:
Hi Etienne,
On Sat, Mar 8, 2014 at 10:56 AM, Tjerk Meesters <
tjerk.meesters@gmail.com> wrote:On Sat, Mar 8, 2014 at 8:27 AM, Tjerk Meesters <
tjerk.meesters@gmail.com>wrote:Hi Etienne,
Thanks for your input, much appreciated.
I have moved the decision up the chain as you suggested; the results
can
be found here:https://github.com/datibbaw/php-src/compare/php:PHP-5.4...bug66834a
It's suffering from a few memory leaks which can be fixed,
They are fixed now and a new PR has been opened:
Hi Etienne,
Are you fine with the changes in the PR?
Two things:
- Where is this planned to be merged? Is it really 5.4 ? As is the PR is
no longer a simple bug fix and while I agree it should be changed that way,
it might introduce hard-to-track BC breaks.After a short discussion on IRC I'm moving the branch to target 5.6
instead.
- Seems to me that with this PR, part of the hack present in ArrayObject
in the has_dimension handler is no longer necessary, as it might be doing
to value-check twice now.I'll start looking into this now. Thanks for the heads up :)
I've updated and cleaned up ext/spl; the most notable change is code
removal according to a changed definition of zend_object_has_dimension_t:
typedef int (*zend_object_has_dimension_t)(zval *object, zval *member
TSRMLS_DC);
PR: https://github.com/php/php-src/pull/621
Let me know if you see any other issues; I'm fine with reverting the
typedef change if that could be an issue.
Best,
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu--
Etienne Kneuss
http://www.colder.ch--
Tjerk
--
Tjerk
Hi Tjerk,
I was playing with ZF2 and getting segfaults for Mvc tests (for example
phpunit ZendTest/Mvc/ApplicationTest.php). It's only on 5.6 and master
branch. Think that it might be because of this patch:
0 zend_call_method zend_interfaces.c 35 0xae17fd
1 spl_array_read_dimension_ex spl_array.c 389 0x87179b
2 spl_array_has_dimension_ex spl_array.c 655 0x87273d
3 spl_array_has_dimension spl_array.c 675 0x8727d1
4 zend_isset_isempty_dim_prop_obj_handler_SPEC_CV_CV
zend_vm_execute.h 40983 0xb817a8
5 ZEND_ISSET_ISEMPTY_DIM_OBJ_SPEC_CV_CV_HANDLER zend_vm_execute.h
41039 0xb819c1
6 execute_ex zend_vm_execute.h 363 0xb097cb
7 zend_execute zend_vm_execute.h 388 0xb098b1
8 zend_call_function zend_execute_API.c 952 0xaa6faa
9 zend_call_method zend_interfaces.c 97 0xae1bea
10 spl_array_read_dimension_ex spl_array.c 389 0x87179b
11 spl_array_has_dimension_ex spl_array.c 655 0x87273d
12 spl_array_has_dimension spl_array.c 675 0x8727d1
13 zend_isset_isempty_dim_prop_obj_handler_SPEC_CV_CV
zend_vm_execute.h 40983 0xb817a8
14 ZEND_ISSET_ISEMPTY_DIM_OBJ_SPEC_CV_CV_HANDLER zend_vm_execute.h
41039 0xb819c1
15 execute_ex zend_vm_execute.h 363 0xb097cb
16 zend_execute zend_vm_execute.h 388 0xb098b1
17 zend_call_function zend_execute_API.c 952 0xaa6faa
18 zend_call_method zend_interfaces.c 97 0xae1bea
19 spl_array_read_dimension_ex spl_array.c 389 0x87179b
20 spl_array_has_dimension_ex spl_array.c 655 0x87273d
... <More>
Let me know if you can't recreate it
Thanks
Jakub
On Mon, Mar 10, 2014 at 4:20 PM, Tjerk Meesters tjerk.meesters@gmail.comwrote:
On Mon, Mar 10, 2014 at 9:58 PM, Tjerk Meesters <tjerk.meesters@gmail.com
wrote:
Hi Etienne,
On Mon, Mar 10, 2014 at 1:43 PM, Ferenc Kovacs tyra3l@gmail.com
wrote:On Sat, Mar 8, 2014 at 10:56 AM, Tjerk Meesters <
tjerk.meesters@gmail.com> wrote:On Sat, Mar 8, 2014 at 8:27 AM, Tjerk Meesters <
tjerk.meesters@gmail.com>wrote:Hi Etienne,
Thanks for your input, much appreciated.
I have moved the decision up the chain as you suggested; the results
can
be found here:https://github.com/datibbaw/php-src/compare/php:PHP-5.4...bug66834a
It's suffering from a few memory leaks which can be fixed,
They are fixed now and a new PR has been opened:
Hi Etienne,
Are you fine with the changes in the PR?
Two things:
- Where is this planned to be merged? Is it really 5.4 ? As is the PR
is
no longer a simple bug fix and while I agree it should be changed that
way,
it might introduce hard-to-track BC breaks.After a short discussion on IRC I'm moving the branch to target 5.6
instead.
- Seems to me that with this PR, part of the hack present in
ArrayObject
in the has_dimension handler is no longer necessary, as it might be
doing
to value-check twice now.I'll start looking into this now. Thanks for the heads up :)
I've updated and cleaned up ext/spl; the most notable change is code
removal according to a changed definition of zend_object_has_dimension_t:typedef int (*zend_object_has_dimension_t)(zval *object, zval *member
TSRMLS_DC);PR: https://github.com/php/php-src/pull/621
Let me know if you see any other issues; I'm fine with reverting the
typedef change if that could be an issue.Best,
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu--
Etienne Kneuss
http://www.colder.ch--
Tjerk
--
Tjerk
Hi,
Hi Tjerk,
I was playing with ZF2 and getting segfaults for Mvc tests (for example
phpunit ZendTest/Mvc/ApplicationTest.php). It's only on 5.6 and master
branch. Think that it might be because of this patch:0 zend_call_method zend_interfaces.c 35 0xae17fd
1 spl_array_read_dimension_ex spl_array.c 389 0x87179b
2 spl_array_has_dimension_ex spl_array.c 655 0x87273d
3 spl_array_has_dimension spl_array.c 675 0x8727d1
4 zend_isset_isempty_dim_prop_obj_handler_SPEC_CV_CV
zend_vm_execute.h 40983 0xb817a8
5 ZEND_ISSET_ISEMPTY_DIM_OBJ_SPEC_CV_CV_HANDLER zend_vm_execute.h
41039 0xb819c1
6 execute_ex zend_vm_execute.h 363 0xb097cb
7 zend_execute zend_vm_execute.h 388 0xb098b1
8 zend_call_function zend_execute_API.c 952 0xaa6faa
9 zend_call_method zend_interfaces.c 97 0xae1bea
10 spl_array_read_dimension_ex spl_array.c 389 0x87179b
11 spl_array_has_dimension_ex spl_array.c 655 0x87273d
12 spl_array_has_dimension spl_array.c 675 0x8727d1
13 zend_isset_isempty_dim_prop_obj_handler_SPEC_CV_CV
zend_vm_execute.h 40983 0xb817a8
14 ZEND_ISSET_ISEMPTY_DIM_OBJ_SPEC_CV_CV_HANDLER
zend_vm_execute.h 41039 0xb819c1
15 execute_ex zend_vm_execute.h 363 0xb097cb
16 zend_execute zend_vm_execute.h 388 0xb098b1
17 zend_call_function zend_execute_API.c 952 0xaa6faa
18 zend_call_method zend_interfaces.c 97 0xae1bea
19 spl_array_read_dimension_ex spl_array.c 389 0x87179b
20 spl_array_has_dimension_ex spl_array.c 655 0x87273d
... <More>
Yeah, it causes infinite recursion because of this:
public function offsetGet($name)
{
if (isset($this[$name])) {
return parent::offsetGet($name);
}
return null;
}
Doing this "detaches" the call chain and enters it again later without any
context to track a potential recursive call. I'm not sure whether it can be
fixed other than not doing this.
I've authored a (bc compatible) ZF2 fix here:
https://github.com/zendframework/zf2/pull/6096
Let me know if you can't recreate it
Thanks
Jakub
On Mon, Mar 10, 2014 at 4:20 PM, Tjerk Meesters tjerk.meesters@gmail.comwrote:
On Mon, Mar 10, 2014 at 9:58 PM, Tjerk Meesters <tjerk.meesters@gmail.com
wrote:
Hi Etienne,
On Mon, Mar 10, 2014 at 1:43 PM, Ferenc Kovacs tyra3l@gmail.com
wrote:On Sat, Mar 8, 2014 at 10:56 AM, Tjerk Meesters <
tjerk.meesters@gmail.com> wrote:On Sat, Mar 8, 2014 at 8:27 AM, Tjerk Meesters <
tjerk.meesters@gmail.com>wrote:Hi Etienne,
Thanks for your input, much appreciated.
I have moved the decision up the chain as you suggested; the
results
can
be found here:https://github.com/datibbaw/php-src/compare/php:PHP-5.4...bug66834a
It's suffering from a few memory leaks which can be fixed,
They are fixed now and a new PR has been opened:
Hi Etienne,
Are you fine with the changes in the PR?
Two things:
- Where is this planned to be merged? Is it really 5.4 ? As is the PR
is
no longer a simple bug fix and while I agree it should be changed that
way,
it might introduce hard-to-track BC breaks.After a short discussion on IRC I'm moving the branch to target 5.6
instead.
- Seems to me that with this PR, part of the hack present in
ArrayObject
in the has_dimension handler is no longer necessary, as it might be
doing
to value-check twice now.I'll start looking into this now. Thanks for the heads up :)
I've updated and cleaned up ext/spl; the most notable change is code
removal according to a changed definition of zend_object_has_dimension_t:typedef int (*zend_object_has_dimension_t)(zval *object, zval *member
TSRMLS_DC);PR: https://github.com/php/php-src/pull/621
Let me know if you see any other issues; I'm fine with reverting the
typedef change if that could be an issue.Best,
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu--
Etienne Kneuss
http://www.colder.ch--
Tjerk
--
Tjerk
--
Tjerk