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

Fix EmptyStackException thrown by elements with xlink:href attributes and no base_uri. #534

Closed
wants to merge 1 commit into from

Conversation

bilts
Copy link

@bilts bilts commented Sep 8, 2011

Nokogiri::XML::Reader required elements with xlink:href attributes to be contained by elements with xml:base attributes. ReaderNode would throw an EmptyStackException if this wasn't the case.

The xlink spec is ambiguous about whether that situation is allowed, but a few online xlink examples and our real-world data have xlink:href attributes with no xml:base.

Nokogiri uses xlink:href to compute the base_uri attribute, but I didn't see any tests for this, so I wrote two. The second one fails without my fix.

@flavorjones
Copy link
Member

Wow, sorry for not responding to this before now.

LibXML behaves slightly differently here, and I'm wondering which is the correct behavior. It appears from the test case that the Java implementation will return "#other" on the end of the URI, while LibXML does not. That is, in the first test, LibXML returns "http://base.example.org/base/".

Similarly for the second test, LibXML returns nil.

From a brief review of RFC 2396 (http://www.ietf.org/rfc/rfc2396.txt) it's not clear to me which is correct. Anybody have an informed opinion?

@quoideneuf
Copy link
Contributor

This is not an informed opinion, but I would say that the behavior in MRI is correct. It seems to me that the base uri doesn't have much to do with the xlink:href value, and that if the xlink:href contains a fragment that fragment shouldn't be appended to the base_uri. In any case, I am going to post a pull request with modification similar to what @bilts posted above, but with conformance to the MRI behavior, e.g.,:
"http://base.example.org/base/" for test 1.
nil for test 2.

@flavorjones
Copy link
Member

Merged dc4501a from pull request #805 which fixes this issue.

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

Successfully merging this pull request may close these issues.

4 participants