We need to wrap the commerce_line_item_save() functions in try catch block, because the entity save can throws errors but the code doesn't handle it correctly.

In file commerce_line_item.module: 

....
            // Save it and add it to the line item reference field's values array.
            commerce_line_item_save($line_item);

            // If the item is saved, we set a variable to notify the user the
            // need of saving the order.
            $form_state['line_item_save_warning'] = TRUE;

            $value[] = array('line_item_id' => $line_item->line_item_id);
.....
CommentFileSizeAuthor
#7 commerce-handling_the_try_catch_block_of_commerce-2578415-7.patch1.86 KBskek
#6 commerce-handling_the_try_catch_block_of_commerce-2578415-6.patch1.87 KBskek
#3 commerce-handling_the_try_catch_block_of_commerce-2578415-2.patch1.39 KBskek

Comments

skek created an issue. See original summary.

skek’s picture

skek commented
Title: Add product button on order create/update page doesn't handle the exceptions correctly. » The "Add product" button on order create/update page doesn't handle the exceptions correctly.
skek’s picture

skek commented
StatusFileSize
new commerce-handling_the_try_catch_block_of_commerce-2578415-2.patch1.39 KB
skek’s picture

skek commented
Status: Active » Needs review
rszrama’s picture

rszrama commented
Issue tags: +sprint
skek’s picture

skek commented
StatusFileSize
new commerce-handling_the_try_catch_block_of_commerce-2578415-6.patch1.87 KB

Adding additional try catch block for the commerce_line_item_save() function calls.

skek’s picture

skek commented
StatusFileSize
new commerce-handling_the_try_catch_block_of_commerce-2578415-7.patch1.86 KB

Not passing the quantity field name when we have exception, just leave the field name empty in order to prevent form submission when we have errors.

rszrama’s picture

rszrama commented
Category: Bug report » Task

Hey skek, I don't suppose you can offer up a scenario that would generate the error? Is there some module that's causing problems here that surfaced the issue for you?

skek’s picture

skek commented

@rszrama,

The issue with try/catch is actually more general and it is connected with entity system.
For example imagine you are getting an PDO exception or an exception from a module that hook on entity operations.
When we do commerce_line_item_save() call, this means that this function can return exception in any reason and we should handle this exception in the place we are calling the function.
If you see for example the CommerceLineItemEntityController::save() method, you will see that the exception is thrown after the rollback.
This means that we will get fatal error screen if for any reason we get an exception, no matter of the source of that exception.
This patch is actually more like UX patch, avoiding WSOD or ugly messages.
The catch() cause in this patch is something that can be modified for example we can set a message notifying customer about an exception has been thrown but I think we should have such try/catch block in order to have better UX.
Also in my understanding, if I call a function, that can throw an exception I always handle the exception and notifying the user that something goes wrong.
I have posted similar issues like this one so it is the same explanation for them.

Regards,
Nikolay