Skip to content

Conversation

@AnthonyCogworks
Copy link
Contributor

Created an interface based on the LogHelper, and created a service, exposed through Application.Services. Made LogHelper obsolete.

http://issues.umbraco.org/issue/U4-5231

Assisted by @cloud46limited

…ted a service, exposed through Application.Services.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should simplify the ILoggingService interface. Anything that has a generic call in it can be an extension method. We should keep this interface as simple as possible.

There's quite a few methods in the LogHelper that are there just for legacy/backwards compatibility reasons, moving forward we shouldn't have them. Any method with the 'showHttpTrace' should not exist on this interface (we can deal with that internally)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Shazwazza showHttpTrace is still important for people still using webforms, where umbDebugShowTrace still works just fine.

@Shazwazza Shazwazza self-assigned this Nov 20, 2014
@Shazwazza
Copy link
Contributor

This will also need to be targeted at 7.2, but i can take care of that in the merge i think.

@AnthonyCogworks
Copy link
Contributor Author

I've removed the items from the interface. I've left the methods in the Service though, as I noticed that if you want to port code to use it, there are a few cases where you might need them.

@Shazwazza
Copy link
Contributor

Coming back to this PR, there's a few things that need to be adjusted... I'll do the following but myself when i pull this in but I'm just writing what needs to be done for completeness:

  • The internal generic methods on the service like: internal ILog LoggerFor() should be extension methods for ILoggingService (which should be renamed to just ILogger)
  • The interface and implementation should not exist as a service, they should exist in the System.Core.Logging namespace
  • The Logger should not be exposed on the ServiceContext, logging is a global thing not a data service. Any object should be able to log without any dependencies. If we were using IoC we'd just inject the ILogger into anything that required logging (which we will still be doing where possible like on all service ctor's, etc...). We are currently using a 'poor mans' IoC using resolvers so people have the ability to swap them on startup. Therefore the way this needs to work is by creating a LoggerResolver that inherits from SingleObjectResolverBase.
  • The LogHelper will then wrap the LoggerResolver.Current.Logger (which returns the ILogger)
  • On startup we create the LoggerResolver instance, developers can replace this with their own during ApplicationStarting
  • Anything that performs logging should in theory take an ILogger in a ctor argument - but really, anything that requires any dependency should take their dependency as a ctor argument so that it's IoC compatible ... developers should really stop trying to rely so much on singletons.

@Shazwazza
Copy link
Contributor

I've pulled in these changes and updated a ton which you can see in revs: 519b06f, 77e672a

There's plenty more work that needs to be done to migrate most things over to use ILogger, we'll slowly chip away at that.

@Shazwazza Shazwazza closed this Jan 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants