Skip to content

Switch DOMNodeList::item() and DOMNamedNodeMap::item() to fast ZPP #11361

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 4, 2023
Merged

Switch DOMNodeList::item() and DOMNamedNodeMap::item() to fast ZPP #11361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions ext/dom/namednodemap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,9 @@ PHP_METHOD(DOMNamedNodeMap, item)
int count;

id = ZEND_THIS;
if (zend_parse_parameters(ZEND_NUM_ARGS(), "l", &index) == FAILURE) {
RETURN_THROWS();
}
ZEND_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_LONG(index)
ZEND_PARSE_PARAMETERS_END();
if (index < 0 || ZEND_LONG_INT_OVFL(index)) {
zend_argument_value_error(1, "must be between 0 and %d", INT_MAX);
RETURN_THROWS();
Expand Down
6 changes: 3 additions & 3 deletions ext/dom/nodelist.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,9 @@ PHP_METHOD(DOMNodeList, item)
int count = 0;

id = ZEND_THIS;
if (zend_parse_parameters(ZEND_NUM_ARGS(), "l", &index) == FAILURE) {
RETURN_THROWS();
}
ZEND_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_LONG(index)
ZEND_PARSE_PARAMETERS_END();
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated, is it me or does this just do nothing when passing a negative index? :| Does the spec allow for asking a negative index or should this be changed to emit a warning?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I also noticed this, and I'm afraid we can't change the behaviour without making a BC break :(
Spec link for item: https://www.w3.org/TR/2003/WD-DOM-Level-3-Core-20030226/DOM3-Core.html#core-ID-349467F9
The problem is that spec defines the index as an unsigned type...
If index is greater than or equal to the number of nodes in this map, this returns null.
and further down: The node at the indexth position in the map, or null if that is not a valid index.
So it sounds to me it shouldn't throw, but rather should return NULL.

Copy link
Member

Choose a reason for hiding this comment

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

Right, normally what we do is that we used to emit an E_WARNING for negative indices, which got changed to ValueErrors in PHP 8.0.

I think changing this to emit a warning for negative indices is at least a good start to indicate there is some mistake. But that's another PR to be done, (and probably check that we are handling indices consistently too...).

Copy link
Member Author

Choose a reason for hiding this comment

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

OTOH, people coming from only knowing the DOM spec will be surprised that this throws. Spec doesn't define exceptions for this.
I tried with javascript, try executing this: document.getElementsByTagName('p').item(-1) it just returns NULL without an exception.

Copy link
Member

Choose a reason for hiding this comment

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

Huh, welp IDK then. But in the other method we do check for the offset so :|


if (index >= 0) {
intern = Z_DOMOBJ_P(id);
Expand Down