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

Restore old namespace reconciliation behaviour #12308

Closed
wants to merge 1 commit into from

Conversation

nielsdos
Copy link
Member

The xmlDOMWrapReconcileNamespaces method we used to fix the namespace corruption issues in 8.1.21/8.2.8 caused regressions. Primarily, there is a similar corruption that the xmlReconciliateNs method used to have in which a namespace is suddenly shifted (SAML-Toolkits/php-saml#562) and the side-effect of removing redundant namespaces causes problems when a specific serialization is required.

REMINDER FOR SELF: there's an additional reconciliation in the lifetime management code in PHP>=8.3!

This means the following issues will re-open:

For now, restore the old behaviour while we think of a general solution (e.g. something related with opt-in using the new classes).

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.

Minor comments, but LGTM

Comment on lines -1 to -5
--TEST--
Bug #47847 (importNode loses the namespace of an XML element)
--EXTENSIONS--
dom
--FILE--
Copy link
Member

Choose a reason for hiding this comment

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

Possibly keep the tests but just mark them as XFAIL with a link to this PR to explain rationale for the revert?

@@ -1489,11 +1489,15 @@ static void dom_reconcile_ns_internal(xmlDocPtr doc, xmlNodePtr nodep, xmlNodePt

static void dom_libxml_reconcile_ensure_namespaces_are_declared(xmlNodePtr nodep)
{
#if 0
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment to explain why this code is dead?

@nielsdos nielsdos closed this in e127f87 Sep 27, 2023
@nielsdos
Copy link
Member Author

Fixed the remarks in the merge, thanks for checking.

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.

None yet

2 participants