Fix Issue #19872 - checking if image is in media directory #21131

Bartlomiejsz · 2019-02-11T12:07:35Z

Description (*)

Issue #19872 (closed) is still not fixed on develop branch.
On develop when image is added from media gallery while configuring category, it's path is saved as /media/path_to_file, but while Magento tries to check if this image is placed in media it compares it's path with pub/media/ instead of media/.

Fixed Issues (if relevant)

Magento 2.3 category edit page "Select from gallery" button not working. #19872: Magento 2.3 category edit page "Select from gallery" button not working.
Assigning Catalog Image from Gallery, then Saving Twice, Clears Image #22092

Manual testing scenarios (*)

Go to category add/edit form
Edit Category Image attribute using Select from Gallery option
Save category
Check if Category Image is visible in backoffice for this category

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)

magento-cicd2 · 2019-02-11T12:07:38Z

All committers have signed the CLA.

magento-engcom-team · 2019-02-11T12:07:39Z

Hi @Bartlomiejsz. 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

Bartlomiejsz · 2019-02-11T12:16:05Z

@magento-engcom-team give me 2.3-develop instance

magento-engcom-team · 2019-02-11T12:16:06Z

Hi @Bartlomiejsz. Thank you for your request. I'm working on Magento 2.3-develop instance for you

magento-engcom-team · 2019-02-11T12:16:36Z

Hi @Bartlomiejsz, here is your Magento instance.
Admin access: https://i-21131-2-3-develop.instances.magento-community.engineering/admin
Login: admin Password: 123123q
Instance will be terminated in up to 3 hours.

Bartlomiejsz · 2019-02-11T12:20:01Z

@magento-engcom-team give me test instance

magento-engcom-team · 2019-02-11T12:20:02Z

Hi @Bartlomiejsz. Thank you for your request. I'm working on Magento instance for you

magento-engcom-team · 2019-02-11T12:21:28Z

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

dmytro-ch · 2019-02-14T18:21:43Z

@magento-engcom-team give me test instance

magento-engcom-team · 2019-02-14T18:22:01Z

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

magento-engcom-team · 2019-02-14T18:22:57Z

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

dmytro-ch · 2019-02-14T19:36:49Z

Hi @Bartlomiejsz,
thank you for your contribution.
I colud not reproduce the issue on test instance in 2.3-develop branch.
Also, the current solution does not work on test instance.

Could you please make sure the issue is not related to your local envronment configuration?

Thank you!

Bartlomiejsz · 2019-02-15T13:13:24Z

@dmytro-ch that's right, I also noticed, that the issue doesn't appear on github-generated instance of Magento.

But I think this is related to location of Magento instance. If it is located in root directory (which is not recommended due to security, but I think still this is used by github-generated instance) the issue doesn't occur. But if Magento is installed in recommended pub/ directory, the issue occurs.
So it is my local environment configuration indeed, but also all environments that are installed according to Magento recommendations I think...

dmytro-ch · 2019-02-15T14:20:04Z

@Bartlomiejsz I was also thinking about that and tested the issue locally in 2.3-develop branch for both configurations /root and /pub.
As result, each time I got the same problem as you described, even when the document root is set to root Magento 2 directory. So I guess, this is not the reason of the issue.

However, after switching to PHP 7.2 the issue is not reproducible anymore.
Could you please check your current PHP version?

Thank you!

Bartlomiejsz · 2019-02-15T14:32:09Z

@dmytro-ch I'm currently running on 7.1.25, but I had same issue on instance running on 7.2.14

…gins with it

Bartlomiejsz · 2019-03-18T22:10:07Z

@magento-engcom-team give me test instance

magento-engcom-team · 2019-03-18T22:10:12Z

Hi @Bartlomiejsz. Thank you for your request. I'm working on Magento instance for you

Bartlomiejsz · 2019-03-19T08:42:38Z

@magento-engcom-team give me test instance

magento-engcom-team · 2019-03-19T08:42:42Z

Hi @Bartlomiejsz. Thank you for your request. I'm working on Magento instance for you

