context.Cancel for "Removing" content handler

[orchardbot]

ivyexcellence created: https://orchard.codeplex.com/workitem/21210

We have context.Cancel available for Publishing content handler which is very nice feature. Once can hook the publishing handler and can cancel the context to stop the process immediately.

I'm working on a requirement where context canceling is required on "Removing" event. Please can we include this in upcoming release of Orchard 1.9

It is very trivial and i can also look into it as well. Kindly accommodate this request as this small change will save me from so many hours of coding and writing workarounds.

Many Thanks

[orchardbot]

@Piedone commented:

Agree.

[Xceno]

I'd provide a PR for this but we first need to think about this in a bit more detail. It seems pretty easy to implement on the first look, but we can't just return here after calling all Removing handlers

The removing handlers itself might run some cleanup code and in turn remove additional data, etc. In fact, I think that the current way of cancelling the Publish event could lead to similar issues.

To solve this. I propose to establish some new OnBeforeX-Handlers, that get called at the very beginning of the existing methods (Publish, Unpublish, Remove, Destroy). So the current Remove method should look like this:

public virtual void Remove(ContentItem contentItem) {
    var context = new RemoveContentContext(contentItem);

    // This here is what I'm talking about.
    Handlers.Invoke(handler => handler.BeforeRemoving(context), Logger);
    if(context.Cancel) {
        return;
    }

    Handlers.Invoke(handler => handler.Removing(context), Logger);

    var activeVersions = _contentItemVersionRepository.Fetch(x => x.ContentItemRecord == contentItem.Record && (x.Published || x.Latest));
    foreach (var version in activeVersions) {
        if (version.Published) {
            version.Published = false;
        }
        if (version.Latest) {
            version.Latest = false;
        }
    }

    Handlers.Invoke(handler => handler.Removed(context), Logger);
}

Any thoughts?

[Piedone]

I think the point of the ing events is exactly to do something before the actual removal happens, thus if you want to free up some external resource for example that should happen in the ed events.

So in principle I see no issue. But how is it in practice, did you see this being used otherwise?

[Xceno]

I agree on how they should be used, but yes I've seen it used otherwise and also used it otherwise myself.

For example, if you have DB constraints that force you actually remove another item, clear a column, etc. before the current one can be removed.

I've also thought about calling TransactionManager.Cancel() in the Removing handler but I'm not sure of its side effects.

[Piedone]

Ah, that's sad... Not sure what to do then, a set of new events doesn't seem appealing to me (that just pushes the problem to another set of events which people will eventually begin to misuse).

Using TransactionManager.Cancel() could indeed have unwanted side effects. Generally it's dangerous to call it anywhere else than on a top level (say, controller) and even there it's risky (consider that some code removed a file somewhere, but a corresponding DB operation is now rolled back; the site's state is now inconsistent).

[Xceno]

So I've discussed this further with @sebastienros in gitter and will just leave his quote here, so it's all in one place.

I would not be opposed to another events interface that would only be for content manager to be called before the actual operation. For instance

BeforePublish
BeforeRemove

With a method Cancel() and this would be used as the first operation in each method. Hence Publishing and Published events would not even be called. And you could still display an alert from your code.

@Piedone I understand, that yet another set of handlers seems overkill; yet with a dedicated Cancel() method it should be clear what the purpose of them is. I also think we all agree that TransactionManager.Cancel() ain't the way to go.

I'll try to implement those handlers in my current project, see how it goes and keep you guys updated. Any further thoughts are highly appreciated though!

[sebastienros]

Not really decided whether it should be in the IContentHandler or a specific events class for the ContentManager