Prevent saving upload media files in root directory

[kenboy]

Description (*)

If it is impossible to save pictures to the "media" directory, the save goes to the root directory

Manual testing scenarios (*)

1. Make a ban on writing to the "pub/media/catalog/product" directory
2. Start importing goods containing media information

Questions or comments

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 are green)

[m2-assistant]

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

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

[rogyar]

Hi @kenboy. Thank you for your collaboration.
Could you provide a little bit more information regarding the main idea of your PR, please? From the PR title/description I see that the goal is to save files to the root (it would be great if you could clarify the "root" definition here since it's completely relative) if the media directory is not writable. However, I see that the following statement:

$destinationDir = "catalog/product";
$destinationPath = $dirAddon . '/' . $this->_mediaDirectory->getRelativePath($destinationDir);

stays unchanged. So, the destination path will be the same as previously. Could you clarify this part, please?

Thank you!

[kenboy]

Hi @rogyar
When the path formed by the following statement:

$destinationDir = "catalog/product";
$destinationPath = $dirAddon . '/' . $this->_mediaDirectory->getRelativePath($destinationDir);

and not available for write, an exception is thrown:

if (!$this->_fileUploader->setDestDir($destinationPath)) {
  throw new LocalizedException(
    __('File directory \'%1\' is not writable.', $destinationPath)
  );
}

setting the "\Magento\CatalogImportExport\Model\Import\Uploader::$_destDir" properties does not occur, the object is not fully formed.

But since it is already stored in the property "\Magento\CatalogImportExport\Model\Import\Product::$_fileUploader", when the function is called again, an incomplete object is used.

[rogyar]

Hi @kenboy. Got it, now it makes sense, thank you!

May I kindly ask you to cover the proposed change by a simple unit test, please? The test will invoke getUploader() method a couple of times.
On the very first invocation, we mock throwing the "not writable" exception.
On the second invocation, we assert that the routine for uploader initialization is being executed (not skipped as in case of the current implementation with $this->_fileUploader === null).

Thank you!

[magento-engcom-team]

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

[magento-engcom-team]

@kenboy thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

[engcom-Alfa]

✔️ QA Passed

[m2-assistant]

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