Skip to content

[feature] document.parent should exist #3466

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

Open
doriantaylor opened this issue Mar 12, 2025 · 7 comments · May be fixed by #3481
Open

[feature] document.parent should exist #3466

doriantaylor opened this issue Mar 12, 2025 · 7 comments · May be fixed by #3481

Comments

@doriantaylor
Copy link
Contributor

doriantaylor commented Mar 12, 2025

IMO this should work for parity when the node type is unknown: document.parent == document.

Consider the following:

$ pry
[1] pry(main)> require 'nokogiri'
=> true                          
[2] pry(main)> document = Nokogiri.XML '<foo/>' 
=> #(Document:0x140 {                   
  name = "document",
  children = [ #(Element:0x154 { name = "foo" })]
  })
[3] pry(main)> document.parent
NoMethodError: undefined method `parent' for #<Nokogiri::XML::Document:0x140 name="document" children=[#<Nokogiri::XML::Element:0x154 name="foo">]>
from (pry):4:in `__pry__'

Expected behavior

It shouldn't crash (indeed, the documentation says the method is there). document.parent should just be a no-op that returns self.

Environment

# Nokogiri (1.18.3)
    ---
    warnings: []
    nokogiri:
      version: 1.18.3
      cppflags:
      - "-I/var/lib/gems/3.2.0/gems/nokogiri-1.18.3-x86_64-linux-gnu/ext/nokogiri"
      - "-I/var/lib/gems/3.2.0/gems/nokogiri-1.18.3-x86_64-linux-gnu/ext/nokogiri/include"
      - "-I/var/lib/gems/3.2.0/gems/nokogiri-1.18.3-x86_64-linux-gnu/ext/nokogiri/include/libxml2"
      ldflags: []
    ruby:
      version: 3.2.3
      platform: x86_64-linux-gnu
      gem_platform: x86_64-linux-gnu
      description: ruby 3.2.3 (2024-01-18 revision 52bb2ac0a6) [x86_64-linux-gnu]
      engine: ruby
    libxml:
      source: packaged
      precompiled: true
      patches:
      - 0001-Remove-script-macro-support.patch
      - 0002-Update-entities-to-remove-handling-of-ssi.patch
      - '0009-allow-wildcard-namespaces.patch'
      - 0010-update-config.guess-and-config.sub-for-libxml2.patch
      - 0011-rip-out-libxml2-s-libc_single_threaded-support.patch
      - '0019-xpath-Use-separate-static-hash-table-for-standard-fu.patch'
      memory_management: ruby
      iconv_enabled: true
      compiled: 2.13.6
      loaded: 2.13.6
    libxslt:
      source: packaged
      precompiled: true
      patches:
      - 0001-update-config.guess-and-config.sub-for-libxslt.patch
      datetime_enabled: true
      compiled: 1.1.42
      loaded: 1.1.42
    other_libraries:
      zlib: 1.3.1
      libgumbo: 1.0.0-nokogiri
@doriantaylor doriantaylor added the state/needs-triage Inbox for non-installation-related bug reports or help requests label Mar 12, 2025
@flavorjones
Copy link
Member

@doriantaylor I'm not opposed to this in principle. Would you be willing to draft a pull request and seeing what (if any) tests are failing? And maybe seeing what's involved to get them to pass?

@flavorjones flavorjones changed the title [bug] document.parent fails with NoMethodError [feature] document.parent should exist Mar 12, 2025
@flavorjones flavorjones added meta/feature-request and removed state/needs-triage Inbox for non-installation-related bug reports or help requests labels Mar 12, 2025
@flavorjones
Copy link
Member

Can you say more about your use case? I've never really run into a situation where I didn't know the object was a Document as opposed to a non-document Node. If you could share some real-world code it would help me understand your motivation.

@nwellnhof
Copy link

While libxml2 also sets a document's parent to the document itself, I consider that a design mistake. Conceptually, the chain of ancestors should be a linked list and setting doc.parent = doc introduces a cycle that can lead to infinite loops if you're not careful. I'd suggest to make doc.parent return nil, so something like the following works:

while node
    ...
    node = node.parent
end

@doriantaylor
Copy link
Contributor Author

doriantaylor commented Mar 16, 2025

You're right; my original use case was a function that took an arbitrary node as an argument but didn't have to care about what kind of node it was, and had logic to orient itself, but I had written it with libxml2 in my head.

I went and double-checked the DOM spec (well, MDN because WHATWG is a dog's breakfast) the following turns out to be the case for parentNode or ownerDocument. For parity with DOM, both those properties should be present on a document node, but return nil. (The current behaviour, ostensibly, is doc.parent raises a NoMethodError despite being in the documentation, and doc.document returns self.)

@flavorjones
Copy link
Member

@doriantaylor Again, I'm open to changing this behavior if we agree (following the advice of @nwellnhof who's the libxml2 maintainer above) that the desired behavior is for Document#parent to return nil. Would you be interested in drafting a pull request to do this?

@flavorjones
Copy link
Member

I've created #3481 for this change.

@flavorjones
Copy link
Member

flavorjones commented Mar 24, 2025

@doriantaylor I also just want to respond to the fact that you (twice!) mentioned that Document#parent is documented ... it's not. You are referencing docs generated by yard, and not rdoc, and yard doesn't seem to recognize when methods have been undefed. I can't control what's on the internet!

Official docs, generated by rdoc, do not currently include #parent: https://nokogiri.org/rdoc/Nokogiri/XML/Document.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants