Fixed bug #61527 (Recursive/ArrayIterator gives misleading notice when array empty or moved to the end) #38

Closed
wants to merge 1 commit into
from

Projects

None yet

7 participants

@reeze
Contributor
reeze commented Apr 5, 2012

Hi,

when calling
- ArrayIterator->next(),
- ArrayIterator->key(),
- ArrayIterator->current(),
- RecurseArrayIterator->getChildren()
- RecurseArrayIterator->hasChildren()
it will raise misleading(array was modified outside) message. when array was empty or moved to then end of array.

This pull request is trying to fix it.

Thanks.

@nikic nikic commented on an outdated diff Apr 5, 2012
ext/spl/spl_array.c
@@ -58,6 +58,29 @@
#define SPL_ARRAY_INT_MASK 0xFFFF0000
#define SPL_ARRAY_CLONE_MASK 0x0300FFFF
+#define SPL_ARRAY_CHECK_POS_EMPTY_OR_END_REACHED(object, ht, err_msg) do { \
+ if (!aht) { \
@nikic
nikic Apr 5, 2012 Contributor

This probably should be ht, as that's what you're passing into the macro.

@nikic nikic and 2 others commented on an outdated diff Apr 5, 2012
ext/spl/spl_array.c
+#define SPL_ARRAY_CHECK_POS_EMPTY_OR_END_REACHED(object, ht, err_msg) do { \
+ if (!aht) { \
+ php_error_docref(NULL TSRMLS_CC, E_NOTICE, "Array was modified outside object and is no longer an array"); \
+ return; \
+ } \
+ if (((object)->ar_flags & SPL_ARRAY_IS_REF)) { \
+ if ((object)->pos == NULL) { \
+ if ((object->pos_h) == 0 && (ht)->pInternalPointer == NULL) { \
+ php_error_docref(NULL TSRMLS_CC, E_NOTICE, "Array was empty " err_msg); \
+ } else { \
+ php_error_docref(NULL TSRMLS_CC, E_NOTICE, "Array was already reached the end " err_msg " and it will be rewinded to the beginning"); \
+ spl_array_rewind((object) TSRMLS_CC); \
+ } \
+ return; \
+ } else { \
+ if (spl_hash_verify_pos_ex(intern, aht TSRMLS_CC) == FAILURE) { \
@nikic
nikic Apr 5, 2012 Contributor

intern and aht should be object and ht.

@reeze
reeze Apr 5, 2012 Contributor

Thanks for point out. it works just because of macro expand. will be update soon. thanks :)

@cataphract
cataphract Apr 6, 2012 Contributor

A static function returning SUCCESS/FAILURE would be more appropriate. You can always mark it inline.

@reeze
reeze Apr 6, 2012 Contributor

thanks. I will update it soon.:)

@reeze
reeze Apr 6, 2012 Contributor

@cataphract updated and rebased the commit. thanks

@nikic nikic and 1 other commented on an outdated diff Apr 5, 2012
ext/spl/spl_array.c
@@ -58,6 +58,29 @@
#define SPL_ARRAY_INT_MASK 0xFFFF0000
#define SPL_ARRAY_CLONE_MASK 0x0300FFFF
+#define SPL_ARRAY_CHECK_POS_EMPTY_OR_END_REACHED(object, ht, err_msg) do { \
+ if (!(ht)) { \
+ php_error_docref(NULL TSRMLS_CC, E_NOTICE, "Array was modified outside object and is no longer an array"); \
+ return; \
+ } \
+ if (((object)->ar_flags & SPL_ARRAY_IS_REF)) { \
+ if ((object)->pos == NULL) { \
+ if (((object)->pos_h) == 0 && (ht)->pInternalPointer == NULL) { \
+ php_error_docref(NULL TSRMLS_CC, E_NOTICE, "Array was empty " err_msg); \
+ } else { \
+ php_error_docref(NULL TSRMLS_CC, E_NOTICE, "Array was already reached the end " err_msg " and it will be rewinded to the beginning"); \
+ spl_array_rewind((object) TSRMLS_CC); \
@nikic
nikic Apr 5, 2012 Contributor

I'm not sure I understand this. Why should the array be rewinded when one reaches the end?

@reeze
reeze Apr 5, 2012 Contributor

I dont like it too. The original current() and etc behave so so I re implement it . for now it looks like an unwanted side effect since we know it is an empty array or ended array.

发自我的 iPhone

在 2012-4-6,2:19,nikicreply@reply.github.com 写道:

@@ -58,6 +58,29 @@
#define SPL_ARRAY_INT_MASK 0xFFFF0000
#define SPL_ARRAY_CLONE_MASK 0x0300FFFF

+#define SPL_ARRAY_CHECK_POS_EMPTY_OR_END_REACHED(object, ht, err_msg) do { \

  • if (!(ht)) { \
  •    php_error_docref(NULL TSRMLS_CC, E_NOTICE, "Array was modified outside object and is no longer an array"); \
    
  •    return; \
    
  • } \
  • if (((object)->ar_flags & SPL_ARRAY_IS_REF)) { \
  •    if ((object)->pos == NULL) { \
    
  •        if (((object)->pos_h) == 0 && (ht)->pInternalPointer == NULL) { \
    
  •            php_error_docref(NULL TSRMLS_CC, E_NOTICE, "Array was empty " err_msg);    \
    
  •        } else { \
    
  •            php_error_docref(NULL TSRMLS_CC, E_NOTICE, "Array was already reached the end " err_msg " and it will be rewinded to the beginning"); \
    
  •            spl_array_rewind((object) TSRMLS_CC); \
    

I'm not sure I understand this. Why should the array be rewinded when one reaches the end?


Reply to this email directly or view it on GitHub:
https://github.com/php/php-src/pull/38/files#r651769

@reeze
Contributor
reeze commented Apr 20, 2012

update: error messsage updated & if else branch cleanup

@reeze
Contributor
reeze commented May 2, 2012

Thanks for reply, I will updated it soon;)

@dsp
Member
dsp commented May 2, 2012

btw are you sure the bug id is correct? The bug you refer to is closed and has little to do with your change.

@smalyshev
Contributor

Which bug you are trying to fix? Bug #61347 is "inconsistent isset behavior of Arrayobject" and already fixed.

@smalyshev
Contributor

There should be no messages when array is iterated to an end - it is perfectly normal thing to do. Empty array is a perfectly normal thing too, there should be no messages on that either. Also, I don't see why array should be rewinded. If it's at the end, it should just stay there.

@reeze
Contributor
reeze commented May 2, 2012

sorry ,my fault it's Bug#61527

@dsp
Member
dsp commented May 2, 2012

I agree with @smalyshev here. It's perfectly valid to iterate over an empty array. As #61527 points out, there shouldn't be any Notice at all in those cases.

@reeze
Contributor
reeze commented May 2, 2012

em, it makes sense, next() didn't notice too.

so no notice at all in this case, am I right?

@reeze
Contributor
reeze commented May 2, 2012

but the method related to current entry still need notice if array was empty or reached the end.
so only next() shouldn't complain. I will fix it soon.

thanks

@reeze
Contributor
reeze commented May 2, 2012

I have updated the patch:

  • No notice anymore when $iter->next() and update the notice message.

@smalyshev I've already rebased the pull request, that has been updated. the previous code show up in discussion.
please refer to the diff.

"There should be no messages when array is iterated to an end - it is perfectly normal thing to do. Empty array is a perfectly normal thing too, there should be no messages on that either. Also, I don't see why array should be rewinded. If it's at the end, it should just stay there."

Thanks.

@smalyshev
Contributor

current() should not give any notices either. It should just return false. That notice does not contribute anything.

@laruence
Member
laruence commented May 3, 2012

actually, I think only notice for :"modified outside" is enough. no need yelling for "reached the end"

@reeze
Contributor
reeze commented May 3, 2012

Hi,
I've updated the patch. no extra notice on next(), current() or key(), only notice when array was really modified.
and I refactor some code to reduce duplication. This bug also affect RecuseArrayIterator, I fixed it both of them.

Thanks.

@php-pulls

Comment on behalf of stas at php.net:

merged

@php-pulls php-pulls closed this Jul 16, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment