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 issue #1508. Throw exception when registering invalid extension assembly/type. #1517

Closed
wants to merge 2 commits into from
Closed

Fix issue #1508. Throw exception when registering invalid extension assembly/type. #1517

wants to merge 2 commits into from

Conversation

Jeinhaus
Copy link
Contributor

See issue #1508.
The problem seemed to be that there were multiple try-catches around the methods loading the assemblies, so that the exception did not bubble out of ParseExtensionsElement(NLogXmlElement extensionsElement, string baseDirectory).
I removed the try-catches in there, so that a NLogConfigurationException is created in the try-catch in the toplevel method XmlLoggingConfiguration.Initialize(XmlReader reader, string fileName, bool ignoreErrors).

Let me know if this fixes the issue and if you see any problems with removing the inner try-catches.

…gistering an assembly or type with the <extensions> Element (with <nlog throwConfigExceptions='true'>).
@Jeinhaus
Copy link
Contributor Author

Mhhh. I don't get why that one test is not passing with appveyor.
And it seems like travis-ci has a problem.

@@ -406,7 +406,7 @@ private void Initialize(XmlReader reader, string fileName, bool ignoreErrors)

if (!ignoreErrors)
{
if (exception.MustBeRethrown())
if (configurationException.MustBeRethrown())
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a good change

@304NotModified
Copy link
Member

Thanks for the PR!

On holiday now but i checked it in short, see comments.

One unit test is broken, but I'm sure the 2 catches should be restored.

I restarted travis-ci.org, it was bugged

Add two new testcases for invalid extension registration.
@Jeinhaus
Copy link
Contributor Author

Alright, I readded the try-catches. You were right. If they were omitted the node wasn't correctly parsed.
Now it should continue parsing the <add .../> elements, even if one doesn't exist (if ThrowConfigExtensions==false).

@304NotModified
Copy link
Member

could you check why InvalidXMLConfiguration_DoesNotThrowErrorWhen_ThrowExceptionFlagIsNotSet fails?

@Jeinhaus
Copy link
Contributor Author

Jeinhaus commented Jul 3, 2016

Mh, I don't get why that test fails on Mono. It passes on my machine. Do you have any recommendations how to debug this?

@304NotModified
Copy link
Member

I have fixed the unit test and merged the code in #1553

Thanks for the PR!

@304NotModified
Copy link
Member

It's online!

@Jeinhaus
Copy link
Contributor Author

Yey, my first PR got merged!
Thank you for your help in figuring this one out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants