community-features icon indicating copy to clipboard operation
community-features copied to clipboard

Save failures return vague messages (exception swallowing)

Open gibbotronic opened this issue 6 years ago • 9 comments

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
  }
]
  1. 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.

gibbotronic avatar Sep 02 '19 16:09 gibbotronic

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

  • [ ] Summary of the issue
  • [ ] Information on your environment
  • [ ] Steps to reproduce
  • [ ] Expected and actual results

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?

  • [ ] yes
  • [ ] no

m2-assistant[bot] avatar Sep 02 '19 16:09 m2-assistant[bot]

@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.

gibbotronic avatar Sep 02 '19 17:09 gibbotronic

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:

  • [ ] 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).

    DetailsIf the issue has a valid description, the label Issue: Format is valid will be added to the issue automatically. Please, edit issue description if needed, until label Issue: Format is valid appears.

  • [ ] 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue. If the report is valid, add Issue: Clear Description label to the issue by yourself.

  • [ ] 3. Add Component: XXXXX label(s) to the ticket, indicating the components it may be related to.

  • [ ] 4. Verify that the issue is reproducible on 2.3-develop branch

    Details- Add the comment @magento give me 2.3-develop instance to deploy test instance on Magento infrastructure.
    - If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.x.
    - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!

  • [ ] 5. Add label Issue: Confirmed once verification is complete.

  • [ ] 6. Make sure that automatic system confirms that report has been added to the backlog.

m2-assistant[bot] avatar Sep 03 '19 07:09 m2-assistant[bot]

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!

engcom-Charlie avatar Sep 03 '19 08:09 engcom-Charlie

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?

gibbotronic avatar Sep 03 '19 08:09 gibbotronic

@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?

engcom-Charlie avatar Sep 03 '19 08:09 engcom-Charlie

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:

  • [ ] 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).

    DetailsIf the issue has a valid description, the label Issue: Format is valid will be added to the issue automatically. Please, edit issue description if needed, until label Issue: Format is valid appears.

  • [ ] 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue. If the report is valid, add Issue: Clear Description label to the issue by yourself.

  • [ ] 3. Add Component: XXXXX label(s) to the ticket, indicating the components it may be related to.

  • [ ] 4. Verify that the issue is reproducible on 2.3-develop branch

    Details- Add the comment @magento give me 2.3-develop instance to deploy test instance on Magento infrastructure.
    - If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.x.
    - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!

  • [ ] 5. Add label Issue: Confirmed once verification is complete.

  • [ ] 6. Make sure that automatic system confirms that report has been added to the backlog.

m2-assistant[bot] avatar Sep 03 '19 08:09 m2-assistant[bot]

@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.

gibbotronic avatar Sep 03 '19 09:09 gibbotronic

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

sdzhepa avatar Sep 03 '19 18:09 sdzhepa