search

magento / community-features

Magento Features Development is an Initiative to Allows Community Memebers Join to Development of Magento Features
46 stars 18 forks source link

Save failures return vague messages (exception swallowing) #194

Open gibbotronic opened 4 years ago

gibbotronic commented 4 years ago

Save failure exceptions are swallowed/hidden, leaving the poor end-user completely baffled as to why something is failing. The helpful exception message should be shown so that we know how to resolve it.

Preconditions (*)

  1. Magento 2.3.2
  2. A configurable product with options assigned (therefore requiring a child product too)

Steps to reproduce (*)

  1. Discover the options related to your product already, eg. a GET from http://127.0.0.1/rest/V1/configurable-products/14029/options/all returns

    [
{
"id": 10764,
"attribute_id": "310",
"label": "Sign Size",
"position": 0,
"values": [
  {
    "value_index": 2864
  },
  {
    "value_index": 2868
  },
  {
    "value_index": 2871
  },
  {
    "value_index": 2875
  },
  {
    "value_index": 2878
  },
  {
    "value_index": 2884
  }
],
"product_id": 7701
}
]

  2. POST to this product's options with the same data, eg. a POST to with

    {
"option": {
"attribute_id": 310,
"label": "sign_size",
"position": 0,
"is_use_default": true,
"values": [{
  "value_index": 2864
}, {
  "value_index": 2868
}, {
  "value_index": 2871
}, {
  "value_index": 2875
}, {
  "value_index": 2878
}, {
  "value_index": 2884
}],
"extensionAttributes": {},
"productId": 0
}
}

This returns a HTTP 400 with the data:

{
  "message": "An error occurred while saving the option. Please try to save again.",
  "trace": "#0 [internal function]: Magento\\ConfigurableProduct\\Model\\OptionRepository->save('14029', Object(Magento\\ConfigurableProduct\\Model\\Product\\Type\\Configurable\\Attribute))\n#1 /usr/local/var/www/app/code/Magento/Webapi/Controller/Rest/SynchronousRequestProcessor.php(95): call_user_func_array(Array, Array)\n#2 /usr/local/var/www/app/code/Magento/Webapi/Controller/Rest.php(188): Magento\\Webapi\\Controller\\Rest\\SynchronousRequestProcessor->process(Object(Magento\\Framework\\Webapi\\Rest\\Request\\Proxy))\n#3 /usr/local/var/www/lib/internal/Magento/Framework/Interception/Interceptor.php(58): Magento\\Webapi\\Controller\\Rest->dispatch(Object(Magento\\Framework\\App\\Request\\Http))\n#4 /usr/local/var/www/lib/internal/Magento/Framework/Interception/Interceptor.php(138): Magento\\Webapi\\Controller\\Rest\\Interceptor->___callParent('dispatch', Array)\n#5 /usr/local/var/www/lib/internal/Magento/Framework/Interception/Interceptor.php(153): Magento\\Webapi\\Controller\\Rest\\Interceptor->Magento\\Framework\\Interception\\{closure}(Object(Magento\\Framework\\App\\Request\\Http))\n#6 /usr/local/var/www/generated/code/Magento/Webapi/Controller/Rest/Interceptor.php(26): Magento\\Webapi\\Controller\\Rest\\Interceptor->___callPlugins('dispatch', Array, Array)\n#7 /usr/local/var/www/lib/internal/Magento/Framework/App/Http.php(137): Magento\\Webapi\\Controller\\Rest\\Interceptor->dispatch(Object(Magento\\Framework\\App\\Request\\Http))\n#8 /usr/local/var/www/generated/code/Magento/Framework/App/Http/Interceptor.php(24): Magento\\Framework\\App\\Http->launch()\n#9 /usr/local/var/www/lib/internal/Magento/Framework/App/Bootstrap.php(261): Magento\\Framework\\App\\Http\\Interceptor->launch()\n#10 /usr/local/var/www/index.php(39): Magento\\Framework\\App\\Bootstrap->run(Object(Magento\\Framework\\App\\Http\\Interceptor))\n#11 {main}"
}

As you can see this does not tell you what is wrong. It simply states that "An error occurred while saving the option.".

Tracing this through, the underlying file at lib/internal/Magento/Framework/Model/ResourceModel/Db/AbstractDb.php save tries to save the object but catches a DuplicateException exception ("Unique constraint violation found"). This is caught in app/code/Magento/ConfigurableProduct/Model/OptionRepository.php in the function

public function save($sku, OptionInterface $option)

which has the very message we need to know ('unique constraint violation') in the exception that is has caught:

try {
    $option->save();
} catch (\Exception $e) {
    throw new CouldNotSaveException(__('An error occurred while saving the option. Please try to save again.'));
}

Instead of passing $e's message out, it just returns the very unhelpful message that it has failed. It should pass the appropriate exception message out so that the poor user (me) can work out what is going on without having to trace through all of this code just to find out what the problem is.

This problem occurs in other places across the codebase as a very casual grep -R "throw new CouldNotSaveException" . shows, eg.

