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: XML::Reader#attribute_hash returns nil on error #2715

merged 1 commit into from Dec 7, 2022


Copy link

@flavorjones flavorjones commented Dec 6, 2022

What problem is this PR intended to solve?

Reader#attribute_hash does not behave the same way Reader#attribute_nodes behaved when parse errors are encountered.

For callers of Reader#attributes this restores the behavior from v1.13.7 before #2602.

See also the related change in #2714 which will land in v1.14.0 that raises a SyntaxError instead of returning nil.

Have you included adequate test coverage?

Yes, coverage added.

Does this change affect the behavior of either the C or the Java implementations?

The C and Java parsers differ in when they detect a syntax error in this case. libxml2 indicates an error when we try to expand a start tag that does not have a matching end tag, but xerces will defer the error until later. The tests reflect that difference.

@flavorjones flavorjones added this to the v1.13.x patch releases milestone Dec 6, 2022
@flavorjones flavorjones force-pushed the flavorjones-fix-reader-error-handling_v1.13.x branch from 3375c1f to f9d9061 Compare December 6, 2022 21:04
Note that on JRuby, the namespaces are still returned because the
parse error would raised on the subsequent node expansion.

This restores the behavior from v1.13.7
@flavorjones flavorjones force-pushed the flavorjones-fix-reader-error-handling_v1.13.x branch from f9d9061 to 9fe0761 Compare December 7, 2022 02:36
@flavorjones flavorjones merged commit 85410e3 into v1.13.x Dec 7, 2022
55 checks passed
@flavorjones flavorjones deleted the flavorjones-fix-reader-error-handling_v1.13.x branch December 7, 2022 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

1 participant