Skip to content

Fix bug #55294 and #47530 and #47847: namespace reconciliation issues #11454

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

@nielsdos nielsdos commented Jun 14, 2023

Fixes #55294
Fixes #47530
Fixes #47847

We'll use the DOM wrapper version of libxml2 instead of the regular one. It's conforming to the behaviour we expect of DOM. Most of this patch is tests.

I based and extended the tests on the code attached with the aforementioned bug reports. Therefore the credits for the tests:
Co-authored-by: hilse at web dot de
Co-authored-by: robin2008 at altruists dot org
Co-authored-by: sgunderson at bigfoot dot com

We'll also change the searching point of the internal reconciliation to start at the top of the added tree to avoid redundant work now that the function is changed.

Merging this PR will unblock making progress on GH-11428.

We'll use the DOM wrapper version of libxml2 instead of the regular one.
It's conforming to the behaviour we expect of DOM.
Most of this patch is tests.

I based and extended the tests on the code attached with the aforementioned
bug reports. Therefore the credits for the tests:
Co-authored-by: hilse at web dot de
Co-authored-by: robin2008 at altruists dot org
Co-authored-by: sgunderson at bigfoot dot com

We'll also change the searching point of the internal reconciliation to
start at the top of the added tree to avoid redundant work now that the
function is changed.
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.

I'm kinda assuming that the test outputs are correct, as I don't really know the DOM spec that well.

One thing that might make reviews easier is to have an initial commit with the tests showing the current output and the second commit fixing the issue also amending the test to the expected behaviour.

This process doesn't make a lot of sense for most PRs, but it might be useful for DOM ones considering that most of the output is not gonna change and seeing a small diff clearly showing what is getting fixed in the output would make it easier to verify, well at least I think!

But the code looks logical and sensible. :)

@nielsdos
Copy link
Member Author

Thanks for the review.
I like your suggestion of doing an initial commit with the wrong output just for getting the diff. I'll do that in the future.
I've done this now for this PR here: nielsdos@6e39d1d in case you are interested.

@nielsdos nielsdos closed this in b30be40 Jun 15, 2023
@Girgias
Copy link
Member

Girgias commented Jun 15, 2023

This indeed makes the fix very obvious :D

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.

2 participants