Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removal during NodeList iteration breaks loop #13863

Closed
nielsdos opened this issue Apr 1, 2024 · 2 comments
Closed

Removal during NodeList iteration breaks loop #13863

nielsdos opened this issue Apr 1, 2024 · 2 comments

Comments

@nielsdos
Copy link
Member

nielsdos commented Apr 1, 2024

Description

The following code:

<?php

$doc = new DOMDocument();
$doc->preserveWhiteSpace = false;
$doc->loadXML(
    <<<EOXML
    <x>
        <item1 />
        <item2 />
        <item3 />
        <item4 />
        <item5 />
    </x>
EOXML
);

foreach ($doc->documentElement->childNodes as $i => $node) {
    echo $node->localName . PHP_EOL;
    if ($i === 2) {
        $node->remove();
    }
}

https://3v4l.org/TJLvF

Resulted in this output:

item1
item2
item3

But I expected this output instead:

item1
item2
item3
item5

Source: nielsdos#93 (comment)
Credits to @veewee. Similar issue exists for attributes.

PHP Version

Since always

Operating System

No response

@damianwadley
Copy link
Member

damianwadley commented Apr 1, 2024

This behavior is correct: childNodes is a "live" NodeList, and changing the children is reflected immediately in the list. Which also means the current i now points to what would have been the next child, thus when the loop advances it skips an item.

Javascript implements it that way. Does PHP really want to be different about it?
https://jsfiddle.net/54hoLzqv/
(keep in mind that it's the console output which shows items 1,2,3, while the document had 3 removed and so ends up with items 1,2,4)

Same for attributes, which is also a live NodeList.

Obligatory "this behavior has existed for a really long time so is there a good enough reason to change it now?"

@nielsdos
Copy link
Member Author

nielsdos commented Apr 2, 2024

The example is badly chosen, I've corrected the example. Then you can see that the iteration stops despite the list being needing to be live.

Obligatory "this behavior has existed for a really long time so is there a good enough reason to change it now?"

Obligatory "we kind of worked around that issue": https://wiki.php.net/rfc/opt_in_dom_spec_compliance

nielsdos added a commit to nielsdos/php-src that referenced this issue Apr 10, 2024
The list is live, so upon cache invalidation we should rewalk the tree
to sync the index again with the node list. We keep the legacy behaviour
for the old DOM classes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants