Skip to content

Fix empty argument cases for DOMParentNode methods #11768

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

Closed
wants to merge 1 commit into from

Conversation

nielsdos
Copy link
Member

They should be callable with empty arguments. It might seem useless but this can happen with empty fragments (as seen in the attached testcase).
This aligns the behaviour to spec.

@petk
Copy link
Member

petk commented Jul 22, 2023

@nielsdos just one unrelated question regarding dom. Should the DOM_API_VERSION be also bumped in ext/dom/php_dom.h with the recent changes in the dom extension?

Maybe it could be fixed to PHP_VERSION so it's more clear.

@nielsdos
Copy link
Member Author

@petk There's an open issue for that: #8038. DOM_API_VERSION reflects the implemented standard version. So it shouldn't be equal to PHP_VERSION. Also, since we don't fully implement the living standard yet, bumping it to the last's revision date is too optimistic. I'm not sure what to do with that and I don't know if people depend on its value...

@petk
Copy link
Member

petk commented Jul 22, 2023

Ok, then let's bump it in PHP 9, I guess. :) Nobody relies on the DOM version anyway because it is constant for many PHP versions.

@nielsdos
Copy link
Member Author

Searching on GH yields indeed no results: https://github.com/search?q=DOM_API_VERSION+language%3APHP+&type=code
I'd like to just get rid of it in the future as it provides more confusion than value, and from a quick look nobody seems to use it. That's future plans though like you say.

@petk
Copy link
Member

petk commented Jul 22, 2023

Composer can use it via composer.json and ext-dom set in it:

image

However, in the code itself checking the version for the DOM extension specifically (phpversion('dom')) is better done by checking the PHP version itself (phpversion()). Yes, this is off-topic of course. Sorry for polluting the PR.

@nielsdos
Copy link
Member Author

Right, having the DOM_API_VERSION and extension version tied together is unfortunate... :/

Yes, this is off-topic of course. Sorry for polluting the PR.

No worries. In the future let's post our findings in that issue I linked above :)

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

@@ -364,12 +364,12 @@ PHP_METHOD(DOMCharacterData, remove)

PHP_METHOD(DOMCharacterData, after)
{
int argc;
int argc = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Those all should be uint32_t in theory, but that's more a master branch thing.

Suggested change
int argc = 0;
uint32_t argc = 0;

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually that's already done in master :D

@nielsdos nielsdos closed this in abb1d2e Jul 24, 2023
jorgsowa pushed a commit to jorgsowa/php-src that referenced this pull request Jul 26, 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.

3 participants