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

Prevent DocumentFragment from causing internal PHP error by using append() #454

Merged
merged 5 commits into from
Apr 25, 2024

Conversation

g105b
Copy link
Member

@g105b g105b commented Apr 25, 2024

This is some weird libxml behaviour. I'm going to put in an upstream bug fix at some point, but one of the purposes of this library is to patch this kind of weird behaviour. All tests passing now.

Closes #453

@g105b
Copy link
Member Author

g105b commented Apr 25, 2024

PHPUnit is failing because it's trying to run PHPUnit 11 which requires PHP >= 8.2, but that's not what it says in the composer.lock (or even componser.json).

I will need to upgrade the actions runner as described here: php-actions/phpunit#54

@g105b g105b merged commit e2460c3 into master Apr 25, 2024
18 checks passed
@g105b g105b deleted the 453-append-child branch April 25, 2024 16:31
nielsdos added a commit to nielsdos/php-src that referenced this pull request May 11, 2024
@nielsdos
Copy link

nielsdos commented May 11, 2024

@g105b PHP core maintainer here. I fixed this here: php/php-src#14206
It was already fixed on the PHP 8.4 development branch because I rewrote the entire ParentNode implementation when implementing opt-in DOM spec-compliance. So I just backported that fix.
Please just submit or ping us when you encounter something like this, otherwise stuff like that never gets fixed or fixed only after a long time. To give an idea: patching this only took 15 minutes from reading this PR until submitting my PR, so even if you just have code it's often easy to fix these issues. It's just that we have to be aware of them in order to be able to fix them.

@g105b
Copy link
Member Author

g105b commented May 11, 2024

@nielsdos hey, thank you for your message. I will submit an upstream issue whenever something like this comes up, but I've never felt confident to do it. I see now that PHP has an issue tracker on Github, so I guess it's just as simple as opening an issue there? That's a lot LOT easier than before, when it was on bugs.php.net.

Thanks for the fix, I'll test it and see if I can remove more of the workarounds in this library.

@nielsdos
Copy link

Yep it's just a matter of opening a GH issue. It doesn't even have to be much, just code is enough.
It's worth noting that for PHP 8.4 I have introduced new classes that fix a lot of crap in the DOM extension, full story in the RFC.

nielsdos added a commit to php/php-src that referenced this pull request May 12, 2024
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.

DocumentFragment append() vs appendChild()
2 participants