Set media storage root to wysiwyg directory in pub/media

[vishal-7037]

Description (*)

Set Media storage root directory to wysiwyg directory in pub/media folder.

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. In the backend go to Content => Pages
2. Click 'Add New Page'
3. Expand the 'Content' section, click 'Show / Hide Editor' and click 'Insert Image...'

After Fixed:

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 on Travis CI are green)

[m2-assistant]

Hi @vishal-7037. 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-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

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

[vishal-7037]

@magento-engcom-team give me test instance

[magento-engcom-team]

Hi @vishal-7037. Thank you for your request. I'm working on Magento instance for you

[magento-engcom-team]

Hi @vishal-7037, here is your new Magento instance.
Admin access: https://pr-22681.instances.magento-community.engineering/admin
Login: admin Password: 123123q

[vishal-7037]

I've tested this changes on Vanilla Magento 2.3 instance and seems working fine.

Thanks.

[magento-engcom-team]

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

[VasylShvorak]

✔️ QA passed

[vishal-7037]

@magento-engcom-team give me test instance

[magento-engcom-team]

Hi @vishal-7037. Thank you for your request. I'm working on Magento instance for you

[magento-engcom-team]

Hi @vishal-7037, here is your new Magento instance.
Admin access: https://pr-22681.instances.magento-community.engineering/admin
Login: admin Password: 123123q

[vishal-7037]

HI @vishal-7037 thank you for collaboration, we found new issue with your changes.

1. Go to catalog -> categories
2. open creation form and expand Content tab
3. Click on image placeholder to upload an image A from local machine
4. Save Category
5. Click on "Select from Gallery button"

expected result: image A should be stored in MEdia Gallery storage that user are able to resue A in other upload place

Actual Result image A is not shown on Media Gallery Storage.

@Nazar65 ,I've checked this in the vanilla instance. and seems working fine.

Please check here: https://www.screencast.com/t/B9qZnhTeh

Thank you.

[Nazar65]

@vishal-7037 There is not

[vishal-7037]

@Nazar65 ,

For that we have to chage path from app/code/Magento/Catalog/etc/di.xml file.

<virtualType name="Magento\Catalog\CategoryImageUpload" type="Magento\Catalog\Model\ImageUploader">
        <arguments>
            <argument name="baseTmpPath" xsi:type="string">catalog/tmp/category</argument>
            <argument name="basePath" xsi:type="string">catalog/category</argument>
        </arguments>
    </virtualType>

and also related changes...

Can I proceed?

Thanks.

[vishal-7037]

I think this issue is created for CMS parts (Pages/Blocks).

So, I don't think we've to change for catalog section.

What do you think?

[vishal-7037]

@Nazar65 @sidolov
What is the process of this PR?
Need to proceed with the above discussion?

[hostep]

Ok, I started to look into this a bit.

I think the change of the getStorageRootSubpath is not correct here.
Since the initiator of the media gallery modal in javascript already passes on the path it wants to use encoded as base64.
For example in the category view, if you want to insert an image in the wysiwyg editor, it will pass d3lzaXd5Zw-- (base64 decoded: wysiwyg). However, if you click the "Select from Gallery" button, it passes Y2F0YWxvZy9jYXRlZ29yeQ,, (base64 decoded: catalog/category).

So we can't hardcode the path to use in that modal to wysiwyg as it will break the "Select from Gallery" button indeed.

What I did notice, is that if you already have a wysiwyg directory in pub/media/, then it will select that as the default directory when opening a modal from a wysiwyg field.
I think the problem we have in #22609 is the fact that we do not have a directory wysiwyg yet in the pub/media/ directory.
So a possible solution would be to simply create an empty directory wysiwyg when that modal gets opened and the wysiwyg directory doesn't already exists. (but then dynamically based on the requested directory which gets passed along, I'm just not sure about the security consequences of this, hopefully that path is already being cleaned up and can not contain .. for example?)

But I don't know if that would be enough? Shouldn't the root of the directory tree at the left side of that modal also not start from the directory you pass it? Instead of always starting from pub/media/?
I still find it a risk that store owners can simply manipulate the entire pub/media/ directory and its contents from a wysiwyg field.

@sidolov, @Nazar65 or @danmooney2, what are your thoughts on this?

@vishal-7037: are you still interested in trying to fix this issue if you get some more info?

[danmooney2]

There are folders that are excluded from being shown in media gallery in Magento/Cms/etc/di.xml, so to say entire pub/media contents can be altered currently from media gallery is not correct. I empathize with both the fix and the current functionality. If we feel that #22681 (review) doesn't provide enough value to justify current functionality then we can proceed with PR.

