-
Notifications
You must be signed in to change notification settings - Fork 745
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
DI: Register MVC/WebApi Controllers using TryAddScoped. #3544
Conversation
This will allow vendors to add/replace controller registrations with their own. Fix AddWebApiControllers typo for AddMvcControllers.
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.
This looks fine to me, thanks @dimarobert!
@bdukes I see that this is part of the 9.5.0 milestone, but what do you think are the chances that it will get merged by then? |
@valadas @ahoefling @mitchelsellers @donker any opinions about getting this change in 9.5.0? I believe this is blocking DNN Sharp modules from working on 9.5.0. |
I have no objection merging this in 9.5.0 providing we redo some tests on MVC and SPA modules, but I am no expert in the DI area, you those who know more than me agree, I have no issue retargetting to 9.5.0 personally... Can anyone test DnnSharp modules with this change? |
We'll have new builds in the next couple of days that should work with 9.5.0. With this PR it should be easier for us to maintain both DNN versions (pre 940 and 950+) without separate install packages for our products. |
Please let us know if this can be merged in DNN 9.5 Thanks! |
Since 9.5.0 was code frozen for about 2 weeks and would get officially released like tomorrow, this pr would not have not went through the testing team. My vote would be to release 9.5.0 as it is ASAP and do a quick 9.5.1 Rc1 with this and other bug fixes that are ready this week with just a 1 week RC stage... Opinions? |
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.
LGTM. I agree with Valadas as this does require a complete test cycle. Including performance
Thanks @dimarobert for the contribution, unfortunately we dropped the ball getting this reviewed for 9.5.0, but this will be included in 9.5.1 (which we're planning to fast-track, RC should be released soon). |
Fixes #3543
Summary
This is a quick fix for the assigned issue.
This will allow vendors to add/replace controller registrations with their own.
The DefaultControllerFactory is also added to the ServiceCollection using the
TryAddSingleton
for the same reasons.Fix AddWebApiControllers typo for AddMvcControllers.