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

Allows loading dependencies during startup even if one fails #2953

Merged
merged 2 commits into from
Aug 18, 2019

Conversation

valadas
Copy link
Contributor

@valadas valadas commented Aug 18, 2019

Closes #2952

Previously if one type in an assembly was failing, all the types of that assembly where ignored by the startup code. This PRs fixes this and returns all the other types to prevent odd behaviours.

@valadas valadas added this to the 9.4.0 milestone Aug 18, 2019
Copy link
Contributor

@mitchelsellers mitchelsellers left a comment

Choose a reason for hiding this comment

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

I reviewed this with Daniel as he created it. Additionally supporting documentation on this behavior can be found on MSDN documentation as well as here: https://haacked.com/archive/2012/07/23/get-all-types-in-an-assembly.aspx/

This looks great for now. It also begs a few additional changes for the future, but this improves the behavior now, which will substantially improve developer experience, with no impact to performance.

Copy link
Contributor

@david-poindexter david-poindexter left a comment

Choose a reason for hiding this comment

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

So glad we caught this early on - this would have been tough for 3rd party extension developers to figure out. Great work!

@david-poindexter david-poindexter merged commit c2ef4fe into dnnsoftware:release/9.4.x Aug 18, 2019
@mitchelsellers
Copy link
Contributor

And just for the record I believe this has been there for a while. I’m not 100% sure if new behavior but for sure something we need to monitor in future PRs

catch (Exception)
{
{
//TODO: We should log the reason of the exception after the API cleanup
Copy link
Contributor

Choose a reason for hiding this comment

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

@valadas can we create a follow-up issue to track this //TODO. If there is already one created can you link it in this PR

@@ -32,8 +33,15 @@ public static Type[] SafeGetTypes(this Assembly assembly)
{
types = assembly.GetTypes();
}
catch (ReflectionTypeLoadException ex)
{
//TODO: We should log the reason of the exception after the API cleanup
Copy link
Contributor

Choose a reason for hiding this comment

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

@valadas can we create a follow-up issue to track this //TODO. If there is already one created can you link it in this PR

@SkyeHoefling
Copy link
Contributor

This looks great! Are there follow-up items created for the //TODOs?

@valadas
Copy link
Contributor Author

valadas commented Sep 1, 2019

I did not create an issue for it, but the //TODO: syntax shows up in the visual studio tasks panel. The reason we have not implemented it yet is because it would have created a new dependency for the logging here and we wanted to push that to later. So maybe we can do that when we start using more dependency injection..

valadas added a commit that referenced this pull request Sep 1, 2019
* Fix error running unit tests (#2940)

When running tests, the following error occurs:
	VSTest: Could not locate executable.

cake-build/cake#1522 (comment)
indicates that there's an issue using VS 2019, and presents a workaround
to find the correct path

* DNN-31366 - Deleted page can still be selected as parent page (#2925)

* DNN-31366 - TabController HasChildren returns false if all children are deleted

* Simplify HasChildren condition

* deleting_themes_from_Extensions_doesnt_remove_it_from_themes (#2950)

* Returns the working types if some fail (#2953)
@valadas valadas deleted the issue-2952 branch February 10, 2020 16:22
DnnSoftwarePersian pushed a commit to DnnSoftwarePersian/Dnn.Platform that referenced this pull request May 17, 2020
…e#2964)

* Fix error running unit tests (dnnsoftware#2940)

When running tests, the following error occurs:
	VSTest: Could not locate executable.

cake-build/cake#1522 (comment)
indicates that there's an issue using VS 2019, and presents a workaround
to find the correct path

* DNN-31366 - Deleted page can still be selected as parent page (dnnsoftware#2925)

* DNN-31366 - TabController HasChildren returns false if all children are deleted

* Simplify HasChildren condition

* deleting_themes_from_Extensions_doesnt_remove_it_from_themes (dnnsoftware#2950)

* Returns the working types if some fail (dnnsoftware#2953)
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.

None yet

4 participants