Cleanup ISmsService

[MikeAlhayek]

No description provided.

[hishamco]

While you working on SMS again, I'm still worrying why you didn't use KeyedServices instead of resolving different providers yourself?!!

[MikeAlhayek]

Because with keyed-services, you can't do this

OrchardCore/src/OrchardCore.Modules/OrchardCore.Sms/Drivers/SmsSettingsDisplayDriver.cs

Lines 52 to 53 in e759e2c

	.Where(entry => entry.Value.IsEnabled)
	.Select(entry => new SelectListItem(entry.Key, entry.Key))

You don't have access to the keys.

[hishamco]

That's why we need the PR #15100. IMHO let us:

1. Revert the PR changes
2. Use the strategy pattern as you did before, but using the keyed services under the hood

[MikeAlhayek]

@hishamco
I don't want to use keyed service for this.

This approach has an advantage over keyed service where we don't need to add extra services to the DI container since these provider are resolved without the being registered into the container. Also, it has the IsEnable flag for each service where one can enable/disable provider.

Also, I don't like that we have to implement a workaround for keyed-services in OC.

In this PR, I am just doing some cleanup to simplify the service

[hishamco]

This approach has an advantage over keyed service where we don't need to add extra services to the DI container since these provider are resolved without the being registered into the container. Also, it has the IsEnable flag for each service where one can enable/disable provider.

There's no extra service, what you are done is keeping the provider name in the options itself, which is the first time I saw in the Orchard Core code-base since the beginning

Also, I don't like that we have to implement a workaround for keyed-services in OC.

This is another thing, we already did something similar in Localization while not supporting ASP.NET Core, don't forget we can't run ASP.NET Core team in something we need

[hishamco]

Suggested change

	private readonly ISmsProviderResolver _smsProviderResolver;
	private ISmsProvider _provider;
	private ISmsProvider _provider;
	private readonly ISmsProviderResolver _smsProviderResolver;

[MikeAlhayek]

I usually leave the static variables top, then readonly then the writable privates. It's done that way everywhere

[hishamco]

You're right