magento-engcom-team · 2019-03-19T08:43:40Z

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

Bartlomiejsz · 2019-03-19T08:46:41Z

@dmytro-ch could you please recheck this one? Current solution should work no matter if installation is in root or pub directory

…o feature/fix_19872_category_image_path

magento-engcom-team · 2019-05-28T21:27:02Z

Hi @Bartlomiejsz. Thank you for your request. I'm working on Magento 2.3-develop instance for you

magento-engcom-team · 2019-05-28T21:27:39Z

Hi @Bartlomiejsz, here is your Magento instance.
Admin access: https://i-21131-2-3-develop.instances.magento-community.engineering/admin
Login: admin Password: 123123q
Instance will be terminated in up to 3 hours.

Bartlomiejsz · 2019-05-28T21:39:02Z

Hi @sivaschenko. I think I understand you're point, but there is one problem I think. Sorry, I haven't thought about this before.
This PR fixes a bit different problem than we discussed. The problem is not caused by different ways of storing paths for images, but it is caused by different document root placement for magento instance.
You're saying about changing way that path is stored - so it would be relative to media. But the issue would still occur then, because if Magento would be installed in pub/ directory media path would have to begin with /media/, and if it would be installed in root dir (one directory up from pub/) - media path would have to begin with /pub/media/ - and it works this way in for all files in pub/ - media, static etc.
And this /pub at the beginning of path is reason of issue here, not two different ways of storing this path.
So I assume you're point can be good - this could be changed, but this PR should be checked as fix for another issue in this area I believe.

Changing way of storing those paths is feature rather than fix in my opinion - I don't think this is the reason of any issue currently.

sivaschenko · 2019-05-29T19:37:50Z

Hi @Bartlomiejsz , thanks for your response. Would you be able to add tests cases to \Magento\Catalog\Model\Category\FileInfo to cover both cases of paths for isExist and getMediaDirectoryPathRelativeToBaseDirectoryPath methods? I believe it will make things more clear.

Bartlomiejsz · 2019-05-29T23:09:17Z

Hi @sivaschenko, will work on it!

…o install dir

Bartlomiejsz · 2019-05-31T21:40:17Z

Hi @sivaschenko, added tests. Especially important here are paths beginning with /pub/media - for installation at root directory and beginning with /media - for installation at /pub directory

sivaschenko

Thanks for the tests @Bartlomiejsz

The only thing that is not covered by the fix is the custom pub directory path.

Please use $this->filesystem->getDirectoryRead(DirectoryList::PUB)->getRelativePath() to retrive the correct pub directory path

Bartlomiejsz · 2019-06-04T21:40:41Z

Hi @sivaschenko, done

magento-engcom-team · 2019-06-05T10:36:42Z

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

soleksii · 2019-06-06T06:14:51Z

✔️ QA Passed

…19872_category_image_path

m2-assistant · 2019-06-07T22:14:45Z

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

…directory magento#21131

wescey · 2020-01-10T14:20:34Z

Now when the selected image is saved (and is still there in backend), is it showing on frontend?

duckchip · 2020-01-27T08:51:02Z

I have that same problem.
After applying this patch, i got a very weird path on the frontend. (Broken image)

Fix checking if image is in media directory

ed43407

magento-engcom-team added Component: Catalog Release Line: 2.3 labels Feb 11, 2019

dmytro-ch self-requested a review February 11, 2019 19:40

dmytro-ch self-assigned this Feb 11, 2019

dmytro-ch added the Progress: needs update label Feb 14, 2019

Bartlomiejsz added 2 commits March 18, 2019 22:44

Merge branch '2.3-develop' into feature/fix_19872_category_image_path

619475d

Revert change from Root to Pub, trim pub directory if filePath not be…

17ed8d4

…gins with it

Merge branch '2.3-develop' of https://github.com/magento/magento2 int…

ae61b70

…o feature/fix_19872_category_image_path

Fix magento#19872 - unit tests for fileInfo with pub/ and root Magent…

