Convert Users ControllersExtensions into Services #11002
Conversation
|
I know these have been extension methods since the beginning but something I've always wondered is, would these be better as services? It always stuck me as a bit odd that they are extension methods instead of service methods, but maybe there was a good reason for that. Since you are making them public, maybe this would be a good opportunity to refactor them into services. Since they were internal before I don't think moving them to services would be a breaking change. |
|
I would prefer them as services, same with the extension method being added on the tenants pr. Just seems to make more sense to me, as I've fought around them in the past. changing how email is sent is a common requirement, and it's quite hard to do with those extensions and you're quite right @rjpowers10 non breaking. |
|
I will try to convert them as service, but naming is hard ;) while they provide different functionality to the controller according the requirement. Hope to send a PR today |
eeed746 to
dc256ec
Compare
|
@deanmarcussen please let me know if you have any concern about the latest changes, so I can reflected into tenants PR |
|
Any idea why it fails on Ubuntu? |
|
Just saw that |
src/OrchardCore.Modules/OrchardCore.Users/Controllers/ControllerExtensions.cs
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Users/Services/DefaultControllerService.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Users/Services/DefaultControllerService.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Users/Services/DefaultControllerService.cs
Outdated
Show resolved
Hide resolved
| public static async Task<bool> SendAsync(this ISmtpService smtpService, string email, string subject, IShape model) | ||
| { | ||
| var body = String.Empty; | ||
| var displayHelper = ShellScope.Services.GetService<IDisplayHelper>(); |
There was a problem hiding this comment.
@jtkech is this a valid usage or is there a proper way to get such service
|
Seems I broke th unit tests ;) |
|
BTW can we rename the service |
|
Just curious: why no interface for the service? I see that there are virtual methods instead. |
|
@rjpowers10 we can introduce as an interface but I was thinking this services for a controller only. Anyhow all the feedback are welcome |
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
|
Closed in favor of #15360 |
No description provided.