Notifications throws error if content saved during startup.

[KevinJump]

If a process attempts to save a content item during Umbraco startup, an error is returned from the Notifications process.

Value cannot be null. Parameter name: siteUri

This is coming from the Notify class in the notifications component
https://github.com/umbraco/Umbraco-CMS/blob/v8/dev/src/Umbraco.Web/Compose/NotificationsComponent.cs#L207

which triggers in this instance from ContentService.Saved which is fired once an item has been saved.

As this throws an exception it bubbles all the way through and is returned to the caller, implying the save failed, when in fact the content save will have worked.

So couple of questions, for what is hopefully a quick PR?

Should this function not just fail silently if there is _runtimeState.ApplicationUrl to send from?
or
is there some Core magic that can get a fallback URI from somewhere?

[nul800sebastiaan]

I'm pretty sure the "fallback" would be if you have a umbracoApplicationUrl set in umbracoSettings.config, can you try that out?

[KevinJump]

It still throws the error for me even with the umbracoApplicationUrl set :(

but even if that did work - should it throw the error at all?

The Notification Code is called after the content save has occurred; so the content is saved/published - but if you are the calling code you get an exception back which will indicate it didn't -

Also not sure what knock on this could have to other things listening for the events (like cache etc), because the content is saved/published, but all the events might not have run?

[nul800sebastiaan]

I'm not sure what it is for, I'll have to ask around!

Sorry for not asking earlier, but what do you mean with "during startup"? Maybe you're saving at the wrong time?

[KevinJump]

ultimately the save is happening from calls originating in the Initialize call of a Component (registered via an IUserComposer so after the core is up ?)

[Shazwazza]

There's an easy fix and a hard fix.

Sure, we could just exist if siteUrl is null and not send any notifications. If you are doing this on umbraco startup, it means there is no site url registered yet because that occurs at the very beginning of a request and during umbraco startup, no request has been created yet, this is happening before even aspnet is initialized.

But if that happens, it means that anyone subscribe to receive notifications will not get them - maybe this is ok if it's being done on startup and for a reason where no notifications are needed? But in cases where that is not ok, then a more difficult fix is needed.... which would require a lot of new plumbing and queing notifications on BackgroundTaskRunner that only initializes after umbraco is fully initialized and then changing how the NotificationService acquires the siteUrl so that it's lazily acquired only when the messages are sent.

Let me know what you think. If notifications need to be sent because services are being used on startup then I'll need to respond with some details as it will take a bit of effort to do that.

[KevinJump]

I am not sure. personally I would accept that content changed at startup doesn't get a notification, but that is a personal preference ☹️ .

the main issue is that the content save call has worked, and the triggered notification process is returning the exception which then goes to the calling process, so there is no way to differentiate between an issue with the save and subsequent triggered errors? maybe thats where the change should be?

But again I can see that some of the things that are triggered might be considered critical (like caching?)

[Shazwazza]

Cache expiration will continue to work, the main issue here is that the application url isn't configured until a request begins which is after the boot process finishes. The other issue is that we do not allow lazily sending notifications.

For now - there's no reason to keep throwing exceptions when the URL is null. Instead, we can add a warning log indicating that the URL was not set (probably because it's during the boot process) and that notifications will not be sent.

Then we should create another (larger) task to fix this correctly and have the notifications be sent in a lazy queue.

[emmaburstow]

Fixed in #6868 :)