Throw exception when editing an order and the old order could not be cancelled.

[woutersamaey]

Throw exception when editing an order and the old order could not be cancelled.

This is to avoid having 2 open orders.

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)

[joshua-bn]

How/why did this happen for you?

[woutersamaey]

I have some custom code that alters the result of $order->canCancel() to false (using an event, 100% clean code)

When you edit an order, the original order is canceled and a new is created.
Magento does not check if the original order is in fact canceled or not. It just continues with the creation of the new order.
The original order remains as it was and you end up with 2 open orders (breaking reports, confusing customers, etc). This is a mistake and we should break the order edit process with an exception.

The original order MUST be canceled.

[colinmollenhour]

Seems like MSMOrder->cancel() should throw an exception if it can't be canceled?

[woutersamaey]

Yes, absolutely! But will everyone support this change?
It's been like this since forever in Magento...

[colinmollenhour]

Yes, absolutely! But will everyone support this change?
It's been like this since forever in Magento...

You're right, probably not worth a BC break.

[woutersamaey]

@colinmollenhour Just to be clear, I'm all for throwing the exception if $order->cancel() fails. But can't decide this on my own.

It's very strange Magento does not throw an exception and just leaves things as is.
Imagine having to write extra code just to double check everything after doing something. It's insane.

Also: Should we leave bad code in? Just because some developer did a shitty job many many years ago? I think not.

I actually had a client run into this issue and had to explain them the situation. They could not believe the stupidity of Magento...

If @Flyingmana and @sreichel can agree, I'd be happy to move the exception to the cancel() method. If BC breaks, I'm assuming that module would be shit too.

All depends on your aim: You want a better program or you want BC (even when it is shitty code)?

[colinmollenhour]

Ok, let me rephrase.. Probably not worth putting your fix on hold. So I think we can accept this as-is, and address the issue of cancel() not throwing an exception as a separate issue in a separate version. Although we really need to finalize the way we manage branches with BC changes in order to do that...

[woutersamaey]

Sounds good. Let’s get this merged.