Create missing directories in imageuploader tree if they don't alread…

[hostep]

…y exist.

Description (*)

This is a replacement for #22681
This is partially fixing #22609

I believe we can no longer fully fix #22609 as the bug has existed for too long (2.3.0, 2.3.1, 2.3.2 & 2.3.3 are affected). If people accidentally uploaded images directly in the pub/media directory and we fully fix #22609 by changing the storage root, they would no longer be able to access those uploaded images in the wrong directory.

This is a fix which creates those missing directories pub/media/wysiwyg or pub/media/catalog/category if the image uploader modal is being opened and those directories don't already exists.

Those directories get declared in some xml files (in the initialMediaGalleryOpenSubpath node):

• https://github.com/magento/magento2/blob/2.3.2/app/code/Magento/Catalog/view/adminhtml/ui_component/category_form.xml#L183
• https://github.com/magento/magento2/blob/2.3.2/app/code/Magento/Ui/view/base/ui_component/etc/definition.xml#L149

And then javascript sends those paths in an ajax request using the parameter 'current_tree_path' after which php then creates those directories if they don't already exist.
It would be great if somebody could take a look at this from a security perspective if this is safe enough, or if some extra security precautions should be taken here to make sure javascript can't willy-nilly create all kinds of unwanted directories? It is probably safe enough due to that regex checking, but I'm not 100% sure?

I believe the current code didn't behave like it was originally intended, because these lines are never executed because the $currentPath variable only gets set when that directory already exists.

I think this problem was probably introduced by some combination of oversights in MAGETWO-87583 and MAGETWO-87986.

/cc @danmooney2, @davemacaulay, @sidolov

Fixed Issues (if relevant)

1. Since Magento 2.3 the wysiwyg image uploader incorrectly uses pub/media as storage root. #22609: Since Magento 2.3 the wysiwyg image uploader incorrectly uses pub/media as storage root

Manual testing scenarios (*)

1. Setup a vanilla Magento installation
2. Verify that the directories pub/media/wysiwyg and pub/media/catalog/category don't exist
3. Log into the backend of Magento, and edit a category
4. Open the Content tab, and click the 'Select from Media Gallery' button.
5. Also click the 'Insert Image' button in the content wysiwyg field
6. It is expected the image uploader selects the correct directories even if they previously didn't existed, with this fix this is now happening, and without this fix, it wasn't happening.

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)
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @hostep. 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.3-develop instance - deploy vanilla Magento instance

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

[m2-assistant]

Hi @hostep, 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.

[hostep]

Closed and re-opened PR for that CLA check.

[hostep]

While trying to fix the unit tests, I start to feel like some more things needs to get refactored, because this getCurrentPath method seems to be becoming quite a mess and responsible for 2 different things.
The behavior seems to be correct now, but the code is really not ok I think.

I'll wait a bit on feedback before continuing this to see if this is the right approach to take to tackle #22609.

To sumarize what I think is needed here (behavioral-wise):

• The Storage Root should always be 'pub/media'
• The selected directory in the tree should be the one passed in the current_tree_path parameter
• If the selected directory (or one of its parents) doesn't exist, it should be created automatically.

[hostep]

Thanks for the feedback @kalpmehta!
I'll wait a bit with addressing it until I got some other feedback to first see if this is the right approach which I'm taking here.

[kalpmehta]

@hostep Do you still want to wait for this PR?

[magento-engcom-team]

Hi @kalpmehta, thank you for the review.
ENGCOM-6275 has been created to process this Pull Request

[engcom-Alfa]

@magento run all tests

[engcom-Alfa]

✔️ QA Passed

[hostep]

Rebased PR on top of latest 2.4-develop, to see what the tests will say.
Will attempt to put some more time into this in the upcoming days/weeks (bit busy in personal life the last few weeks, but I'll see what I can do).

Would be great if there could be some feedback on my questions, in the mean while, thanks!

[hostep]

Force pushed some more updates (also rebased again on latest 2.4-develop).
Unit tests should pass now.

I'm still not too happy about this, I feel like the original code was not readable enough and it's still not that much improved I think.

[hostep]

@kalpmehta or somebody else: this can be reviewed again and feedback is welcome :)

[magento-engcom-team]

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

[engcom-Alfa]

✔️ QA Passed

[m2-assistant]

Hi @hostep, 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.