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

Refactor libXML error handling to remove global state #12663

Conversation

straight-shoota
Copy link
Member

libXML error handlers are now set right before calling a lib function collecting errors directly in that context. Afterwards the error handler is reset. There is no longer a global array collecting errors at XML::Error.errors.

The idea was sparked by @asterite in #12659 (comment), with explanation in #12659 (comment)

Resolves #12649
Closes #12652
Closes #12659

libXML error handlers are now set right before calling a lib function
collecting errors directly in that context. Afterwards the error handler
is reset. There is no longer a global array collecting errors at
`XML::Error.errors`.
src/xml/reader.cr Outdated Show resolved Hide resolved
src/xml/reader.cr Outdated Show resolved Hide resolved
straight-shoota and others added 2 commits October 26, 2022 12:01
Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

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

This is a sensible change, but it's also a breaking change. We need to discuss a path to incorporate it.

src/xml/error.cr Show resolved Hide resolved
src/xml/error.cr Outdated Show resolved Hide resolved
Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

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

I think this sounds better, wdyt?

src/xml/error.cr Outdated Show resolved Hide resolved
Co-authored-by: Beta Ziliani <beta@manas.tech>
@straight-shoota straight-shoota added this to the 1.7.0 milestone Nov 15, 2022
@straight-shoota straight-shoota merged commit 7a04fdd into crystal-lang:master Nov 17, 2022
@straight-shoota straight-shoota deleted the fix/xml-wrap-error-function branch November 17, 2022 00:02
@gdotdesign
Copy link

Hi, just letting you know this is a breaking change since the errors array is always there even if there are no errors while the type is still nillable. I had to do this for a workaround mint-lang/mint@3e4917d (#532)

@straight-shoota
Copy link
Member Author

Good point! The detail to return nil when empty got lost in the process. I don't think this is a great API design. But it probably makes sense to add this in 🤷 It's a simple check in the getter method.

straight-shoota added a commit to straight-shoota/crystal that referenced this pull request Nov 28, 2022
This restores the original semantics prior to the refactoring of crystal-lang#12663
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parse errors leaking from XML::Reader
4 participants