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

Iterate node lists with foreach #90

Merged
merged 4 commits into from
Oct 11, 2024
Merged

Iterate node lists with foreach #90

merged 4 commits into from
Oct 11, 2024

Conversation

jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Mar 16, 2024

DOMNodeList implements Traversable.

There are some for loops left but we cannot simply replace those: PHP follows the DOM specification, which requires that NodeList objects in the DOM are live. As a result, any operation that removes a node list member node from its parent (such as removeChild, replaceChild or appendChild) will cause the next node in the iterator to be skipped.

We could work around that by converting those node lists to static arrays using iterator_to_array but not sure if it is worth it.

Also add few cleanups I noticed.

@jtojnar jtojnar changed the title Use foreach instead of for loop where possible Iterate node lists with foreach Mar 16, 2024
@jtojnar jtojnar force-pushed the foreaches branch 3 times, most recently from 37e795b to 9173e41 Compare March 17, 2024 02:18
Copy link
Owner

@j0k3r j0k3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still found it more readable to read foreach than for loop.
So it's ok for me

src/Readability.php Show resolved Hide resolved
@jtojnar
Copy link
Contributor Author

jtojnar commented Mar 18, 2024

I have rebased this, deciding to skip the reverse for loops for now – the naïve port to iterator_to_array does not really work (see #91, possibly due to nested nodes being matched by xpath) . Maybe it would work with array_reverse but I will need too give the invariants deeper thought.

`DOMNode::$childNodes` always contained `DOMNodeList`.
This simplifies the code a bit and will make it slightly easier in case we decide to switch to `foreach` iteration.
This was forgotten in b580cf2.
`DOMNodeList` implements `Traversable`.

There are some `for` loops left but we cannot simply replace those:
PHP follows the DOM specification, which requires that `NodeList`
objects in the DOM are live. As a result, any operation that removes
a node list member node from its parent (such as `removeChild`,
`replaceChild` or `appendChild`) will cause the next node
in the iterator to be skipped.

We could work around that by converting those node lists to static arrays
using `iterator_to_array` but not sure if it is worth it.
@j0k3r j0k3r merged commit f825dcf into j0k3r:master Oct 11, 2024
10 checks passed
@jtojnar jtojnar deleted the foreaches branch October 11, 2024 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants