-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
.NET Core transition: Inject service interfaces wherever possible #7369
Comments
I'm an Umbraco fan from version 3 or 4, so I decide to pick this one as my first contribution to the great ecosystem |
While this is a huge change I will break this task into many PRs per service @Shazwazza where should I register the services, I don't see |
|
Wonderful initiative @hishamco ! I'll create some review tasks to have a look at your PRs hopefully within the next week. I did notice that in some of your PRs that you are removing services from the ServiceContext but we don't want to do that. The ServiceContext shouldn't be changed. The main task for all of this is to remove the usages of the Thanks! |
@hishamco Ideally we want to have minimal amount of changes where necessary. While I appreciate the huge effort to remove the usages of ServiceContext it is not necessary and actually makes merge cases and the transition to the next version a little more difficult. Ideally we just want to have the |
Ok, I will revert my changes in the |
Hi @hishamco.. I also looked into some of the PRs, but none of them actually removes any usages of Here is an example of how to fix a single file (Diff version):
After
As you can see, before |
@bergmania I knew how to fix it ;)
Could you please refer me to one file that didn't have the change? perhaps it's missed accidentally |
@hishamco, if I look at the changes here: https://github.com/umbraco/Umbraco-CMS/pull/7390/files |
Let me check ... |
Hi @hishamco.. Based on the changes on the PRs, I still in doubt that you understand the meaning of this issue. Basically it is just these 110 occurrences that should be eliminated, or at least as many of them as possible:
|
Hi all. The WebApi Filter Attributes are probably going to take some extra attention, as well as any other usages of |
The attributes should probably be left until we are on the .NET Core, and can use of the TypeFilterAttribute or ServiceFilterAttribute to inject instances into attribute and filter constructors. I'll try to do some of there. One question though. There are a number of ones which use Current.Services in a default constructor to call a non-default constructor. Do you want these default constructors removed to remove the Current.Services dependency, or should we leave these alone for now? |
In then end, I only fixed 6 of the 110 references to Current.Services. The rest break out as follows: 50 - Unit tests (these could likely be done, but not tonight) |
Hi @benjaminc Good work.. I agree, most of them should just be left for now, but the unit tests could be nice to resolve before we close the issue.. |
Is solved by the final version of PR #7542 |
Fixed the unit tests here: #7644 I think we can close the issue when this is merged. |
Merged #7644! As @bergmania suggested, will close the issue |
Good to see this one is done now! Just to get back to @hishamco - thanks again for all your work! ⭐ We'll do our best for future tasks to describe what the intention is in a little more detail since we wouldn't want people to spend a lot of time on the wrong thing. 🙈 |
Really I spent a lot of time based on your earlier description .. anyhow I will come back to continue o OrchardCore, hopefully I do some contributions on Umbraco Core |
This task is up for grabs, for contributing to the .NET Core transition project
On the development branch of the .net core transition project
netcore/dev
, we have usedCurrent.Services.*
a lot.When this is used, we cannot move the class into Umbraco.Abstractions or Umbraco.Infrastructure.
This task is about eliminating the usage of
Current.Services.*
in general, by replacing it with a constructor injection of the interface of the given service that is in use. E.g.Current.Services.LocalizationService
has to be replaced with injection ofILocalizationService
.Expected Procedure:
Current.Services.*
.Current.Services.*
with the field.If any questions, feel free to ask :)
Feel free to resolve this task in multiple smaller PRs, Due to the fact long running feature branch most likely have merge conflict with
netcore/dev
The text was updated successfully, but these errors were encountered: