Hi internals,
Would someone with Zend karma be so kind to merge the following into 5.6:
https://github.com/php/php-src/pull/621
It makes isset() and empty() call offsetGet() on objects that extend
ArrayObject if offsetExists() returns TRUE.
This work has been checked off with Etienne; if there are any issues with
it, please let me know.
--
Tjerk
Hi!
https://github.com/php/php-src/pull/621
It makes isset() and empty() call offsetGet() on objects that extend
ArrayObject if offsetExists() returns TRUE.
Looks at this patch, isn't that what check_empty parameter was supposed
to be doing? Why doesn't it work and it needs to be moved to opcodes?
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi Stas,
Hi!
https://github.com/php/php-src/pull/621
It makes isset() and empty() call offsetGet() on objects that extend
ArrayObject if offsetExists() returns TRUE.Looks at this patch, isn't that what check_empty parameter was supposed
to be doing? Why doesn't it work and it needs to be moved to opcodes?
Yes, that's probably what it was supposed to do, but as you can see from bug 66834 that's clearly not the case. I had earlier approached the problem purely from SPL standpoint (see linked PR) but Etienne said that it would be a better idea to tackle this issue further up the chain so to speak :)
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi!
Yes, that's probably what it was supposed to do, but as you can see
from bug 66834 that's clearly not the case. I had earlier approached
the problem purely from SPL standpoint (see linked PR) but Etienne
said that it would be a better idea to tackle this issue further up
the chain so to speak :)
I think we need to first find out why that code did not work, instead of
ripping out the code that already was supposed to do exactly what this
bug is saying and replacing it with other code. At least we need to know
the reason why that did not work. So please do not merge this patch
until we know what's going on there.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
On Thu, Mar 20, 2014 at 9:16 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:
Hi!
Yes, that's probably what it was supposed to do, but as you can see
from bug 66834 that's clearly not the case. I had earlier approached
the problem purely from SPL standpoint (see linked PR) but Etienne
said that it would be a better idea to tackle this issue further up
the chain so to speak :)I think we need to first find out why that code did not work, instead of
ripping out the code that already was supposed to do exactly what this
bug is saying and replacing it with other code.
Here's my earlier work on this: https://github.com/php/php-src/pull/614
It currently "doesn't work" because isset()/empty() work like
array_key_exists() on objects that extend ArrayObject, which is in some
cases not the desired behaviour; see also:
http://lxr.php.net/xref/PHP_5_6/ext/spl/spl_array.c#604
If 'offsetExists()' is not overridden, the logic is done correctly; see
also: http://lxr.php.net/xref/PHP_5_6/ext/spl/spl_array.c#617
The behaviour is currently correct inside zend_std_has_dimension(), so
making the fix in SPL alone (#614) would make performance "suffer" less
unless offsetGet() is overridden.
Personally I'm fine either way; like I said, my first patch was only done
in SPL when Etienne suggested the approach in #621.
At least we need to know
the reason why that did not work. So please do not merge this patch
until we know what's going on there.--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
--
Tjerk
Sorry, I hit "Send" too soon.
On Thu, Mar 20, 2014 at 10:36 AM, Tjerk Meesters
tjerk.meesters@gmail.comwrote:
On Thu, Mar 20, 2014 at 9:16 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:
Hi!
Yes, that's probably what it was supposed to do, but as you can see
from bug 66834 that's clearly not the case. I had earlier approached
the problem purely from SPL standpoint (see linked PR) but Etienne
said that it would be a better idea to tackle this issue further up
the chain so to speak :)I think we need to first find out why that code did not work, instead of
ripping out the code that already was supposed to do exactly what this
bug is saying and replacing it with other code.Here's my earlier work on this: https://github.com/php/php-src/pull/614
It currently "doesn't work" because isset()/empty() work like
array_key_exists()on objects that extend ArrayObject, which is in some
cases not the desired behaviour; see also:
http://lxr.php.net/xref/PHP_5_6/ext/spl/spl_array.c#604If 'offsetExists()' is not overridden, the logic is done correctly; see
also: http://lxr.php.net/xref/PHP_5_6/ext/spl/spl_array.c#617
I forgot to mention that the current behaviour is not necessarily correct
if offsetGet() is overridden in a child class; the overridden method may
decide to return null or something empty.
The behaviour is currently correct inside zend_std_has_dimension(), so
making the fix in SPL alone (#614) would make performance "suffer" less
unless offsetGet() is overridden.Personally I'm fine either way; like I said, my first patch was only done
in SPL when Etienne suggested the approach in #621.At least we need to know
the reason why that did not work. So please do not merge this patch
until we know what's going on there.--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227--
Tjerk
--
Tjerk
On Thu, Mar 20, 2014 at 2:16 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:
Hi!
Yes, that's probably what it was supposed to do, but as you can see
from bug 66834 that's clearly not the case. I had earlier approached
the problem purely from SPL standpoint (see linked PR) but Etienne
said that it would be a better idea to tackle this issue further up
the chain so to speak :)I think we need to first find out why that code did not work, instead of
ripping out the code that already was supposed to do exactly what this
bug is saying and replacing it with other code. At least we need to know
the reason why that did not work. So please do not merge this patch
until we know what's going on there.
The main question here is the following:
what is the supposed semantics of has_dimension: is it the semantics of
isset(), or array_key_exists().
The current behavior is:
isset() -> has_dimension(check_exmpty=false) -> offsetExists()
empty() -> !has_dimension(check_exmpty=true) -> !offsetExists() ||
offsetGet()
which one might argue is inconsistent, at least in names. offsetExists
relates to array_key_exists, and as such should IMHO return true for
entries that are defined to NULL.
ArrayObject follows that by (badly) hacking within has_dimension:
$o = new ArrayObject(array('a' => NULL));
var_dump($o->offsetExists('a')); // true
var_dump(isset($o['a'])); // false as long as ArrayObject::offset* are not
overriden
This patch is about decoupling has_dimension from isset()/empty() and
doing, instead:
isset() -> has_dimension() && get_dimension() !== NULL ->
offsetExists() && offsetGet() !== NULL
empty() -> !has_dimension() || !get_dimension() -> !offsetExists()
&& !offsetGet()
which is in my opinion a sensible thing to do.
Now it is clear it is a BC breaking change for people that defined
offsetExists with array_key_exists: isset() which returned true for NULL
will now invoke offsetGet and thus return false, but it is a change towards
more consistency.
If not everybody is confident about this, we will need a RFC.
--
Etienne Kneuss
oops, few typos.
On Thu, Mar 20, 2014 at 2:16 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:
Hi!
Yes, that's probably what it was supposed to do, but as you can see
from bug 66834 that's clearly not the case. I had earlier approached
the problem purely from SPL standpoint (see linked PR) but Etienne
said that it would be a better idea to tackle this issue further up
the chain so to speak :)I think we need to first find out why that code did not work, instead of
ripping out the code that already was supposed to do exactly what this
bug is saying and replacing it with other code. At least we need to know
the reason why that did not work. So please do not merge this patch
until we know what's going on there.The main question here is the following:
what is the supposed semantics of has_dimension: is it the semantics of
isset(), orarray_key_exists().The current behavior is:
isset() -> has_dimension(check_exmpty=false) -> offsetExists()
empty() -> !has_dimension(check_exmpty=true) -> !offsetExists() ||
offsetGet()
empty() -> !has_dimension(check_exmpty=true) -> !offsetExists() ||
!offsetGet()
which one might argue is inconsistent, at least in names. offsetExists
relates to array_key_exists, and as such should IMHO return true for
entries that are defined to NULL.ArrayObject follows that by (badly) hacking within has_dimension:
$o = new ArrayObject(array('a' => NULL));
var_dump($o->offsetExists('a')); // true
var_dump(isset($o['a'])); // false as long as ArrayObject::offset* are not
overridenThis patch is about decoupling has_dimension from isset()/empty() and
doing, instead:isset() -> has_dimension() && get_dimension() !==
NULL->
offsetExists() && offsetGet() !==NULL
empty() -> !has_dimension() || !get_dimension() -> !offsetExists()
&& !offsetGet()
isset() -> has_dimension() && read_dimension() !== NULL ->
offsetExists() && offsetGet() !== NULL
empty() -> !has_dimension() || !read_dimension() -> !offsetExists()
|| !offsetGet()
which is in my opinion a sensible thing to do.
Now it is clear it is a BC breaking change for people that defined
offsetExists with array_key_exists: isset() which returned true forNULL
will now invoke offsetGet and thus return false, but it is a change towards
more consistency.If not everybody is confident about this, we will need a RFC.
--
Etienne Kneuss
--
Etienne Kneuss
http://www.colder.ch
Hi!
The current behavior is:
isset() -> has_dimension(check_exmpty=false) -> offsetExists()
empty() -> !has_dimension(check_exmpty=true) -> !offsetExists()
|| offsetGet()
I think isset part is wrong - it should use offsetGet (probably via
check_empty=true) and then verify that the result is not null.
This patch is about decoupling has_dimension from isset()/empty() and
doing, instead:isset() -> has_dimension() && get_dimension() !==
NULL->
offsetExists() && offsetGet() !==NULL
empty() -> !has_dimension() || !get_dimension() ->
!offsetExists() && !offsetGet()
I'm not sure this is the right way to go. It changes the existing API
without any need - while it can be handled within the existing API as well.
Now it is clear it is a BC breaking change for people that defined
offsetExists with array_key_exists: isset() which returned true forNULL
will now invoke offsetGet and thus return false, but it is a change
towards more consistency.
This is actually OK, isset should return for ArrayObject the same it
would return for array with the same data.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
On Thu, Mar 20, 2014 at 9:05 PM, Stas Malyshev smalyshev@sugarcrm.comwrote:
Hi!
The current behavior is:
isset() -> has_dimension(check_exmpty=false) -> offsetExists()
empty() -> !has_dimension(check_exmpty=true) -> !offsetExists()
|| offsetGet()I think isset part is wrong - it should use offsetGet (probably via
check_empty=true) and then verify that the result is not null.This patch is about decoupling has_dimension from isset()/empty() and
doing, instead:isset() -> has_dimension() && get_dimension() !==
NULL->
offsetExists() && offsetGet() !==NULL
empty() -> !has_dimension() || !get_dimension() ->
!offsetExists() && !offsetGet()I'm not sure this is the right way to go. It changes the existing API
without any need - while it can be handled within the existing API as well.
I agree that the API change is not in fact needed. We could limit ourselves
to changing the behavior of the default has_dimension handler. It is also
probably faster to do everything in has_dimension.
I still think the "new version" is clearer, but it might not be worth the
hassle of forcing this API change.
The important part is that isset() triggers read_dimension and eventually
offsetGet() for isset(), and we seem to agree on that.
Now it is clear it is a BC breaking change for people that defined
offsetExists with array_key_exists: isset() which returned true forNULL
will now invoke offsetGet and thus return false, but it is a change
towards more consistency.This is actually OK, isset should return for ArrayObject the same it
would return for array with the same data.--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
--
Etienne Kneuss
http://www.colder.ch
Hi!
The important part is that isset() triggers read_dimension and
eventually offsetGet() for isset(), and we seem to agree on that.
Yes, agreed. Unfortunately, I'll be busy till the next week so couldn't
propose a fix, but if nobody beats me to it I'll look at it next week.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi Stas,
On Fri, Mar 21, 2014 at 6:02 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:
Hi!
The important part is that isset() triggers read_dimension and
eventually offsetGet() for isset(), and we seem to agree on that.Yes, agreed. Unfortunately, I'll be busy till the next week so couldn't
propose a fix, but if nobody beats me to it I'll look at it next week.
It seems that PR 614 covers exactly that; it uses the check_empty flag
within spl_array_has_dimension_ex() to make sure offsetGet() is called when
needed.
https://github.com/php/php-src/pull/614
Was there something I missed there? I'm happy to work on this :)
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
--
Tjerk
Hi!
It seems that PR 614 covers exactly that; it uses the check_empty flag
within spl_array_has_dimension_ex() to make sure offsetGet() is called
when needed.
I understand that the problem may be that check_empty is not sent on
isset? Didn't verify that though.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi Stas,
On Fri, Mar 21, 2014 at 9:11 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:
Hi!
It seems that PR 614 covers exactly that; it uses the check_empty flag
within spl_array_has_dimension_ex() to make sure offsetGet() is called
when needed.I understand that the problem may be that check_empty is not sent on
isset? Didn't verify that though.
It does, as can be seen here:
http://lxr.php.net/xref/PHP_5_6/Zend/zend_vm_def.h#4737
The check_empty argument value is ternary, meaning it can be 0 (isset), 1
(empty) or 2 (key exists).
Having looked it over I realized that my older patch had some issues; I've
addressed those and reopened #614.
I've closed #621.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
--
Tjerk
Hi Stas,
On Fri, Mar 21, 2014 at 10:48 AM, Tjerk Meesters
tjerk.meesters@gmail.comwrote:
Hi Stas,
On Fri, Mar 21, 2014 at 9:11 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:
Hi!
It seems that PR 614 covers exactly that; it uses the check_empty flag
within spl_array_has_dimension_ex() to make sure offsetGet() is called
when needed.I understand that the problem may be that check_empty is not sent on
isset? Didn't verify that though.It does, as can be seen here:
http://lxr.php.net/xref/PHP_5_6/Zend/zend_vm_def.h#4737
The check_empty argument value is ternary, meaning it can be 0 (isset), 1
(empty) or 2 (key exists).Having looked it over I realized that my older patch had some issues; I've
addressed those and reopened #614.
Do you have any issues with the code as it stands now?
https://github.com/php/php-src/pull/614
It's currently targeted at 5.4; if there are any objections to that I'll
move the changes to 5.6 instead.
I've closed #621.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227--
Tjerk
--
Tjerk
It's currently targeted at 5.4; if there are any objections to that I'll
move the changes to 5.6 instead.
I object. I really like the changes but don't think they should go into
those releases. While this is a bug fix I imagine there really are people
out there using this behavior; better to put it into PHP 5.6.
I only did a really quick glance, though; maybe someone else can comment on
it and we can reach an agreement.
Hi!
Do you have any issues with the code as it stands now?
Something I don't understand in this patch: so if we don't have "has"
override, we'd get value == NULL and then we go and check internal
hashtable. Shouldn't we first check the get override and ask it with
BP_VAR_IS, and only if get override is not there use direct hash access?
E.g. imagine this:
class SecretArrayObject extends ArrayObject
{
public function offsetGet($offset) {
var_dump('Called: '.METHOD);
return parent::offsetGet(str_rot13($offset));
}
public function offsetSet($offset) {
var_dump('Called: '.METHOD);
return parent::offsetSet(str_rot13($offset));
}
}
I think on this example your code may fail to go to correct items when
doing isset/isempty, since if I ask for 'qux' it would actually check
the value I've put there for 'dhk'.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
On Tue, Mar 25, 2014 at 11:34 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:
Hi!
Do you have any issues with the code as it stands now?
Something I don't understand in this patch: so if we don't have "has"
override, we'd get value ==NULLand then we go and check internal
hashtable. Shouldn't we first check the get override and ask it with
BP_VAR_IS, and only if get override is not there use direct hash access?
It's my understanding that you should (have to) only call "get" if you know
that the offset exists.
E.g. imagine this:
class SecretArrayObject extends ArrayObject
{
public function offsetGet($offset) {
var_dump('Called: '.METHOD);
return parent::offsetGet(str_rot13($offset));
}
public function offsetSet($offset) {
var_dump('Called: '.METHOD);
return parent::offsetSet(str_rot13($offset));
}
}I think on this example your code may fail to go to correct items when
doing isset/isempty, since if I ask for 'qux' it would actually check
the value I've put there for 'dhk'.
This particular class should override offsetExists($offset) as well, i.e.:
public function offsetExists($offset)
{
return parent::offsetExists(str_rot13($offset));
}
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
--
Tjerk
Hi!
It's my understanding that you should (have to) only call "get" if you
know that the offset exists.
Yes, you are right. I'd still add some example that changes the keys to
the tests to ensure all cases are covered properly. But looks like it
works OK in all cases I could think of.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi,
On Tue, Mar 25, 2014 at 11:54 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:
Hi!
It's my understanding that you should (have to) only call "get" if you
know that the offset exists.Yes, you are right. I'd still add some example that changes the keys to
the tests to ensure all cases are covered properly. But looks like it
works OK in all cases I could think of.
I've updated the test case with such an example.
PHP-5.6 and master have been updated.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
--
Tjerk
In a similar vein have we checked offsetSet? It seems offsetSet is not
called with +=; I am not sure if this is intended; see
https://bugs.php.net/bug.php?id=66641
On Tue, Mar 25, 2014 at 4:38 AM, Tjerk Meesters tjerk.meesters@gmail.comwrote:
Hi,
On Tue, Mar 25, 2014 at 11:54 AM, Stas Malyshev <smalyshev@sugarcrm.com
wrote:
Hi!
It's my understanding that you should (have to) only call "get" if you
know that the offset exists.Yes, you are right. I'd still add some example that changes the keys to
the tests to ensure all cases are covered properly. But looks like it
works OK in all cases I could think of.I've updated the test case with such an example.
PHP-5.6 and master have been updated.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227--
Tjerk
Hi Levi,
On Tue, Mar 25, 2014 at 9:51 PM, Levi Morrison morrison.levi@gmail.comwrote:
In a similar vein have we checked
offsetSet? It seemsoffsetSetis not
called with+=; I am not sure if this is intended; see
https://bugs.php.net/bug.php?id=66641
This is due to how binary assignment is applied to properties vs arrays:
http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_vm_execute.h#31979
The workaround here is to disable the get_property_ptr_ptr inside
spl_array.c, like this:
https://gist.github.com/datibbaw/703597322ad411287461
Not sure if everyone will agree to that change, but it's one way to fix the
bug other than changing the API itself :)
On Tue, Mar 25, 2014 at 4:38 AM, Tjerk Meesters tjerk.meesters@gmail.comwrote:
Hi,
On Tue, Mar 25, 2014 at 11:54 AM, Stas Malyshev <smalyshev@sugarcrm.com
wrote:
Hi!
It's my understanding that you should (have to) only call "get" if you
know that the offset exists.Yes, you are right. I'd still add some example that changes the keys to
the tests to ensure all cases are covered properly. But looks like it
works OK in all cases I could think of.I've updated the test case with such an example.
PHP-5.6 and master have been updated.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227--
Tjerk
--
Tjerk
On Wed, Mar 26, 2014 at 12:14 PM, Tjerk Meesters
tjerk.meesters@gmail.comwrote:
Hi Levi,
On Tue, Mar 25, 2014 at 9:51 PM, Levi Morrison morrison.levi@gmail.comwrote:
In a similar vein have we checked
offsetSet? It seemsoffsetSetis
not called with+=; I am not sure if this is intended; see
https://bugs.php.net/bug.php?id=66641This is due to how binary assignment is applied to properties vs arrays:
http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_vm_execute.h#31979
The workaround here is to disable the
get_property_ptr_ptrinside
spl_array.c, like this:
The test suite proved me wrong about this, as bug65328.phpt fails and
causes some memory leaks when applied; some tweaks may be required or an
altogether different approach :)
Not sure if everyone will agree to that change, but it's one way to fix
the bug other than changing the API itself :)On Tue, Mar 25, 2014 at 4:38 AM, Tjerk Meesters <tjerk.meesters@gmail.com
wrote:
Hi,
On Tue, Mar 25, 2014 at 11:54 AM, Stas Malyshev <smalyshev@sugarcrm.com
wrote:
Hi!
It's my understanding that you should (have to) only call "get" if
you
know that the offset exists.Yes, you are right. I'd still add some example that changes the keys to
the tests to ensure all cases are covered properly. But looks like it
works OK in all cases I could think of.I've updated the test case with such an example.
PHP-5.6 and master have been updated.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227--
Tjerk
--
Tjerk
--
Tjerk