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

Fix #80332: Completely broken array access functionality with DOMNamedNodeMap #11468

Closed
wants to merge 2 commits into from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Jun 17, 2023

The problem is the usage of zval_get_long(). In particular, if the
string is non-numeric the result of zval_get_long() will be 0 without
giving an error or warning. This is misleading for users: users get the
impression that they can use strings to access the map because it
coincidentally works for the first item (which is at index 0). Of
course, this fails with any other index which causes confusion and bugs.

This patch adds proper support for using string offsets while accessing
the map. It does so by detecting if it's a non-numeric string, and then
using the getNamedItem() method instead of item(). I had to split up the
array access implementation code for DOMNodeList and DOMNamedNodeMap
first to be able to do this.

…dNodeMap

The problem is the usage of zval_get_long(). In particular, if the
string is non-numeric the result of zval_get_long() will be 0 without
giving an error or warning. This is misleading for users: users get the
impression that they can use strings to access the map because it
coincidentally works for the first item (which is at index 0). Of
course, this fails with any other index which causes confusion and bugs.

This patch adds proper support for using string offsets while accessing
the map. It does so by detecting if it's a non-numeric string, and then
using the getNamedItem() method instead of item(). I had to split up the
array access implementation code for DOMNodeList and DOMNamedNodeMap
first to be able to do this.
@nielsdos nielsdos force-pushed the fix-broken-access-in-named-map branch from a553b05 to c45d29e Compare June 17, 2023 19:08
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

LGTM, although seems this would be a useful case for pushing #10175 again to be added as an engine API.

@nielsdos
Copy link
Member Author

LGTM, although seems this would be a useful case for pushing #10175 again to be added as an engine API.

Thanks. And yes, this would be very nice to have! If that would get merged I'll refactor this code to make use of that.

@nielsdos nielsdos closed this in 9f7d888 Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants