improve Magento\Catalog\Model\ImageUploader error handler

[fsw]

Description (*)

Magento\Catalog\Model\ImageUploader is catching detailed exceptions and turning it into generic "Something went wrong while saving the file(s)." Data on what actually went wrong is gone.
This fix logs exception and passes previous exception to constructor not to loose this info.

Also I would like to discuss if there are some guidelines on handling such cases in Magento 2.
In my humble opinion in development mode we show detailed info and in production mode we show "something went wrong" and log exception so catching exception just to show "Something went wrong while saving the file" in both modes and loose debug info does not make much sense and I would remove both this try/catches at all.

Manual testing scenarios (*)

1. change media dir permissions to not writable
2. upload a file in admin panel
3. see info on what folder lacks permissions in error log.

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @fsw. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

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

[lenaorobei]

I agree with adding logging, but for security reasons we dont want to expose additional info.

[fsw]

I agree with adding logging, but for security reasons we dont want to expose additional info.

Yes but additional info is hidden anyway in production mode (witch is indeed great for security). I think showing stack in developer mode and logging it in production and showing generic message is a good logic and can't see why we should treat some cases differently.

[lenaorobei]

LocalizedException is designed to be shown to the end user and it is not recommended to add any additional info other then allowed to be displayed. Logging should be good enough in this case.

[fsw]

@lenaorobei , way of handling exceptions is of course disputable hence I was wondering is there any official Magento guidance for it.
Just to be clear, I was not opting to add any additional info to LocalizedException but to simply remove try/catches like this.

Because without this specific try/catch:

• in development mode we show full details
• in production we show "something went wrong" witch is also translatable

And with it:

• in development we show "something went wrong saving the file" witch is not very helpful
• in production we change "something went wrong" to "something went wrong saving the file" witch is also not very helpful
• we have 5 LOC more increasing complexity without any profit.

hence I think first option is better.

[lenaorobei]

QA notes: please make sure that extended message is shown in developer mode only.

[magento-engcom-team]

Hi @lenaorobei, thank you for the review.
ENGCOM-7067 has been created to process this Pull Request
✳️ @lenaorobei, could you please add one of the following labels to the Pull Request?

Label	Description
Auto-Tests: Covered	All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered	Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required	Changes in Pull Request does not require coverage by auto-tests

[magento-engcom-team]

@fsw thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

[engcom-Alfa]

✔️ QA Passed

[engcom-Golf]

Failed functional tests not related to the changes in this PR

[m2-assistant]

Hi @fsw, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.