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 '#x0' string issue (issue #150) #151

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

Conversation

morgangiraud
Copy link

Fix issue #150

@subzey
Copy link
Contributor

subzey commented Apr 20, 2015

Please, note that #x0; is invalid in XML.
Entity must refer to valid Char and Char range doesn't include a null character.

@isaacs
Copy link
Owner

isaacs commented Apr 21, 2015

I'm perfectly happy to accept whatever you all decide is best, but every change needs to have a test.

@morgangiraud
Copy link
Author

Yeah you're right, If i include a test with a broken example which got fixed by my PR, will it be ok ?

@isaacs
Copy link
Owner

isaacs commented Apr 21, 2015

Sure. You can copy the examples from the other files in the tests folder. Please use a minimal amount of xml; it's rather rough to deal with tests that are multi-gb of real-world xml docs ;)

@thiakil
Copy link
Contributor

thiakil commented Oct 11, 2015

@subzey while it isn't valid once parsed, it is technically valid as a CharRef, just not the 'Well-formedness constraint'.

I believe we should still not barf on it in strict mode (whether we add a special case for not expanding/parsing it in strict mode or not) as it could be seen in the wild.

As for how to solve the issue, we are able to add a simple forward reference to the RegExp in question and the issue solves itself. See thiakil/sax-js@72ae06c (NB: next commits on that branch switches the test to strict mode and add more CharRefs to the test). Can make a pull request if wanted.

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