U4-6843 - Error loading backoffice in v7.2.7 with Courier installed

Created by Mihail Trifonov 15 Jul 2015, 19:28:10 Updated by Shannon Deminick 16 Jul 2015, 10:56:32

Relates to: U4-6346

The backkoffice login screen doesn't load at all and just shows a blank page with Courier installed. I've tracked the cause of this issue to the fix to this other issue: U4-6346 More precisely in this pull request https://github.com/umbraco/Umbraco-CMS/pull/722 in umbraco.BusinessLogic.Actions.Action class, this method has been changed

        public static List<string> GetJavaScriptFileReferences()
        	return ActionsResolver.Current.Actions
				.Where(x => !string.IsNullOrWhiteSpace(x.JsSource))
{color:red}-               .Select(x => x.JsSource).ToList();{color}
{color:green}+               .Select(x => IOHelper.ResolveUrl(x.JsSource)).ToList();{color}

Now, Courier has a couple of IAction implementations that instead of returning a script url refernece within the JsSource property, actually return a whole script block. You can see how this would fail with a bang when passed to IOHelper.ResolveUrl and causes the whole BackOfficeController to crash.

PS. Umbraco HQ, if you guys decide that this is worth a new release (7.2.8?), and please, please do, can you also backport the fix to U4-6687 (https://github.com/umbraco/Umbraco-CMS/pull/707) as it is causing major data corruption to our site and I think should be considered critical as well. What made it even worse for us with 7.2.6 was issue U4-5052, that caused content to be saved with every preview, hence corrupting almost every page that's previewed. In fact, right now I'm forced to run a custom build of 7.2.7 with this fix included.


Shannon Deminick 15 Jul 2015, 19:33:49

Thanks for the details, we've actually finished discussing this exact issue and will look to release a 7.2.8, back porting that issue sounds like a good idea too.

Tom Fulton 15 Jul 2015, 19:42:58

Sorry guys :(. Let me know if I can help...sounds like you have it figured out.

Shannon Deminick 15 Jul 2015, 19:57:36

It's my fault, I forgot that there's legacy code that injects while script blocks. Just goes to show that tiny changes can have a big affect. A dirty hack will be required for this which is to determine if the JsSource is a script block or a path, most likely achieved by what the string starts with. Tom if you've got time now-ish, would happily accept a pr for this fix. Otherwise I can update it tomorrow morn (night time here now). Unit tests also need to be written for this scenario unfortunately since we still need to support legacy until v8

Paul Sterling 15 Jul 2015, 21:20:20

Can confirm that reverting to the prior code as @mtrifonov has done resolves the issue for Courier. Unfortunately, not sure what else depends on this. For Courier users we recommend NOT updating to 7.2.7 until we have addressed this.

Mihail Trifonov 15 Jul 2015, 21:58:52

Btw, I don't know it this helps, but the way I've fixed this locally for the moment is by reverting that one line in the Action class and moving the call to IOHelper.ResolveUrl to the last moment possible in the BackOfficeController.Application() method. That way the old logic in GetLegacyActionJs() does its magic to separate script blocks from script references and only in the Application() method where I'm sure I'm dealing with urls I use the helper to replace the '~/' prefix.

I didn't actually revert the rest of the changes Tom did, specifically the two legacy paths in umbraco.presentation, so I'm hoping his fix should still work, although I haven't tested it and I don't need it in my scenario.

Tom Fulton 15 Jul 2015, 23:18:40

@mtrifonov Thanks for the tip - I went ahead and did that here: https://github.com/umbraco/Umbraco-CMS/pull/742 // @Shandem

So far this looks to fix Courier and still maintains the fix for the RelationType actions.

I looked into adding some tests for this but I have a feeling it might take some effort...did you have any idea of how/where to do this Shannon?

Tom Fulton 16 Jul 2015, 02:50:04

@Shandem I took a quick stab at adding some tests [in a separate branch|https://github.com/tomfulton/Umbraco-CMS/commit/a1f40a3e6ec328316e07d403d2fca7cdd5e879ff]. Not sure if they are what you had in mind or if this is the right approach. It doesn't test this specific issue but at least adds some tests around the JS code/url separation. Not sure if it's kosher to change the method visibilities like that either.

Also these tests pointed out an issue, I added a commit to the PR to test for ~/ before using IOHelper.ResolveUrl()

Shannon Deminick 16 Jul 2015, 07:46:59

I've created a PR with your tests... easier to create a PR and review than to review code in a diff branch. It all looks fine, I might tweak some things, etc... but overall it's good

Priority: Critical

Type: Bug

State: Fixed


Difficulty: Easy


Backwards Compatible: True

Fix Submitted: Pull request

Affected versions: 7.2.7

Due in version: 7.2.8


Story Points: