Remove media gallery assets metadata when a directory removed #26015

rogyar · 2019-12-12T15:20:18Z

Description (*)

This PR introduces a logic for removing meta-information from media gallery (database records) once a directory with media assets has been removed.

Fixed Issues (if relevant)

The image state is not updated when the containing folder has been deleted adobe-stock-integration#825 : The image state is not updated when the containing folder has been deleted

Manual testing scenarios (*)

Install Magento with Adobe Stock Integration
Configure the integration in Stores -> Configuration -> System -> Adobe Stock Integration fieldset

From Magento Admin go to Content - Pages, click Add New Page
Expand Content Click Show / Hide Editor, select Insert Image...
Select Storage Root and click Create Folder, name the folder and click OK
Select the previously created folder and Click Search Adobe Stock
Select an image from Adobe Stock and Save Preview, Confirm
Select the previously created folder and click Delete Folder
Click Search Adobe Stock again and observe the previously saved image

The "Save Preview" and "License and Save" buttons should be present and work as expected

m2-assistant · 2019-12-12T15:20:22Z

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

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

rogyar · 2019-12-12T15:21:46Z

Hi @sivaschenko.

Could you, please, review the proof of concept? After your green light, I will proceed with the tests.

Thank you!

lbajsarowicz · 2019-12-14T20:51:02Z

app/code/Magento/MediaGallery/Model/Asset/Command/DeleteByDirectoryPath.php

+            if (substr($directoryPath, -1) !== '/') {
+                $directoryPath .= '/';
+            }


I haven't measured that - but isn't the better option just rtrim($directoryPath, '/').'/'

lbajsarowicz · 2019-12-14T20:51:58Z

app/code/Magento/MediaGallery/Model/Asset/Command/DeleteByDirectoryPath.php

+            $connection = $this->resourceConnection->getConnection();
+            $tableName = $this->resourceConnection->getTableName(self::TABLE_MEDIA_GALLERY_ASSET);
+            $connection->delete($tableName, [self::MEDIA_GALLERY_ASSET_PATH . ' LIKE ?' => $directoryPath . '%']);
+        } catch (\Exception $exception) {


Import? :-)

lbajsarowicz · 2019-12-14T20:54:16Z

app/code/Magento/MediaGallery/Plugin/Wysiwyg/Images/Storage.php

