# Convert SplFixedArray to Aggregate?

- Thread: https://externals.io/message/110731
- Original archive: https://news-web.php.net/php.internals/110731
- Started by Alex on 2020-06-26T06:44:19+00:00
- Messages: 5
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · RFCs at https://externals.io/rfc.md · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #110731 — Alex — 2020-06-26T06:44:19+00:00

URL: https://externals.io/message/110731#110731

Dear PHP Internals,

I would like to propose a backwards-incompatible change to
SplFixedArray which fixes the strange and almost certainly unintended
behavior reported in Bug 79404
(https://bugs.php.net/bug.php?id=79404).

In short: Because SplFixedArray is an Iterator, and stores its own
iteration state, it cannot be used in nested foreach loops. If you
try, the inner loop will overwrite the iteration position of the outer
loop, so that the outer loop 'thinks' it is finished after the inner
loop finishes.

To illustrate:

$spl = SplFixedArray::fromArray([0, 1]);
foreach ($spl as $a) {
  foreach ($spl as $b) {
    echo "$a $b";
  }
}

Only prints two lines:

0 0
0 1

The fix is to convert SplFixedArray to an IteratorAggregate, so each
nested foreach loop operates on a different iterator object and thus
nested loops don't interfere with each other.

Now, it would be possible to do this while still defining `key()`,
 `current()`, `next()`, etc. methods on SplFixedArray. However, that would
cause an even more insidious BC break, since these methods would no
longer work as formerly when called in the body of a foreach loop.
Therefore, I think it better to remove them. This may break a few
users' code, but in a way which will be easier for them to debug than
if the old methods were kept but subtly changed in behavior.

Code to implement this change is here:
https://github.com/php/php-src/pull/5384/files

Your comments will be appreciated,
Alex Dowad

---

## #110734 — Levi Morrison via internals — 2020-06-26T13:56:55+00:00

URL: https://externals.io/message/110731#110734 · In reply to: #110731

>
> Dear PHP Internals,
>
> I would like to propose a backwards-incompatible change to
> SplFixedArray which fixes the strange and almost certainly unintended
> behavior reported in Bug 79404
> (https://bugs.php.net/bug.php?id=79404).
>
> In short: Because SplFixedArray is an Iterator, and stores its own
> iteration state, it cannot be used in nested foreach loops. If you
> try, the inner loop will overwrite the iteration position of the outer
> loop, so that the outer loop 'thinks' it is finished after the inner
> loop finishes.
>
> To illustrate:
>
> $spl = SplFixedArray::fromArray([0, 1]);
> foreach ($spl as $a) {
>   foreach ($spl as $b) {
>     echo "$a $b";
>   }
> }
>
> Only prints two lines:
>
> 0 0
> 0 1
>
> The fix is to convert SplFixedArray to an IteratorAggregate, so each
> nested foreach loop operates on a different iterator object and thus
> nested loops don't interfere with each other.
>
> Now, it would be possible to do this while still defining `key()`,
> `current()`, `next()`, etc. methods on SplFixedArray. However, that would
> cause an even more insidious BC break, since these methods would no
> longer work as formerly when called in the body of a foreach loop.
> Therefore, I think it better to remove them. This may break a few
> users' code, but in a way which will be easier for them to debug than
> if the old methods were kept but subtly changed in behavior.
>
> Code to implement this change is here:
> https://github.com/php/php-src/pull/5384/files
>
> Your comments will be appreciated,
> Alex Dowad
>
> --
> 
> To unsubscribe, visit: https://www.php.net/unsub.php
>

As someone who has patches in the SPL, I support the idea of this
change. One gotcha of aggregates is iterator invalidation; are you
sure the patch takes care of it? If you have an array of size 3, get
an iterator, iterate to offset 2, shrink the size with `setSize`, and
use the iterator at offset 2, will everything be okay?

---

## #110735 — Alex — 2020-06-26T14:04:30+00:00

URL: https://externals.io/message/110731#110735 · In reply to: #110734

Dear Levi, I will add a test case to the PR to ensure that things work
properly in the described situation.

On Fri, Jun 26, 2020 at 3:57 PM Levi Morrison
<levi.morrison@datadoghq.com> wrote:
>
> 
> > […]
>
> As someone who has patches in the SPL, I support the idea of this
> change. One gotcha of aggregates is iterator invalidation; are you
> sure the patch takes care of it? If you have an array of size 3, get
> an iterator, iterate to offset 2, shrink the size with `setSize`, and
> use the iterator at offset 2, will everything be okay?

---

## #110737 — Alex — 2020-06-26T18:09:45+00:00

URL: https://externals.io/message/110731#110737 · In reply to: #110735

Dear Levi Morrison, please see this added test case:
https://github.com/php/php-src/pull/5384/files#diff-dc4d304baf106b4a30432f80dae1a538

The iteration behavior of the modified SplFixedArray is quite humane
even in the face of changing array size.

---

## #110758 — Nikita Popov — 2020-06-28T12:41:02+00:00

URL: https://externals.io/message/110731#110758 · In reply to: #110731

> Dear PHP Internals,
>
> I would like to propose a backwards-incompatible change to
> SplFixedArray which fixes the strange and almost certainly unintended
> behavior reported in Bug 79404
> (https://bugs.php.net/bug.php?id=79404).
>
> In short: Because SplFixedArray is an Iterator, and stores its own
> iteration state, it cannot be used in nested foreach loops. If you
> try, the inner loop will overwrite the iteration position of the outer
> loop, so that the outer loop 'thinks' it is finished after the inner
> loop finishes.
>
> To illustrate:
>
> $spl = SplFixedArray::fromArray([0, 1]);
> foreach ($spl as $a) {
>   foreach ($spl as $b) {
>     echo "$a $b";
>   }
> }
>
> Only prints two lines:
>
> 0 0
> 0 1
>
> The fix is to convert SplFixedArray to an IteratorAggregate, so each
> nested foreach loop operates on a different iterator object and thus
> nested loops don't interfere with each other.
>
> Now, it would be possible to do this while still defining `key()`,
> `current()`, `next()`, etc. methods on SplFixedArray. However, that would
> cause an even more insidious BC break, since these methods would no
> longer work as formerly when called in the body of a foreach loop.
> Therefore, I think it better to remove them. This may break a few
> users' code, but in a way which will be easier for them to debug than
> if the old methods were kept but subtly changed in behavior.
>
> Code to implement this change is here:
> https://github.com/php/php-src/pull/5384/files
>
> Your comments will be appreciated,
> Alex Dowad
>

I agree that we should make this change for PHP 8. The current behavior is
pretty clearly a bug, and the practical backwards compatibility impact
should be rather limited (and only annoying to work around if you extend
SplFixedArray and override Iterator methods).

Nikita