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
Implement RelationshipErrorHandler #1619
base: develop
Are you sure you want to change the base?
Conversation
v2.12.3 should be available soon with the fix. |
Thanks for the speedy fix, as usual, @twsouthwick |
cef9272
to
e8afa8e
Compare
…iption onto new line
…app.xml has changed.
e8afa8e
to
f76c253
Compare
bd90b33
to
cffc32a
Compare
@Pankraty Work in progress, but if you can start looking at this, it would be appreciated. |
cffc32a
to
6a43b5e
Compare
6a43b5e
to
7f57906
Compare
Sorry, this got lost among incoming messages. |
|
||
public XLWorkbook(LoadOptions loadOptions) | ||
{ | ||
EventTracking = loadOptions.EventTracking; |
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.
Do we still use EventTracking or we are keeping this for backward compatibility only? I thought, events processing was dropped long ago so maybe it's time to deprecate this flag?
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.
Just checked - LoadOptions.EventTracking
is only set and never read.
@@ -664,6 +664,8 @@ public XLWorkbook() | |||
internal XLWorkbook(String file, Boolean asTemplate) | |||
: this(new LoadOptions()) | |||
{ | |||
if (!asTemplate) throw new NotSupportedException(); |
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.
Why don't we fallback to ordinary loading? As a user, I would expect these two calls producing identical result:
var wb = new XLWorkbook(fileName);
var wb = new XLWorkbook(fileName, asTemplate: false);
[TestCaseSource(nameof(MalformedFiles))] | ||
public void CanSuccessfullyLoadMalformedFilesWithRemoveMalformedHyperlinksRelationshipErrorHandlerFactory(string file) |
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.
Perhaps, we should also have a test checking that opening these files without RemoveMalformedHyperlinksRelationshipErrorHandlerFactory
fails. If that test starts to fail one day, that will be an indicator for us that the factory is not needed anymore and we can consider removing (deprecating) it.
Fixes #249
Depends on dotnet/Open-XML-SDK#887The
RelationshipErrorHandler
requires modifying the package. It's an implicit assumption in ClosedXML that we don't do that when we load the package. Therefore, as a workaround, when an implementation ofRelationshipErrorHandler
is provided, the input stream is copied to a newMemoryStream
before the loading continues. This enables the handler to rewrite the package as required without touching the user-supplied file or stream.