Restore Builder namespace inheritance behavior, and introduce Document#namespace_inheritance#2320
Merged
flavorjones merged 3 commits intomainfrom Aug 29, 2021
Conversation
This was referenced Aug 29, 2021
flavorjones
force-pushed
the
2317-builder-namespace-inheritance_based_on_main
branch
from
dff6282 to
29f7f22
Compare
flavorjones
force-pushed
the
2317-builder-namespace-inheritance_based_on_main
branch
from
29f7f22 to
2cfe7b0
Compare
This restores the Builder behavior from Nokogiri versions before 1.12.0. This can be switched off by passing a kwarg to the Builder initializer. See #2317.
flavorjones
force-pushed
the
2317-builder-namespace-inheritance_based_on_main
branch
from
2cfe7b0 to
59cd533
Compare
flavorjones
deleted the
2317-builder-namespace-inheritance_based_on_main
branch
flavorjones
added a commit
that referenced
this pull request
Aug 29, 2021
commit cb2b9cfae3bc335beea7300f813a7f3e0b2776f1
Author: Mike Dalessio <mike.dalessio@gmail.com>
Date: Wed Aug 18 20:12:41 2021 -0400
update CHANGELOG
commit c5074b51f26f9b72bacf11c65f16d637440d93fc
Author: Mike Dalessio <mike.dalessio@gmail.com>
Date: Sat Aug 28 15:19:02 2021 -0400
feat/fix: re-enable namespace_inheritance for Builder documents
This restores the Builder behavior from Nokogiri versions before
1.12.0. This can be switched off by passing a kwarg to the Builder
initializer. See #2317.
commit 3814b47d4c3503cf4960caa2d0c5af84b982ee80
Author: Mike Dalessio <mike.dalessio@gmail.com>
Date: Sat Aug 28 15:15:07 2021 -0400
feat/fix: introduce Document#namespace_inheritance attr
When true, reparented elements without a namespace will inherit their
new parent's namespace (if one exists). Defaults to +false+.
This is part of addressing the behavior change introduced in v1.12 by
1f483f0, allowing us to switch it on or off per-document. See #2317.
flavorjones
added a commit
to flavorjones/signer
that referenced
this pull request
Aug 29, 2021
Nokogiri v1.12 introduced a breaking change related to namespace inheritance of reparented child nodes. This commit using the feature added in v1.12.4 to opt - for new versions of Nokogiri that support it, set `Document#namespace_inheritance` - intermediate versions of Nokogiri will be avoided via gemspec version specification - old versions of Nokogiri will continue to work See sparklemotion/nokogiri#2320 for more details. Fixes ebeigarts#30
flavorjones
added a commit
to flavorjones/Viewpoint
that referenced
this pull request
Aug 29, 2021
Nokogiri v1.12.0 introduced a breaking change that is fixed in v1.12.4. This PR will avoid those broken versions. See sparklemotion/nokogiri#2320
pcai
pushed a commit
to WinRb/Viewpoint
that referenced
this pull request
Jul 8, 2024
Nokogiri v1.12.0 introduced a breaking change that is fixed in v1.12.4. This PR will avoid those broken versions. See sparklemotion/nokogiri#2320
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem is this PR intended to solve?
Document#namespace_inheritanceatttribute which defaults tofalseXML::BuildersetsDocument#namespace_inheritancetotruebut allows it to be overridden via constructor parameterI'd like this to be backported to v1.12.
Deeper context
Prior to v1.12.0, the CRuby and JRuby implementations had different behavior with respect to how children inherit namespaces when added to a parent node (via the
Node#add_childfamily of methods). In CRuby, children without a namespace would automatically inherit their new parent's namespace (a bug originally reported in 2011 at #425). In JRuby, this was not the case and the child would either have the document's default namespace or no namespace.In v1.12.0, a intentional change was introduced to the CRuby implementation to bring it in line with the JRuby implementation. While the maintainers consider this a "bug" "fix", it was nevertheless a breaking change for some libraries and users (see, for example, ebeigarts/signer#30).
In addition, the intentional "fix" in v1.12.0 introduced an unintentional regression in
Nokogiri::XML::Builderin which we actually do want children to inherit their parent's namespace by default (see, for example, #2317).That said, there's evidence that users may want different namespace inheritance behavior depending on the situation. For example, #1712 requests a way to avoid Builder namespace inheritance.
Have you included adequate test coverage?
Yes. Historically the incompleteness of test coverage around namespaces during reparenting has been an issue, but I believe we now have adequate coverage to prevent regressions.
Does this change affect the behavior of either the C or the Java implementations?
Previously the C and Java implementations had different behavior; v1.12.0 improved that somewhat; this changeset includes changes to bring them completely in line with each other (as far as our current test coverage goes).