-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add tidyNode::getNextSibling() and tidyNode::getPreviousSibling() #15047
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
Conversation
newobj->ptdoc->ref_count++; | ||
tidy_add_node_default_properties(newobj); | ||
} else { | ||
ZVAL_NULL(return_value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just lost here ... not necessary anymore right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You never need to set the return value to IS_NULL because the VM & JIT do it prior to the call.
I removed it to reduce the number of lines that are very very similar.
FWIW I sent an email to internals to ask if anyone objects: https://externals.io/message/124530 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to merge the first commit.
Don't forget to add an UPGRADING note, but I think this is a good addition will also reply on the ML.
These get the next and previous sibling nodes, respectively. We can already kind of do this by using the $child array, but that's inconvenient when actually walking the tree by only using node instances. Since the class is final, there is no BC break here.
This looks like a useful addition to the extension to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I merged this as no one complained and reaction was positive. Thanks for checking! |
First commit refactors the code a bit to make implementation easier.
Second commit actually implements it:
These get the next and previous sibling nodes, respectively.
We can already kind of do this by using the $child array, but that's
inconvenient when actually walking the tree by only using node
instances. Since the class is final, there is no BC break here.