Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

1245: fix image still exists after delete #1279

Merged
merged 3 commits into from
Apr 28, 2020
Merged

1245: fix image still exists after delete #1279

merged 3 commits into from
Apr 28, 2020

Conversation

coderimus
Copy link
Contributor

@coderimus coderimus commented Apr 26, 2020 •
edited by sivaschenko
Loading

Description (*)

To fix the reported issue I provide this PR: use Magento\Cms\Model\Wysiwyg\Images\Storage::deleteFile for image delete process. This method removes the image physically and has a plugin which will remove data from a data storage - \Magento\MediaGallery\Plugin\Wysiwyg\Images\Storage::afterDeleteFile I also choose this method because of Magento_MediaGalleryUi has a dependency and use the Magento_CMS module.

Fixed Issues (if relevant)

#1245

Depends on: magento/magento2#27994

@coderimus coderimus added has dependency Backend Priority: P1 Needs to be fixed before any other issues labels Apr 26, 2020
@coderimus coderimus requested a review from sivaschenko April 26, 2020 12:13
@coderimus coderimus linked an issue Apr 26, 2020 that may be closed by this pull request
Deleting doesn't remove image from pub/media, thus it appears in Media Gallery after synchronizing #1245
Closed
@coderimus coderimus mentioned this pull request Apr 26, 2020
Merged
4 tasks
sivaschenko
sivaschenko requested changes Apr 26, 2020
View reviewed changes
Copy link
Member

@sivaschenko sivaschenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pull request, @coderimus ! please see my comment

MediaGalleryUi/Model/DeleteImage.php
public function execute(AssetInterface $asset): void
{
$mediaDirectory = $this->filesystem->getDirectoryRead(DirectoryList::MEDIA);
$absolutePath = $mediaDirectory->getAbsolutePath($asset->getPath());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check if the path is blacklisted before deleting an image (IsPathBlacklistedInterface).
Also, please add the information that the asset is removed from the database via the plugin

@sivaschenko sivaschenko added the Severity: S1 Affects critical data or functionality and forces users to employ a workaround label Apr 26, 2020
@sivaschenko
Copy link
Member

@coderimus does this pull request depends on magento/magento2#27994 ? Or what is the dependency?

@coderimus
Copy link
Contributor Author

@sivaschenko thank you for your review!
At the beginning of working on this PR, I faced with the issue I thought made those 2 PR related. But I was wrong. Dependencies removed.

sivaschenko
sivaschenko previously approved these changes Apr 26, 2020
View reviewed changes
@coderimus coderimus requested a review from sivaschenko April 26, 2020 20:18
engcom-Bravo
engcom-Bravo suggested changes Apr 27, 2020
View reviewed changes
Copy link
Contributor

@engcom-Bravo engcom-Bravo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ QA Failed

Now the behavior is opposite, the image is deleted from pub/media but, as I understand, it is not deleted from the database. As a result - broken images are displayed on Media Gallery grig
1245_2020-04-27

@coderimus
Copy link
Contributor Author

@engcom-Bravo thank you for the QA. May I asked you to make one more test: as I can see you added an image to the root directory and I faced with the same behaviour as you but could reproduce it only several times and than it was gone. Thought that it was my local problem.

Test scenario: add an image to any kind of folder except root media directory and then test delete it.

Thank you

@coderimus
Copy link
Contributor Author

@sivaschenko I would like to request your expertise :) Yesterday when I made this PR magento/magento2#27994 I faced with an issue which I considered as my local. But it appears to be the real one according to the current PR QA result.

Steps to reproduce: add an image to the root media gallery folder. It's path should be /download.jpg for example. After that try to delete it. The result will be next: image deleted, an entry in the media_content_asset still exists.

I found that the reason for this is that the $path variable here equals to the image name without / for cases as I described before.

I tried to fix it like on my screenshot and it seems to be work but I am not totally sure that this is the correct way. Please, check this.

image

@sivaschenko
Copy link
Member

sivaschenko commented Apr 27, 2020 •
edited
Loading

@coderimus I don't think the path should be changed in the delete service, the correct path should be passed by client

@lenaorobei lenaorobei added this to the 1.1.0 milestone Apr 27, 2020
@Nazar65
Copy link
Member

Nazar65 commented Apr 28, 2020

✔️ QA Passed images deleted from physical storage, folder deleted from physical storage and from the media_gallery_asset
Note Tested with linked PR on Magento 2 magento/magento2#27994

@sivaschenko sivaschenko mentioned this pull request Apr 28, 2020
Merged
@lenaorobei lenaorobei merged commit 77248b4 into magento:1.1-develop Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@engcom-Bravo engcom-Bravo engcom-Bravo requested changes

@sivaschenko sivaschenko Awaiting requested review from sivaschenko

Assignees

@sivaschenko sivaschenko

@engcom-Bravo engcom-Bravo

Labels
Award: bug fix Award: category of expertise Backend Community Insider: Magecom community-insider-contribution has dependency Priority: P1 Needs to be fixed before any other issues Severity: S1 Affects critical data or functionality and forces users to employ a workaround
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.

Deleting doesn't remove image from pub/media, thus it appears in Media Gallery after synchronizing
6 participants
@coderimus @sivaschenko @Nazar65 @engcom-Bravo @lenaorobei @magento-engcom-team