-
Notifications
You must be signed in to change notification settings - Fork 27
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
[fontique] fontconfig: allow_dtd for xml config #44
Conversation
The commit used in linebender/xilem#224 (4e6109f) doesn't match the head commit of this PR (3dd6f9b) Indeed, 4e6109f is apparently 3 weeks old? |
allow_dtd: true, | ||
nodes_limit: u32::MAX, | ||
}, | ||
) else { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not having any error handling here is a bit weird - there's a real difference between "the file doesn't exist, so try the next file" (?), and "the file does exist but is malformed XML"
Although I see that's a pre-existing choice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't have a great answer for this. We can surface some sort of error which I suppose would have helped us catch this particular bug but in the general case, there's no real way to guard against the user having either a broken font configuration or one that we're incapable of reading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving based on xilem#224
My previous comments still stand, but are not blocking
Thanks for catching the bad rev. I have no idea how I ended up with that value. |
Appears to fix the issue with the roxmltree update.