./app/code/Magento/Quote/Model/QuoteManagement.php:            throw new CouldNotSaveException(__("The quote can't be created."));
./app/code/Magento/Quote/Model/Quote/Item/CartItemPersister.php:            throw new CouldNotSaveException(__("The quote couldn't be saved."));
./app/code/Magento/Quote/Model/Quote/Item/Repository.php:            throw new CouldNotSaveException(__("The item couldn't be removed from the quote."));
./app/code/Magento/Catalog/Model/Product/TierPriceManagement.php:            throw new CouldNotSaveException(__("The group price couldn't be saved."));
./app/code/Magento/Catalog/Model/Product/Option/Repository.php:            throw new CouldNotSaveException(__("The custom option couldn't be removed."));
./app/code/Magento/Catalog/Model/Product/PriceModifier.php:            throw new CouldNotSaveException(__('The tier_price data is invalid. Verify the data and try again.'));
./app/code/Magento/Catalog/Model/ProductLink/Management.php:            throw new CouldNotSaveException(__('The linked products data is invalid. Verify the data and try again.'));
./app/code/Magento/Catalog/Model/ProductLink/Repository.php:            throw new CouldNotSaveException(__('The linked products data is invalid. Verify the data and try again.'));
./app/code/Magento/Catalog/Model/ProductLink/Repository.php:            throw new CouldNotSaveException(__('The linked products data is invalid. Verify the data and try again.'));
./app/code/Magento/Catalog/Model/ResourceModel/Product/Link/SaveHandler.php:            throw new CouldNotSaveException(__('The linked products data is invalid. Verify the data and try again.'));
./app/code/Magento/Bundle/Model/Option/SaveAction.php:            throw new CouldNotSaveException(__("The option couldn't be saved."), $e);
./app/code/Magento/CatalogImportExport/Model/StockItemImporter.php:            throw new CouldNotSaveException(__('Invalid Stock data for insert'), $e);
./app/code/Magento/GiftMessage/Model/CartRepository.php:            throw new CouldNotSaveException(__("The gift message isn't available."));
./app/code/Magento/GiftMessage/Model/OrderItemRepository.php:            throw new CouldNotSaveException(__("The gift message isn't available."));
./app/code/Magento/GiftMessage/Model/GiftMessageManager.php:            throw new CouldNotSaveException(__("The gift message couldn't be added to Cart."));
./app/code/Magento/GiftMessage/Model/ItemRepository.php:            throw new CouldNotSaveException(__("The gift message isn't available."));
./app/code/Magento/GiftMessage/Model/OrderRepository.php:            throw new CouldNotSaveException(__("The gift message isn't available."));
./app/code/Magento/ConfigurableProduct/Model/OptionRepository.php:            throw new CouldNotSaveException(__('An error occurred while saving the option. Please try to save again.'));

Expected result (*)

  1. The underlying exception's message should be passed out

Actual result (*)

  1. A vague message with no clue as to the real explicit reason for failure. The real reason is kept a secret.
m2-assistant[bot] commented 4 years ago

Hi @gibbotronic. Thank you for your report. To help us process this issue please make sure that you provided the following information:

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento give me 2.3-develop instance - upcoming 2.3.x release

For more details, please, review the Magento Contributor Assistant documentation.

@gibbotronic do you confirm that you were able to reproduce the issue on vanilla Magento instance following steps to reproduce?

gibbotronic commented 4 years ago

@gibbotronic do you confirm that you were able to reproduce the issue on vanilla Magento instance following steps to reproduce?

* [x]  yes

* [ ]  no

Yes this occurs on vanilla 2.3.2, honestly.

m2-assistant[bot] commented 4 years ago

Hi @engcom-Charlie. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

engcom-Charlie commented 4 years ago

Hello @gibbotronic I can't reproduce this issue on fresh Magento 2.3-develop. Manual testing scenario:

  1. Create configurable product with options assigned and with child item
  2. Discover the options related to your product already, eg. a GET from http://magento2.loc/rest/V1/configurable-products/testconf2/options/all rerurns: image
  3. POST to this product's options with the same data, eg. a POST from http://magento2.loc/rest/V1/configurable-products/testconf2/options/ returns: image So i have to close this issue. Thanks for your report!
gibbotronic commented 4 years ago

Please do not close this bug report!

You can clearly see from the code I mentioned in the report (app/code/Magento/ConfigurableProduct/Model/OptionRepository.php) that the exception handler is swallowing the exception thrown lower down. You can also clearly see this in the list of files I have included where this also happens. Respectfully, did you not look at the file I referenced??

This report is to highlight that the exception is being swallowed and no helpful message is being returned. Do you want me to provide a series of requests so that you can repeat this?

By closing this, are you saying that: a. you want unhelpful messages to be output when failures occur? b. you are are happy that Magento does not reveal the real cause of a failure?

engcom-Charlie commented 4 years ago

@gibbotronic it looks like a feature request. So i think we need to transfer this issue to feature requests repository. @sdzhepa can you help with it?

m2-assistant[bot] commented 4 years ago

Hi @sdzhepa. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

gibbotronic commented 4 years ago

@gibbotronic it looks like a feature request. So i think we need to transfer this issue to feature requests repository.

@engcom-Charlie ok thanks. If this can get resolved somehow I would appreciate it because I think the messages should be more helpful and indicative of the underlying problem instead of the vague "it didn't work" messages. Someone else had this problem (https://magento.stackexchange.com/q/267855) but it is not clear if this is on 2.3.2 or 2.3.1.

I will try to write a series of curl requests at some point so this can be duplicated.

This issue affects a number of places as indicated in my original report. Thanks.

sdzhepa commented 4 years ago

Based on the description and label feature request the issue has been transferred to Magento Feature Request public repo.