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

Throw on invalid entity #127

Merged
merged 1 commit into from
Oct 2, 2018
Merged

Conversation

mogsie
Copy link
Contributor

@mogsie mogsie commented Sep 28, 2018

This throws an error: Illegal XML entity 'foobar' if it sees the text &foobar;.

This is a breaking change, in that it changes the behaviour to be a standard. It can surely be described as a bugfix, because some might say that accepting invalid XML is a bug. The PR builds on #125 mainly because it changes the test introduced by #125, I wil happily rebase it if/when #125 is merged. The code change itself can be applied regardless of #125.

@mogsie mogsie force-pushed the throw-on-invalid-entity branch 4 times, most recently from e92e660 to 4765f56 Compare October 1, 2018 14:39
@mogsie
Copy link
Contributor Author

mogsie commented Oct 1, 2018

Rebased off master now that #125 was merged. I'm a bit conflicted as to if this should be considered a breaking change or not. It's a change that causes no change to how valid XML files are treated; it only causes invalid XML to be flagged.

Any client code that relies on this incorrect usage should not rely on the incorrect usage, so I'm more inclined to include this as a bugfix rather than (as I previously was thinking) consider it as a breaking change...

@sonnyp
Copy link
Member

sonnyp commented Oct 1, 2018

sounds like a bug fix to me, errors should be handled anyway

This is more in line with standard XML parsers.
@mogsie
Copy link
Contributor Author

mogsie commented Oct 2, 2018

I removed the word (breaking) from the commit message to reflect that. \o/

@sonnyp sonnyp merged commit 12d15fa into xmppjs:master Oct 2, 2018
@mogsie mogsie deleted the throw-on-invalid-entity branch October 3, 2018 15:21
@sonnyp
Copy link
Member

sonnyp commented Oct 3, 2018

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

Successfully merging this pull request may close these issues.

2 participants