diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f7ea16d0cc..7f6f5313002 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,11 @@ This release ends support for: * [JRuby] `Node#line` behavior has been modified to return the line number of the node in the _final DOM structure_ (the value returned in JRuby is increased by 1 to count the XML prolog). This behavior is different from CRuby, which returns the node's position in the _input string_. This difference is not ideal, but at least is now officially documented and tested. The real-world impact of this change is that the value returned in JRuby is greater by 1 to account for the XML prolog in the output. [[#2380](https://github.com/sparklemotion/nokogiri/issues/2380)] (Thanks, [@dabdine](https://github.com/dabdine)!) +### Deprecated + +* Passing a `Nokogiri::XML::Node` as the second parameter to `Node.new` is deprecated and will generate a warning. This will become an error in a future version of Nokogiri. [[#975](https://github.com/sparklemotion/nokogiri/issues/975)] + + ## 1.12.5 / 2021-09-27 ### Security diff --git a/ext/java/nokogiri/XmlNode.java b/ext/java/nokogiri/XmlNode.java index f7feb43b51f..ee8d970a9f2 100644 --- a/ext/java/nokogiri/XmlNode.java +++ b/ext/java/nokogiri/XmlNode.java @@ -305,6 +305,14 @@ public class XmlNode extends RubyObject IRubyObject name = args[0]; IRubyObject doc = args[1]; + if (!(doc instanceof XmlNode)) { + throw context.runtime.newArgumentError("document must be a Nokogiri::XML::Node"); + } + if (!(doc instanceof XmlDocument)) { + // TODO: deprecate allowing Node + context.runtime.getWarnings().warn("Passing a Node as the second parameter to Node.new is deprecated. Please pass a Document instead, or prefer an alternative constructor like Node#add_child. This will become an error in a future release of Nokogiri."); + } + Document document = asXmlNode(context, doc).getOwnerDocument(); if (document == null) { throw context.runtime.newArgumentError("node must have owner document"); diff --git a/ext/nokogiri/xml_node.c b/ext/nokogiri/xml_node.c index ba9e60c85e8..14f1a871ff9 100644 --- a/ext/nokogiri/xml_node.c +++ b/ext/nokogiri/xml_node.c @@ -1765,24 +1765,31 @@ rb_xml_node_line_set(VALUE rb_node, VALUE rb_line_number) static VALUE rb_xml_node_new(int argc, VALUE *argv, VALUE klass) { - xmlDocPtr doc; - xmlNodePtr node; - VALUE name; - VALUE document; + xmlNodePtr c_document_node; + xmlNodePtr c_node; + VALUE rb_name; + VALUE rb_document_node; VALUE rest; VALUE rb_node; - rb_scan_args(argc, argv, "2*", &name, &document, &rest); + rb_scan_args(argc, argv, "2*", &rb_name, &rb_document_node, &rest); - Data_Get_Struct(document, xmlDoc, doc); + if (!rb_obj_is_kind_of(rb_document_node, cNokogiriXmlNode)) { + rb_raise(rb_eArgError, "document must be a Nokogiri::XML::Node"); + } + if (!rb_obj_is_kind_of(rb_document_node, cNokogiriXmlDocument)) { + // TODO: deprecate allowing Node + rb_warn("Passing a Node as the second parameter to Node.new is deprecated. Please pass a Document instead, or prefer an alternative constructor like Node#add_child. This will become an error in a future release of Nokogiri."); + } + Data_Get_Struct(rb_document_node, xmlNode, c_document_node); - node = xmlNewNode(NULL, (xmlChar *)StringValueCStr(name)); - node->doc = doc->doc; - noko_xml_document_pin_node(node); + c_node = xmlNewNode(NULL, (xmlChar *)StringValueCStr(rb_name)); + c_node->doc = c_document_node->doc; + noko_xml_document_pin_node(c_node); rb_node = noko_xml_node_wrap( klass == cNokogiriXmlNode ? (VALUE)NULL : klass, - node + c_node ); rb_obj_call_init(rb_node, argc, argv); diff --git a/lib/nokogiri/html5.rb b/lib/nokogiri/html5.rb index d3976c5edaa..c980efa8a77 100644 --- a/lib/nokogiri/html5.rb +++ b/lib/nokogiri/html5.rb @@ -254,6 +254,7 @@ def self.fragment(string, encoding = nil, **options) # * :follow_limit => number of redirects which are followed # * :basic_auth => [username, password] def self.get(uri, options = {}) + # TODO: deprecate warn("Nokogiri::HTML5.get is deprecated and will be removed in a future version of Nokogiri.", uplevel: 1, category: :deprecated) get_impl(uri, options) diff --git a/lib/nokogiri/xml/node.rb b/lib/nokogiri/xml/node.rb index e6c7b9819e4..ce731ce9252 100644 --- a/lib/nokogiri/xml/node.rb +++ b/lib/nokogiri/xml/node.rb @@ -119,7 +119,7 @@ class Node # # [Parameters] # - +name+ (String) - # - +document+ (Nokogiri::XML::Document) + # - +document+ (Nokogiri::XML::Document) The document to which the the returned node will belong. # [Yields] Nokogiri::XML::Node # [Returns] Nokogiri::XML::Node # diff --git a/test/xml/test_node.rb b/test/xml/test_node.rb index 90c13edd403..2f4db3e8116 100644 --- a/test/xml/test_node.rb +++ b/test/xml/test_node.rb @@ -726,6 +726,16 @@ def test_xml_node_new_with_block assert_equal(node, block_param, "Node.new block should be passed the new node") end + def test_xml_node_new_must_take_document_type + assert_raises(ArgumentError) do + Nokogiri::XML::Node.new("input", "not-a-document") + end + + assert_output(nil, /deprecated/) do + Nokogiri::XML::Node.new("input", xml.root.children.first) + end + end + def test_to_str name = xml.xpath("//name").first assert_match(/Margaret/, "" + name) diff --git a/test/xml/test_unparented_node.rb b/test/xml/test_unparented_node.rb index c13ed264a60..b214179ee91 100644 --- a/test/xml/test_unparented_node.rb +++ b/test/xml/test_unparented_node.rb @@ -91,7 +91,7 @@ def test_each end def test_new - assert(node = Nokogiri::XML::Node.new("input", @node)) + assert(node = Nokogiri::XML::Node.new("input", @node.document)) assert_equal(1, node.node_type) end @@ -423,7 +423,7 @@ def test_encode_special_chars end def test_content - node = Nokogiri::XML::Node.new("form", @node) + node = Nokogiri::XML::Node.new("form", @node.document) assert_equal("", node.content) node.content = "hello world!" @@ -445,7 +445,7 @@ def test_replace first = set[0] second = set[1] - node = Nokogiri::XML::Node.new("form", @node) + node = Nokogiri::XML::Node.new("form", @node.document) first.replace(node) assert(set = @node.search(".//employee"))