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 unescapeHTML #48

Merged
merged 3 commits into from
Nov 30, 2023
Merged

Fix unescapeHTML #48

merged 3 commits into from
Nov 30, 2023

Conversation

flosacca
Copy link
Contributor

The behavior of optimized_unescape_html is inconsistent with Ruby's implementation of unescapeHTML for some edge cases, such as &a>, where Ruby's implementation gives &a> but optimized_unescape_html leaves it unchanged because the second & is skipped after a is parsed.

@ioquatix
Copy link
Member

This looks like a reasonable problem to solve. Do you mind adding an initial commit with failing test cases?

@flosacca flosacca closed this Nov 27, 2023
@flosacca flosacca deleted the fix-unescape-html branch November 27, 2023 20:27
@flosacca flosacca restored the fix-unescape-html branch November 27, 2023 20:33
@flosacca flosacca reopened this Nov 27, 2023
@flosacca
Copy link
Contributor Author

Done.

@flosacca
Copy link
Contributor Author

It appears the added test cases make JRuby fail as well. It is now fixed.

Copy link
Member

@ioquatix ioquatix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@byroot
Copy link
Member

byroot commented Nov 28, 2023

Looks good to me, but I think @nobu is the CGI maintainer.

@byroot byroot requested review from nobu and removed request for byroot November 28, 2023 11:57
Copy link
Contributor

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also looks fine to me.

@nobu nobu merged commit e4c6337 into ruby:master Nov 30, 2023
29 checks passed
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.

5 participants