U4-10892 - GridValueConverter requires HttpContext

Created by Daniël Knippers 24 Jan 2018, 16:08:39 Updated by Daniël Knippers 13 Jun 2018, 11:29:23

Tags: PR

I noticed the GridValueConverter relies on HttpContext.Current being set, but only to set an "IsDebug" flag on IGridConfig. This raises a NullReferenceException whenever a Grid property is read in contexts without an HttpContext, like a background thread, resulting in the Grid property value being null. Our specific case is sending scheduled e-mails from a background thread, where the e-mail content is created using the Grid. We can obtain the IPublishedContent just fine, but the Grid property is NULL due to the GridValueConverter crashing in the ConvertDataToSource method. Other property values are present without issues.

Could this be changed to not crash when HttpContext.Current is NULL? Below is the source, where I added a comment above the line that causes the crash. // Umbraco.Core.PropertyEditors.ValueConverters.GridValueConverter.cs#L43 var gridConfig = UmbracoConfig.For.GridConfig( ApplicationContext.Current.ProfilingLogger.Logger, ApplicationContext.Current.ApplicationCache.RuntimeCache, new DirectoryInfo(IOHelper.MapPath(SystemDirectories.AppPlugins)), new DirectoryInfo(IOHelper.MapPath(SystemDirectories.Config)),

// Cause of crash. 
HttpContext.Current.IsDebuggingEnabled);

Changing this line to HttpContext.Current?.IsDebuggingEnabled == true would be enough to remedy the NullReferenceException (and of course, causing IsDebug to be set to false whenever there is no HttpContext).

-- Daniƫl Knippers

1 Attachments

Comments

Sebastiaan Janssen 24 Jan 2018, 16:19:31

Sure, send us a PR?

HttpContext.Current?.IsDebuggingEnabled == true will need to be the verbose version, something like HttpContext.Current == null || HttpContext.Current.IsDebuggingEnabled I think is what you're trying to achieve? We don't support C# 6 yet.


Daniël Knippers 24 Jan 2018, 16:29:15

@sebastiaan Ah I see, however the equivalent longer version would be HttpContext.Current != null && HttpContext.Current.IsDebuggingEnabled (otherwise it would set IsDebug to true in Production environments when HttpContext is null). Will likely submit a PR this week and link it here. Thanks for the quick reply!


Sebastiaan Janssen 24 Jan 2018, 16:34:46

Sure thing! I didn't really think it through, will do for the review though ;)


Daniël Knippers 25 Jan 2018, 08:01:01

@sebastiaan PR here: https://github.com/umbraco/Umbraco-CMS/pull/2420


Daniël Knippers 12 Jun 2018, 07:40:44

@sebastiaan Was this never merged in? The [current version of dev-v7|https://github.com/umbraco/Umbraco-CMS/blob/dev-v7/src/Umbraco.Core/PropertyEditors/ValueConverters/GridValueConverter.cs#L48] still has the old code and will crash without an HttpContext.


Sebastiaan Janssen 13 Jun 2018, 11:25:56

That is super weird! I can see it merged and being part of 7.7.10+.. See screenshot.

But it's not in the current file and the current file history says... neh, that commit was never there.

I've cherry-picked it now and it will be part of 7.11.0. I really cannot figure out what went wrong here, very strange.

Reference to the commit, which is definitely in dev-v7 this time: https://github.com/umbraco/Umbraco-CMS/commit/3520f2d1e4138e5da5350fafb5897d3fac2f0641


Daniël Knippers 13 Jun 2018, 11:29:23

@sebastiaan Weird, but thanks for including it again regardless :)!


Priority: Normal

Type: Exception

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted: Pull request

Affected versions:

Due in version: 7.11.0

Sprint:

Story Points:

Cycle: