search

rasmusjp / umbraco-multi-url-picker

Multi Url Picker for Umbraco 7
MIT License
31 stars 29 forks source link

#42: Log original request with 'deleted node' warning #55

Closed PhilDye closed 6 years ago

PhilDye commented 7 years ago

"Fix" for #42. Unfortunately there isn't any more info available about which property is causing the warning.

I have a similar fix for 1.3.2, but would need a branch to PR it to.

PhilDye commented 7 years ago

v1.3 fix at https://github.com/EtchUK/umbraco-multi-url-picker/commit/93caf8f8f5ef2e6e75ce949114bc9dab61f974a3

ronaldbarendse commented 6 years ago

The current code contains a breaking change, because the previous public constructor public MultiUrls(string propertyData) is removed. This could easily be fixed by making the requestingUrl optional: public MultiUrls(string propertyData, string requestingUrl = null).

It could also throw a NullReferenceException if PublishedContentRequest or even the Uri is null. This could be fixed by using the null-conditional operator: UmbracoContext.Current.PublishedContentRequest?.Uri?.ToString().

However: with this implementation, the URL must be retrieved and converted to a string on every initialization/call, even without deleted nodes! So why not only do this when the warning needs to be logged instead?

rasmusjp commented 6 years ago

Thanks for the PR and sorry for the late reply.

As mentioned by @ronaldbarendse it's a breaking change since someone might use the constructor.

I would suggest adding %aspnet-request{HTTP_HOST}%aspnet-request{PATH_INFO} to the conversionPattern in the log4net.config, it will log the request url (if available) to every lentry in the log files.

e.g:

<layout type="log4net.Layout.PatternLayout">
  <conversionPattern value=" %date [P%property{processId}/D%property{appDomainId}/T%thread] %-5level %logger (%aspnet-request{HTTP_HOST}%aspnet-request{PATH_INFO}) - %message%newline" />
</layout>