magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.
http://www.magento.com
Open Software License 3.0
11.44k stars 9.29k forks source link

Stop catching exceptions and destroying them #30241

Open oliverde8 opened 3 years ago

oliverde8 commented 3 years ago

Summary (*)

Magento catches very often \Exception and then does nothing with it. No logs nothing.

This means when such an error happens everyone thinks all is well, but in reality, hell might be breaking loose.

Examples (*)

Gift Messages:

If in the checkout your gift can't be saved due an error (for example sender name to long); the interface updates, the user thinks all is well, there are no error logs but the gift message is ignored.

Cause: https://github.com/magento/magento2/blob/2.4.0/app/code/Magento/GiftMessage/Model/GiftMessageManager.php#L87

Why are the exceptions caught, in the example case the method calling the add function has a try-catch as well.

Proposed solution

  1. Don't try catch \Exception if possible
  2. When catching an \Exception at least log it
  3. When catching an exception and creating a new one, add it to it's parents.

How hard is it to handle an exception properly It's not done properly in so many places.

We are miserable since magento2 came out; hating our profession. Can't we get these small things fixed?


Please provide Severity assessment for the Issue as Reporter. This information will help during Confirmation and Issue triage processes.

Additional info

One way to reproduce the issue is to add a throw new CouldNotSaveException(__("Show custom error.")); code to the /app/code/Magento/GiftMessage/Model/GiftMessageManager.php file show_error and try to imitate an exception

Preconditions

Magento 2.4-develop Allow Gift Messages is enabled

Steps to reproduce

  1. Add a throw new CouldNotSaveException(__("Show custom error.")); code to the /app/code/Magento/GiftMessage/Model/GiftMessageManager.php file
  2. Add a Product to Cart
  3. Type long name with different characters into From and To fields under Gift Options
  4. Update

Expected Result

An exception message in logs

Actual Result

No exception message in logs

m2-assistant[bot] commented 3 years ago

Hi @oliverde8. 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.4-develop instance - upcoming 2.4.x release

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

Please, add a comment to assign the issue: @magento I am working on this


:clock10: You can find the schedule on the Magento Community Calendar page.

:telephone_receiver: The triage of issues happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

:movie_camera: You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

:pencil2: Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

ihor-sviziev commented 3 years ago

Hi @oliverde8, I do agree with you, it have to be at least logged somewhere.

This issue should be split to two things that basically quite related to each other:

1. Do not throw an \Exception

Unfortunately we still have such places in Magento2 code that need to be refactored, examples https://github.com/magento/magento2/blob/9544fb243d5848a497d4ea7b88e08609376ac39e/app/code/Magento/UrlRewrite/Helper/UrlRewrite.php#L38 https://github.com/magento/magento2/blob/89d3cda99975c0db2967165311184be84d26e9b4/lib/internal/Magento/Framework/Search/Request/Mapper.php#L116

In order to not introduce new such places we have a static test for that https://github.com/magento/magento-coding-standard/blob/1b65bd6062c7f1da40c70b64aa3970acd4563087/Magento2/Sniffs/Exceptions/DirectThrowSniff.php

2. Empty catch statements

The same situation with empty catch statements - unfortunately we have them and should clean them up, examples https://github.com/magento/magento2/blob/9544fb243d5848a497d4ea7b88e08609376ac39e/app/code/Magento/Integration/Model/OauthService.php#L152-L153 https://github.com/magento/magento2/blob/9544fb243d5848a497d4ea7b88e08609376ac39e/dev/tests/integration/testsuite/Magento/Catalog/_files/product_system_attribute_rollback.php#L32-L34

We even have the static test that fails in case of empty catch section, example https://github.com/magento/magento2/pull/25279#discussion_r495915313 image


Feel free to create PRs for fixing such places!

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 14 days if no further activity occurs. Is this issue still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? Thank you for your contributions!

ihor-sviziev commented 3 years ago

I think we should keep this issue open

engcom-Bravo commented 3 years ago

Hello @oliverde8

Thank you for your report.

We Confirm that this is a valid issue on the latest 2.4-develop. Additional info is added to the description.

magento-engcom-team commented 3 years ago

:white_check_mark: Confirmed by @engcom-Bravo Thank you for verifying the issue. Based on the provided information internal tickets MC-40757 were created

Issue Available: @engcom-Bravo, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

DiZeroX commented 3 years ago

@magento I am working on it

m2-assistant[bot] commented 3 years ago

Hi @DiZeroX! :wave: Thank you for collaboration. Only members of Community Contributors Team are allowed to be assigned to the issue. Please use @magento add to contributors team command to join Contributors team.

DiZeroX commented 3 years ago

@magento add to contributors team

m2-assistant[bot] commented 3 years ago

Hi @DiZeroX! :wave: Thank you for joining. Please accept team invitation :point_right: here :point_left: and add your comment one more time.

DiZeroX commented 3 years ago

@magento I am working on it

diazwatson commented 1 year ago

@ihor-sviziev is the static test you mentioned above (the one to test empty catch block) public? Can't find it inside Magento static tests (2.4.4) nor in the coding standard repo :(

diazwatson commented 1 year ago

Forget it I found it :)

lotcz commented 11 months ago

3 years later and issue still persists. Nothing works and nothing is ever logged! I hate you Magento2!