-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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.
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
Remove media gallery assets metadata when a directory removed #26015
Remove media gallery assets metadata when a directory removed #26015
Conversation
Hi @rogyar. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
Hi @sivaschenko. Could you, please, review the proof of concept? After your green light, I will proceed with the tests. Thank you! |
if (substr($directoryPath, -1) !== '/') { | ||
$directoryPath .= '/'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't measured that - but isn't the better option just rtrim($directoryPath, '/').'/'
$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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import? :-)
try { | ||
$this->deleteMediAssetByDirectoryPath->execute($relativePath); | ||
} catch (\Exception $exception) { | ||
$this->logger->critical($exception); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are logging the same exception twice?
https://github.com/magento/magento2/pull/26015/files#diff-01fc33f90d1b68e3f97bbed6e144e565R72
Hi @lbajsarowicz. Thank you for your review. |
I saw "WIP" notification so I'm aware of the work that is in progress. Just giving the feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Hi @sivaschenko. Thank you for your suggestion. Working on the solution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rogyar thanks for the update. Please take a look at my comments
public function execute(string $directoryPath): void | ||
{ | ||
try { | ||
$this->validateDirectoryPath($directoryPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
)
return $result; | ||
} | ||
|
||
$relativePath = $this->filesystem->getDirectoryRead(DirectoryList::MEDIA)->getRelativePath($path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@magento run tests |
try { | ||
$mediaDirectoryRead = $this->filesystem->getDirectoryRead(DirectoryList::MEDIA); | ||
$relativePath = $mediaDirectoryRead->getRelativePath($path); | ||
if ($mediaDirectoryRead->isExist($relativePath) === false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this check necessary? Considering that it is afterDeleteDirectory
method body?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rogyar I think this check should be removed as the plugin is afterDeleteDirectory
so the directory should not exist when it's executed anyway
Hi @sivaschenko, thank you for the review.
|
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! |
Hi @engcom-Delta. Did you have a chance to review this PR? Thank you |
Hi @rogyar I rechecked PR with latest changes and now issue is fixed |
HI @sivaschenko. Could you review this one, please, once again? Thank you! |
Thanks for updates @rogyar ! |
Hi @sivaschenko, thank you for the review.
|
Failed B2B MFTF test doesn't seem to be related to the changes |
Hi @rogyar, thank you for your contribution! |
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)
Manual testing scenarios (*)
The "Save Preview" and "License and Save" buttons should be present and work as expected