Fix a PHP error being thrown on a Magento error being thrown

[mariusjp]

Description (*)

I ran into a small problem with the product media (because my pub/media wasn't writable). But I did not get the actual error that is thrown, I got a PHP error because 'directory' was not set (as you can see by my fix).
I thought, why not have my first contribution be a simple fix.

PHP: 7.4.15
Magento: 2.4.2

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

1. Have a simple magento shop with X (might as well be 1) product including images
2. Change the pub/media to incorrect permissions/owners
3. Try to visit the product detail page in the backend

Screenshot of the PHP Notice in stead of the actual error being thrown (also in the exception.log)

Questions or comments

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)
• README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
• All automated tests passed successfully (all builds are green)

Resolved issues:

1. resolves [Issue] Fix a PHP error being thrown on a Magento error being thrown #32819: Fix a PHP error being thrown on a Magento error being thrown

[m2-assistant]

Hi @MJTheOne. Thank you for your contribution
Here are 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

❗ Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s)
  For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE,
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests
13. Semantic Version Checker

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

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

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests 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.

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

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

[mariusjp]

@magento run all tests

[mrtuvn]

Not sure why we need change the pub/media to incorrect permissions/owners for reproduce
What if we set correct permissions at first
Just curiosity

[mariusjp]

Not sure why we need change the pub/media to incorrect permissions/owners for reproduce
What if we set correct permissions at first
Just curiosity

After a deployment I somehow ended up with incorrect permissions, and got the error "Undefined index 'directory'" in stead of the actual error "Unable to save.."

To be fair, this is probably not a situation that will happen often. Doesn't take away the fact the code is incorrect in assuming 'directory' is set as index on $file.

[mrtuvn]

Can you add screenshot show issue errors ? Or video records if possible. What php version and magento version used
That will improve for description make issue clear for QA can reproduce

[gabrieldagama]

@magento create issue

[mrtuvn]

@magento run WebAPI Tests, Functional Tests B2B

[mariusjp]

[image removed]

PHP: 7.4.15
Magento: 2.4.2

Well, I'll be damned. I am getting the error again (by setting the permissions incorrectly to be able to create a screenshot), but I'm not getting the "Undefined index 'directory'.." anymore..

[mrtuvn]

Can you update infor in ticket description

[mariusjp]

Can you update infor in ticket description

Done!

[mrtuvn]

@magento run Functional Tests B2B

[ihor-sviziev]

@mrtuvn I believe that covering this case with a test would be quite easy, so it's better to cover it in order not to have any regression here in the future.

@MJTheOne, will you be able to cover your change with a simple unit test?

[mrtuvn]

Agreed! Let's cover its with automated test

[mariusjp]

I'll get on it asap!

[mariusjp]

I'm sorry it's taking me a while. Having some trouble with how this is set up. The tests want to run "setup:install", which I find weird. Also been a bit busy. Will try to update this week!

[mariusjp]

Unit test added! Took me a while to figure out how this is set up. It's not like I have no UnitTest experience, just overlooked a few things so it was quit confusing at the start.

[mariusjp]

On it!

[ihor-sviziev]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[ihor-sviziev]

@magento run Unit Tests, Integration Tests, Functional Tests EE, Functional Tests B2B

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[Den4ik]

Failed Unit test "ui/js/grid/columns/multiselect.Default state - Select no rows" is not related to changes

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[magento-engcom-team]

Hi @ihor-sviziev, thank you for the review.
ENGCOM-9144 has been created to process this Pull Request

[ihor-sviziev]

@magento run Unit Tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[engcom-Hotel]

Hello @MJTheOne,
Thanks for the contribution!

I have tried to reproduce the issue in Magento version 2.4-develop with the following steps. But unable to reproduce the issue:

1. Setup Magento vanilla instance on local.
2. Create a product from the admin panel with a dummy image.
3. Change the permission of the pub/media folder recursively using chmod -R 111 pub/media command. I have tried other commands as well.
4. Try to access the product detail page from the admin panel. I have also tried to upload an image after changing the permission but the appropriate error message has appeared in that case.

The product edit page is open without any error.

I have also applied some logs in app/code/Magento/MediaStorage/Model/File/Storage/File.php to check if the saveFile function is called or not. But the logs are not printed in the debug.log file. Please have a look at the below screenshot:

Please let us know if we missed any steps in order to reproduce the issue.

Thanks

[mariusjp]

Hey @engcom-Hotel

As discussed before. I think it has to do with an S3 module I have installed, because I could very easily reproduce it on my server but not on a fresh install. And as we all (kinda silently) agreed upon, the assumption the code makes in the Exception, even though it's extensively checked before the Exception, is incorrect, hence why the PR wasn't closed.
But as you can see by my own screenshot the situation does occur, but in a very unique situation apparently.

[engcom-Hotel]

✔️ QA Passed

Hello @MJTheOne,

Thanks for the contribution!

I am now able to reproduce the issue. But in order to reproduce, I need to make some changes in code level. Please have a look at my description below:

1. Change the Media Storage as Database:
   

2. Go to the product edit page, and open the content tab, click on the Insert/Edit Image icon, this will popups the below window:
   

3. Now click on the Select Images icon, this will popups a new window as follows:
   

4. Check above screenshot having some error message, that is because of some code changes in order to reproduce the issue, below screenshot can refer for code changes:

5. Please find below the logs where the error is visible:

Before ✖️

But after deploying this branch and make the same changes in code, the PHP error is not visible in logs:
After ✔️

Tested all the manual scenarios, no impact on regression testing.

Thanks

[mariusjp]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.