07a296a

…o install dir

sivaschenko requested changes Jun 3, 2019

View reviewed changes

Fix magento#19872 - replace hardcoded pub path with getRelativePath call

a142ef2

p-bystritsky removed the Progress: needs update label Jun 5, 2019

sivaschenko approved these changes Jun 5, 2019

View reviewed changes

ghost added the Progress: ready for testing label Jun 5, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels Jun 5, 2019

ghost added Progress: extended testing and removed Progress: testing in progress labels Jun 6, 2019

p-bystritsky added 2 commits June 6, 2019 14:32

Merge remote-tracking branch 'mainline/2.3-develop' into feature/fix_…

de8df60

…19872_category_image_path

magento#21131: Static test fix.

d06fb10

ghost added Progress: accept and removed Progress: extended testing labels Jun 6, 2019

magento-engcom-team merged commit d06fb10 into magento:2.3-develop Jun 7, 2019

magento-engcom-team pushed a commit to okorshenko/magento2 that referenced this pull request Jun 7, 2019

ENGCOM-4591: Fix Issue magento#19872 - checking if image is in media …

ebf344e

…directory magento#21131

magento-engcom-team added this to the Release: 2.3.3 milestone Jun 7, 2019

Bartlomiejsz deleted the feature/fix_19872_category_image_path branch September 12, 2019 09:23

Fix Issue #19872 - checking if image is in media directory #21131

Fix Issue #19872 - checking if image is in media directory #21131

Uh oh!

Conversation

Bartlomiejsz commented Feb 11, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

Uh oh!

magento-cicd2 commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magento-engcom-team commented Feb 11, 2019

Uh oh!

Bartlomiejsz commented Feb 11, 2019

Uh oh!

magento-engcom-team commented Feb 11, 2019

Uh oh!

magento-engcom-team commented Feb 11, 2019

Uh oh!

Bartlomiejsz commented Feb 11, 2019

Uh oh!

magento-engcom-team commented Feb 11, 2019

Uh oh!

magento-engcom-team commented Feb 11, 2019

Uh oh!

dmytro-ch commented Feb 14, 2019

Uh oh!

magento-engcom-team commented Feb 14, 2019

Uh oh!

magento-engcom-team commented Feb 14, 2019

Uh oh!

dmytro-ch commented Feb 14, 2019

Uh oh!

Bartlomiejsz commented Feb 15, 2019

Uh oh!

dmytro-ch commented Feb 15, 2019

Uh oh!

Bartlomiejsz commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bartlomiejsz commented Mar 18, 2019

Uh oh!

magento-engcom-team commented Mar 18, 2019

Uh oh!

Bartlomiejsz commented Mar 19, 2019

Uh oh!

magento-engcom-team commented Mar 19, 2019

Uh oh!

magento-engcom-team commented Mar 19, 2019

Uh oh!

Bartlomiejsz commented Mar 19, 2019

Uh oh!

magento-engcom-team commented May 28, 2019

Uh oh!

magento-engcom-team commented May 28, 2019

Uh oh!

Bartlomiejsz commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sivaschenko commented May 29, 2019

Uh oh!

Bartlomiejsz commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bartlomiejsz commented May 31, 2019

Uh oh!

sivaschenko left a comment

Choose a reason for hiding this comment

Uh oh!

Bartlomiejsz commented Jun 4, 2019

Uh oh!

magento-engcom-team commented Jun 5, 2019

Uh oh!

soleksii commented Jun 6, 2019

Uh oh!

m2-assistant bot commented Jun 7, 2019

Uh oh!

Bartlomiejsz commented Feb 11, 2019 •

edited by ghost

Loading

magento-cicd2 commented Feb 11, 2019 •

edited

Loading

Bartlomiejsz commented Feb 15, 2019 •

edited

Loading

Bartlomiejsz commented May 28, 2019 •

edited

Loading

Bartlomiejsz commented May 29, 2019 •

edited

Loading

wescey commented Jan 10, 2020 •

edited

Loading