+        try {
+            $this->deleteMediAssetByDirectoryPath->execute($relativePath);
+        } catch (\Exception $exception) {
+            $this->logger->critical($exception);


You are logging the same exception twice?
https://github.com/magento/magento2/pull/26015/files#diff-01fc33f90d1b68e3f97bbed6e144e565R72

rogyar · 2019-12-15T15:12:50Z

Hi @lbajsarowicz. Thank you for your review.
This PR is going to be reworked since it's just a proof of concept. I'm waiting for confirmation from @sivaschenko since we have discussed this approach separately and now working together on a final implementation 😉

lbajsarowicz · 2019-12-15T19:47:30Z

I saw "WIP" notification so I'm aware of the work that is in progress. Just giving the feedback.

sivaschenko

Hi @rogyar thanks for proof of concept, the approach looks right to me. The only proposition is to involve MediaDirectory instance in DeleteByDirectoryPath service (instead of the plugin) to validate that a valid path inside the media directory is provided (getRelativePath, isExists) before further operations based on provided paths

rogyar · 2019-12-26T06:24:13Z

Hi @sivaschenko. Thank you for your suggestion. Working on the solution

sivaschenko

Hi @rogyar thanks for the update. Please take a look at my comments

sivaschenko · 2020-01-06T12:03:03Z

app/code/Magento/MediaGallery/Model/Asset/Command/DeleteByDirectoryPath.php

+    public function execute(string $directoryPath): void
+    {
+        try {
+            $this->validateDirectoryPath($directoryPath);


validateDirectoryPath throws a LocalizedException. I don't think it's necessary to recapture that exception, I propose to move the call out of try-catch block (also it may be good to change the LocalizedException to more specific CouldNotDeleteException in validateDirectoryPath)

sivaschenko · 2020-01-06T12:10:51Z

app/code/Magento/MediaGallery/Plugin/Wysiwyg/Images/Storage.php

+            return $result;
+        }
+
+        $relativePath = $this->filesystem->getDirectoryRead(DirectoryList::MEDIA)->getRelativePath($path);


getRelativePath throws ValidatorException. I think it should be logged without interrupting the code execution.

Also, it looks like getRelativePath will return the original passed path if an incorrect path was provided, so I'd check if the path exists in media directory not to remove assets if the StorageSubject provided an incorrect path for some reason

rogyar · 2020-01-10T13:39:40Z

@magento run tests

sivaschenko · 2020-02-07T15:14:15Z

app/code/Magento/MediaGallery/Plugin/Wysiwyg/Images/Storage.php

+        try {
+            $mediaDirectoryRead = $this->filesystem->getDirectoryRead(DirectoryList::MEDIA);
+            $relativePath = $mediaDirectoryRead->getRelativePath($path);
+            if ($mediaDirectoryRead->isExist($relativePath) === false) {


Is this check necessary? Considering that it is afterDeleteDirectory method body?

Hi @sivaschenko.

The idea was the following. If the original method's (deleteDirectory) $path for some reason does not exist, we most likely will not have an exception, just successful method execution (see:

magento2/lib/internal/Magento/Framework/Filesystem/Directory/Write.php

Line 182 in 3adb1eb

return true;

).
After that our plugin will execute the DB query which does not make sense.

This is an edge-case. If you believe that we don't need this check, I'm OK as well.

Thank you!

@rogyar I think this check should be removed as the plugin is afterDeleteDirectory so the directory should not exist when it's executed anyway

magento-engcom-team · 2020-02-17T21:16:22Z

Hi @sivaschenko, thank you for the review.
ENGCOM-6918 has been created to process this Pull Request
✳️ @sivaschenko, could you please add one of the following labels to the Pull Request?

Label	Description
`Auto-Tests: Covered`	All changes in Pull Request is covered by auto-tests
`Auto-Tests: Not Covered`	Changes in Pull Request requires coverage by auto-tests
`Auto-Tests: Not Required`	Changes in Pull Request does not require coverage by auto-tests

engcom-Delta · 2020-02-18T12:47:45Z

Hi @rogyar I am still able to reproduce issue #825 on PR branch with adobe-stock-integration (branch 1.1-develop)

Could you take a look?

rogyar · 2020-02-24T15:43:49Z

Hi @engcom-Delta it was another issue connected rather to the files rendering than to the issue that was supposed to be fixed in the scope of this PR. Anyway, I have tweaked the rendering process as well in my recent commit. Could you check it out once again, please?

Thank you!

rogyar · 2020-03-02T11:05:37Z

Hi @engcom-Delta. Did you have a chance to review this PR? Thank you

engcom-Delta · 2020-03-02T15:03:20Z

Hi @rogyar I rechecked PR with latest changes and now issue is fixed
@lbajsarowicz @sivaschenko Could you review last commit?

rogyar · 2020-03-11T08:13:26Z

HI @sivaschenko. Could you review this one, please, once again?

Thank you!

sivaschenko · 2020-03-18T17:06:21Z

Thanks for updates @rogyar !

magento-engcom-team · 2020-03-18T17:06:45Z

Hi @sivaschenko, thank you for the review.
ENGCOM-6918 has been created to process this Pull Request
✳️ @sivaschenko, could you please add one of the following labels to the Pull Request?

Label	Description
`Auto-Tests: Covered`	All changes in Pull Request is covered by auto-tests
`Auto-Tests: Not Covered`	Changes in Pull Request requires coverage by auto-tests
`Auto-Tests: Not Required`	Changes in Pull Request does not require coverage by auto-tests

sivaschenko · 2020-03-18T17:08:27Z

Failed B2B MFTF test doesn't seem to be related to the changes

…moved #26015

m2-assistant · 2020-03-19T08:12:44Z

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

[WIP] Remove media gallery assets metadata when a directory removed

ed0cdb1

magento-engcom-team added Partner: Atwix Pull Request is created by partner Atwix partners-contribution Pull Request is created by Magento Partner labels Dec 12, 2019

lbajsarowicz reviewed Dec 14, 2019

View reviewed changes

sivaschenko reviewed Dec 19, 2019

View reviewed changes

rogyar added 2 commits December 26, 2019 06:34

Merge branch '2.4-develop' into 825-adobestock-remove-directory

f6ac9e7

Refactoring and unit test coverage

da9526f

rogyar added 2 commits December 30, 2019 15:22

Added an extended class purpose description

145cb44

Changed DocBlock description of the parameter type

8679c9a

rogyar changed the title ~~[WIP] Remove media gallery assets metadata when a directory removed~~ Remove media gallery assets metadata when a directory removed Jan 4, 2020

rogyar requested a review from sivaschenko January 4, 2020 09:20

sivaschenko requested changes Jan 6, 2020

View reviewed changes

ghost assigned sivaschenko Jan 6, 2020

ghost added the Progress: needs update label Jan 6, 2020

Errors handling refactoring and additional test coverage

847e5bc

rogyar requested a review from sivaschenko January 10, 2020 09:15

sivaschenko added 2 commits January 24, 2020 13:22

Merge branch '2.4-develop' into 825-adobestock-remove-directory

c24206e

Merge branch '2.4-develop' into 825-adobestock-remove-directory

57a6a4b

sivaschenko requested changes Feb 7, 2020

View reviewed changes

Removed unnecessary check from the plugin

dce54ce

sivaschenko approved these changes Feb 17, 2020

View reviewed changes

ghost removed the Progress: needs update label Feb 17, 2020

magento-engcom-team added the Component: MediaGalleryApi label Feb 17, 2020

engcom-Delta self-assigned this Feb 18, 2020

ghost added Progress: testing in progress and removed Progress: ready for testing labels Feb 18, 2020

ghost added Progress: needs update and removed Progress: testing in progress labels Feb 18, 2020

rogyar added 2 commits February 23, 2020 13:49

Merge branch '2.4-develop' into 825-adobestock-remove-directory

1eabf19

Refresh file list after a directory has been removed

5c94848

rogyar requested a review from engcom-Delta March 2, 2020 11:05

sivaschenko approved these changes Mar 18, 2020

View reviewed changes

ghost added Progress: ready for testing Progress: testing in progress Progress: accept and removed Progress: needs update Progress: ready for testing Progress: testing in progress labels Mar 18, 2020

magento-engcom-team added the Area: Lib/Frontend label Mar 18, 2020

sivaschenko added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Mar 18, 2020

magento-engcom-team pushed a commit that referenced this pull request Mar 19, 2020

ENGCOM-6918: Remove media gallery assets metadata when a directory re…

799f9a1

…moved #26015

magento-engcom-team merged commit 8a78108 into magento:2.4-develop Mar 19, 2020

Remove media gallery assets metadata when a directory removed #26015

Remove media gallery assets metadata when a directory removed #26015

Uh oh!

Conversation

rogyar commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Uh oh!

m2-assistant bot commented Dec 12, 2019

Uh oh!

rogyar commented Dec 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogyar commented Dec 15, 2019

Uh oh!

lbajsarowicz commented Dec 15, 2019

Uh oh!

sivaschenko left a comment

Choose a reason for hiding this comment

Uh oh!

rogyar commented Dec 26, 2019

Uh oh!

sivaschenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogyar commented Jan 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magento-engcom-team commented Feb 17, 2020

Uh oh!

engcom-Delta commented Feb 18, 2020

Uh oh!

rogyar commented Feb 24, 2020

Uh oh!

rogyar commented Mar 2, 2020

Uh oh!

engcom-Delta commented Mar 2, 2020

Uh oh!

rogyar commented Mar 11, 2020

Uh oh!

sivaschenko commented Mar 18, 2020

Uh oh!

magento-engcom-team commented Mar 18, 2020

Uh oh!

sivaschenko commented Mar 18, 2020

Uh oh!

m2-assistant bot commented Mar 19, 2020

Uh oh!

Uh oh!

rogyar commented Dec 12, 2019 •

edited

Loading