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

Handle empty xml:base #294

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chrisguttandin
Copy link

I noticed that a feed with an empty xml:base="" breaks the parsing algorithm in a strange way. I came across such a feed in the wild. This PR attempts to fix that. Please let me know if anything needs to be changed.

I added a new test fixture which is a copy of test/feeds/tpm.atom. The only difference is that I added xml:base="".

The test that I added breaks with the new fixture. The additional checks fix the problem. I'm not sure though if it's the best way to do this as I'm not familiar with the code base. Please let me know if it should be done differently.

@danmactough
Copy link
Owner

@chrisguttandin Thanks so much for reporting this and submitting a fix. 🤗 Turns out the relevant spec has a special-case rule for when the xml:base is an empty string:

RFC 3986 defines certain relative URI references, in particular the empty string and those of the form #fragment, as same-document references. Dereferencing of same-document references is handled specially. However, their use as the value of an xml:base attribute does not involve dereferencing, and XML Base processors should resolve them in the usual way. In particular, xml:base="" does not reset the base URI to that of the containing document.

It would be awesome if you could update your PR to implement that logic, if you're up for it.

@chrisguttandin
Copy link
Author

Hi @danmactough, thanks for taking a look. I would be happy to make the fix spec compliant but I'm not sure what the spec is trying to say.

As far as I understand it it says an empty xml:base should be ignored. At least it says it should not do dereferencing and it should not reset the base URI.

Sorry for the dumb question, but what would need to change in the implementation to make it spec compliant?

@chrisguttandin
Copy link
Author

Hi @danmactough, could you please let me know what needs to be changed in order to make these changes spec compliant?

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

Successfully merging this pull request may close these issues.

None yet

2 participants