tschettler edited a comment on pull request #73:
URL: https://github.com/apache/logging-log4net/pull/73#issuecomment-953506178
@fluffynuts you are correct, due to the way types are instantiated in
log4net, using a service locator is the way I would do this now. However, when
I mention "DI", I'm not referring to a service locator per se. I'm rather
referring to the dependency inversion principle, where the dependencies for a
class are provided instead of having to "new" them up and tightly couple low
level modules and higher level modules. The service locator is a means to this
end, and unfortunately, one of the only (if not, the only) means when
attempting to use dependency inversion with log4net.
To decouple high level and low level modules with no changes to log4net, the
consuming application must expose some sort of `GetService` method from the
composition root, consequently encouraging the use of the service locator
pattern when working with log4net, if one wishes to use dependency inversion.
This exposed service locator would then be referenced from say a custom pattern
converter to pull in the necessary dependency within its implementation and not
as an injected dependency.
I agree with your sentiment on the service locator pattern, it should be
avoided when possible. However, if one wants to achieve loose coupling within
custom log4net components, service locator is the only option that I see. I
think it would be amazing if log4net would "buy into the DI system from the
get-go", but the changes this would require are much more significant and far
reaching in my opinion. Unless/Until that happens, as it is right now, service
locator in one shape or another is the only real option. The benefit of
allowing a very constrained service locator within log4net is that it would -
with minimal changes - encourage developers to use proper dependency injection
when utilizing log4net, rather than requiring the use of the service locator
pattern to achieve the same results.
Presently, with service locator:
```cs
public class Program {
// the developer needs to expose the service locator in order to allow
the component to resolve dependencies.
public static IServiceLocator ServiceLocator =
Container.GetServiceResolver();
}
public class HttpContextPatternLayoutConverter : PatternLayoutConverter {
private IHttpContextAccessor _httpContextAccessor;
protected override void Convert (TextWriter writer, LoggingEvent
loggingEvent) {
if (_httpContextAccessor == null) {
_httpContextAccessor =
Program.ServiceLocator.GetService<IHttpContextAccessor> ();
}
var httpContext = _httpContextAccessor.HttpContext;
if (httpContext == null) {
writer.Write (SystemInfo.NotAvailableText);
} else {
Convert (writer, loggingEvent, httpContext);
}
}
...
}
```
Note that this also violates SRP since the code also requires logic to
retrieve its own dependencies.
By allowing for dependency inversion, the same pattern layout converter can
now have dependencies injected, as the service locator logic is confined to an
implementation provided to log4net (as per this PR):
```cs
public class HttpContextPatternLayoutConverter : PatternLayoutConverter {
public HttpContextPatternLayoutConverter (IHttpContextAccessor
httpContextAccessor) {
_httpContextAccessor = httpContextAccessor;
}
protected override void Convert (TextWriter writer, LoggingEvent
loggingEvent) {
var httpContext = _httpContextAccessor.HttpContext;
if (httpContext == null) {
writer.Write (SystemInfo.NotAvailableText);
} else {
Convert (writer, loggingEvent, httpContext);
}
}
...
}
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