Shouldn't the root of the directory tree at the left side of that modal also not start from the directory you pass it? Instead of always starting from pub/media/?

Yes this is ideal. That would eliminate confusion and close the gap on any accidental or intentional modifications from underprivileged admins.

[hostep]

Thanks for the feeback @danmooney2!

There are folders that are excluded from being shown in media gallery in Magento/Cms/etc/di.xml, so to say entire pub/media contents can be altered currently from media gallery is not correct.

Thanks, I wasn't aware of this, good to know!

If we feel that #22681 (review) doesn't provide enough value to justify current functionality then we can proceed with PR.

I do feel like that remark is very valid, and should certainly be supported.
Third party modules could then also make use of this feature should they want to target a different directory in the pub/media directory.

The biggest problem in my eyes currently, is the fact that on a clean installation of Magento, the directories pub/media/wysiwyg and pub/media/catalog/category simply do not exist.
So if that media picker modal opens, it can't show and select those directories since they do not exist.

The first fix which needs to happen here, is that those directories get created. I'm just not sure when they should get created? When setting up a new project, or when that media picker modal first tries to access a directory which doesn't exist yet?
Previously, that wysiwyg directory got auto created when it didn't exist before, but that changed in this commit: a270b5b#diff-624b27794e761bd53831eba1ede517bcL73
So I'm in favor of letting the directories get created based on the dynamically parameter which gets passed from javascript to php. I'm just not sure about the security consequences of this. Can you try to figure out if that would be ok to do? Previously it was a hardcoded value, so that's pretty safe, but a dynamic value might not be so safe?

The second fix would then be to change the storage root to the directory which gets passed from javascript.

Let me know your thoughts!

[danmooney2]

I think it's OK to proceed with this PR. I wouldn't want dynamic folder creation based on client side values, and I (re-)noticed that a category image upon upload initially gets placed into a tmp folder inside catalog, and a) that's confusing UX to see that in media gallery b) if it gets chosen from the gallery inside tmp folder, then the file reference eventually becomes stale when it gets moved to its permanent residence. On top of that, file dispersion is enabled for products (i.e. p/o/p/popcorn.jpg ; overall not a desirable UX). Customizable storage root subpaths sound like a good idea at face value but there's too much baggage/overhead with the current inner workings of catalog media (like tmp storage) and there's legitimate concern of tampering with files contained within. Sure, the same file is going to be uploaded multiple times if the merchant wants to use the same category image they'd used prior, but it's the safer route.

[davemacaulay]

This PR has potential to break a lot of tests within Page Builder, even though we don't require bundled extensions to be ran for PRs I'd really appreciate if we could run Page Builder alongside the builds for this change.

[hostep]

Hmm, I don't really like the state of this PR, I would not merge it as is currently.

As already argued above, these directories which don't exist should get created somehow.
@davemacaulay: are there directories in pub/media which Page Builder needs which don't exist on a vanilla Magento installation?

For now I would just add the following lines to Magento\Cms\Helper\Wysiwyg\Images:: __construct:

$this->_directory->create(\Magento\Cms\Model\Wysiwyg\Config::IMAGE_DIRECTORY);
$this->_directory->create(\Magento\Catalog\Model\ResourceModel\Category\Attribute\Frontend\Image::IMAGE_PATH_SEGMENT); // but then maybe without using the const and just using 'catalog/category', because this would introduce a dependency we don't want

That would already solve the biggest problem for me.
Are there other directories which are being used in vanilla Magento which are needed for using this file explorer modal?

It's not the best solution, but might do for the interim, a more robust solution would need to be found later on which allows this to happen in a more automated fashion (maybe by defining directories in xml config files which should be created by default, this configuration could then be moved to the individual modules themselves instead of now doing this hardcoded in the cms module code and let the cms module code just find these configurations and create the directories if they don't exist?)

[davemacaulay]

@hostep we only utilize the wysiwyg folder, however we have a lot of automation that validates this user flow within Page Builder, so I want to ensure we don't break these tests when we merge this work.

[sidolov]

@davemacaulay , @hostep we ran builds with proposed changes and a lot of tests related to PageBuilder is failing due to usage of wysiwyg folder

[hostep]

Yes I'm aware that this PR is not good, I proposed a better change but haven't received much feedback on it yet. Should I just start a new PR with these suggested changes?

[hostep]

Created a follow up PR which might be a better fix: #24878
If it is better and gets approved, we should probably close this one.

[ihor-sviziev]

Hi @vishal-7037,

We have a fix #24878 for the same issue, but it does fix in backward compatible way, so by reviewing both solutions I decided to go with last one.

I'm closing this PR. Thank you so much for your contribution!

[m2-assistant]

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