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

Product images are being duplicated on import #26713

Merged
merged 12 commits into from
Oct 13, 2020
Merged

Product images are being duplicated on import #26713

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
912b3f7
Added hashbased check to prevent image duplication on product import …
Feb 5, 2020
ffdb341
Adjusted the product import test
Feb 5, 2020
eb8607c
First try-out to fix static tests / integration test
Feb 5, 2020
e4812b8
Fix unit tests
Feb 25, 2020
d03b223
Merge branch '2.4-develop' into product-import-fix-duplicate-images
engcom-Charlie Sep 10, 2020
d4931f7
fix duplicate image during import products, refactor tests
engcom-Charlie Sep 14, 2020
f16a12f
Merge branch '2.4-develop' into product-import-fix-duplicate-images
engcom-Charlie Sep 22, 2020
819fc7c
refactor
engcom-Charlie Sep 22, 2020
f16811c
Merge branch '2.4-develop' into product-import-fix-duplicate-images
engcom-Charlie Sep 24, 2020
bca6bf6
fix import image from external url
engcom-Charlie Sep 24, 2020
3da6f23
refactor
engcom-Charlie Sep 25, 2020
9b505ee
Merge branch '2.4-develop' into product-import-fix-duplicate-images
PieterCappelle Oct 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
187 changes: 163 additions & 24 deletions app/code/Magento/CatalogImportExport/Model/Import/Product.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1020,6 +1020,7 @@ public function setParameters(array $params)
* Delete products for replacement.
*
* @return $this
* @throws \Exception
*/
public function deleteProductsForReplacement()
{
Expand Down Expand Up @@ -1111,6 +1112,11 @@ protected function _importData()
* Replace imported products.
*
* @return $this
* @throws LocalizedException
* @throws \Magento\Framework\Exception\CouldNotSaveException
* @throws \Magento\Framework\Exception\InputException
* @throws \Magento\Framework\Validation\ValidationException
* @throws \Zend_Validate_Exception
*/
protected function _replaceProducts()
{
Expand All @@ -1132,6 +1138,11 @@ protected function _replaceProducts()
* Save products data.
*
* @return $this
* @throws LocalizedException
* @throws \Magento\Framework\Exception\CouldNotSaveException
* @throws \Magento\Framework\Exception\InputException
* @throws \Magento\Framework\Validation\ValidationException
* @throws \Zend_Validate_Exception
*/
protected function _saveProductsData()
{
Expand Down Expand Up @@ -1274,6 +1285,11 @@ protected function _prepareRowForDb(array $rowData)
* Must be called after ALL products saving done.
*
* @return $this
* @SuppressWarnings(PHPMD.CyclomaticComplexity)
* @SuppressWarnings(PHPMD.NPathComplexity)
* @SuppressWarnings(PHPMD.ExcessiveMethodLength)
* @throws LocalizedException
* phpcs:disable Generic.Metrics.NestingLevel
*/
protected function _saveLinks()
{
Expand Down Expand Up @@ -1305,6 +1321,7 @@ protected function _saveLinks()
*
* @param array $attributesData
* @return $this
* @throws \Exception
*/
protected function _saveProductAttributes(array $attributesData)
{
Expand Down Expand Up @@ -1436,6 +1453,7 @@ private function getOldSkuFieldsForSelect()
*
* @param array $newProducts
* @return void
* @throws \Exception
*/
private function updateOldSku(array $newProducts)
{
Expand All @@ -1459,6 +1477,7 @@ private function updateOldSku(array $newProducts)
* Get new SKU fields for select
*
* @return array
* @throws \Exception
*/
private function getNewSkuFieldsForSelect()
{
Expand Down Expand Up @@ -1542,6 +1561,7 @@ public function getImagesFromRow(array $rowData)
* @SuppressWarnings(PHPMD.ExcessiveMethodLength)
* @SuppressWarnings(PHPMD.UnusedLocalVariable)
* @throws LocalizedException
* @throws \Zend_Validate_Exception
* phpcs:disable Generic.Metrics.NestingLevel.TooHigh
*/
protected function _saveProducts()
Expand All @@ -1559,12 +1579,17 @@ protected function _saveProducts()
$this->categoriesCache = [];
$tierPrices = [];
$mediaGallery = [];
$uploadedFiles = [];
$galleryItemsToRemove = [];
$labelsForUpdate = [];
$imagesForChangeVisibility = [];
$uploadedImages = [];
$previousType = null;
$prevAttributeSet = null;

$importDir = $this->_mediaDirectory->getAbsolutePath($this->getImportDir());
$existingImages = $this->getExistingImages($bunch);
$this->addImageHashes($existingImages);

foreach ($bunch as $rowNum => $rowData) {
// reset category processor's failed categories array
Expand Down Expand Up @@ -1660,6 +1685,7 @@ protected function _saveProducts()
if (!array_key_exists($rowSku, $this->websitesCache)) {
$this->websitesCache[$rowSku] = [];
}

// 2. Product-to-Website phase
if (!empty($rowData[self::COL_PRODUCT_WEBSITES])) {
$websiteCodes = explode($this->getMultipleValueSeparator(), $rowData[self::COL_PRODUCT_WEBSITES]);
Expand Down Expand Up @@ -1711,12 +1737,11 @@ protected function _saveProducts()
foreach (array_keys($imageHiddenStates) as $image) {
//Mark image as uploaded if it exists
if (array_key_exists($image, $rowExistingImages)) {
$rowImages[self::COL_MEDIA_IMAGE][] = $image;
$uploadedImages[$image] = $image;
}
//Add image to hide to images list if it does not exist
if (empty($rowImages[self::COL_MEDIA_IMAGE])
|| !in_array($image, $rowImages[self::COL_MEDIA_IMAGE])
) {

if (empty($rowImages)) {
$rowImages[self::COL_MEDIA_IMAGE][] = $image;
}
}
Expand All @@ -1730,29 +1755,68 @@ protected function _saveProducts()
$position = 0;
foreach ($rowImages as $column => $columnImages) {
foreach ($columnImages as $columnImageKey => $columnImage) {
if (!isset($uploadedImages[$columnImage])) {
$uploadedFile = $this->uploadMediaFiles($columnImage);
$uploadedFile = $uploadedFile ?: $this->getSystemFile($columnImage);
if ($uploadedFile) {
$uploadedImages[$columnImage] = $uploadedFile;
} else {
unset($rowData[$column]);
$this->addRowError(
ValidatorInterface::ERROR_MEDIA_URL_NOT_ACCESSIBLE,
$rowNum,
null,
null,
ProcessingError::ERROR_LEVEL_NOT_CRITICAL
);
$hash = '';
if (filter_var($columnImage, FILTER_VALIDATE_URL) === false) {
$filename = $importDir . DIRECTORY_SEPARATOR . $columnImage;
if (file_exists($filename)) {
$hash = hash_file('sha256', $importDir . DIRECTORY_SEPARATOR . $columnImage);
}
} else {
$uploadedFile = $uploadedImages[$columnImage];
$hash = hash_file('sha256', $columnImage);
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably don't need to call the hash_file twice here, wouldn't be better to define something like $imageFilePath and then call the private method to hash the file just once.

}

// Add new images
if (empty($rowExistingImages)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we need a comment to explain the intention it may worth creating a private method with a descriptive name.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually would recommend wrapping this ifelse within a function that returns a bool, and just use it on row 1786 instead of defining a variable.

$imageAlreadyExists = false;
} else {
$imageAlreadyExists = array_reduce(
$rowExistingImages,
function ($exists, $file) use ($hash) {
if ($exists) {
return $exists;
}
if (isset($file['hash']) &&
!empty($file['hash']) &&
$file['hash'] === $hash) {
return $file['value'];
}
return $exists;
},
''
);
}

if ($imageAlreadyExists) {
$uploadedFile = $imageAlreadyExists;
} else {
if (!isset($uploadedImages[$columnImage])) {
$uploadedFile = $this->uploadMediaFiles($columnImage);
$uploadedFile = $uploadedFile ?: $this->getSystemFile($columnImage);
if ($uploadedFile) {
$uploadedImages[$columnImage] = $uploadedFile;
} else {
unset($rowData[$column]);
$this->addRowError(
ValidatorInterface::ERROR_MEDIA_URL_NOT_ACCESSIBLE,
$rowNum,
null,
null,
ProcessingError::ERROR_LEVEL_NOT_CRITICAL
);
}
} else {
$uploadedFile = $uploadedImages[$columnImage];
}
}

if ($uploadedFile && $column !== self::COL_MEDIA_IMAGE) {
$rowData[$column] = $uploadedFile;
}

if ($uploadedFile) {
$uploadedFiles[] = $uploadedFile;
}

if (!$uploadedFile || isset($mediaGallery[$storeId][$rowSku][$uploadedFile])) {
continue;
}
Expand Down Expand Up @@ -1800,6 +1864,14 @@ protected function _saveProducts()
}
}

// 5.1 Items to remove phase
if (!empty($rowExistingImages)) {
$galleryItemsToRemove[] = \array_diff(
\array_keys($rowExistingImages),
$uploadedFiles
);
}

// 6. Attributes phase
$rowStore = (self::SCOPE_STORE == $rowScope)
? $this->storeResolver->getStoreCodeToId($rowData[self::COL_STORE])
Expand Down Expand Up @@ -1910,6 +1982,8 @@ protected function _saveProducts()
$tierPrices
)->_saveMediaGallery(
$mediaGallery
)->_removeOldMediaGalleryItems(
$galleryItemsToRemove
)->_saveProductAttributes(
$attributes
)->updateMediaGalleryVisibility(
Expand All @@ -1928,6 +2002,27 @@ protected function _saveProducts()
}
//phpcs:enable Generic.Metrics.NestingLevel

/**
* Generate hashes for existing images for comparison with newly uploaded images.
*
* @param array $images
*/
public function addImageHashes(&$images)
{
$productMediaPath = $this->filesystem->getDirectoryRead(DirectoryList::MEDIA)
->getAbsolutePath('/catalog/product');

foreach ($images as $storeId => $skus) {
foreach ($skus as $sku => $files) {
foreach ($files as $path => $file) {
if (file_exists($productMediaPath . $file['value'])) {
$images[$storeId][$sku][$path]['hash'] = hash_file('sha256', $productMediaPath . $file['value']);
}
}
}
}
}

/**
* Prepare array with image states (visible or hidden from product page)
*
Expand Down Expand Up @@ -2063,6 +2158,24 @@ protected function _saveProductTierPrices(array $tierPriceData)
return $this;
}

/**
* Returns the import directory if specified or a default import directory (media/import).
*
* @return string
*/
protected function getImportDir()
{
$dirConfig = DirectoryList::getDefaultConfig();
$dirAddon = $dirConfig[DirectoryList::MEDIA][DirectoryList::PATH];

if (!empty($this->_parameters[Import::FIELD_NAME_IMG_FILE_DIR])) {
$tmpPath = $this->_parameters[Import::FIELD_NAME_IMG_FILE_DIR];
} else {
$tmpPath = $dirAddon . '/' . $this->_mediaDirectory->getRelativePath('import');
}
return $tmpPath;
}

/**
* Returns an object for upload a media files
*
Expand All @@ -2079,11 +2192,7 @@ protected function _getUploader()
$dirConfig = DirectoryList::getDefaultConfig();
$dirAddon = $dirConfig[DirectoryList::MEDIA][DirectoryList::PATH];

if (!empty($this->_parameters[Import::FIELD_NAME_IMG_FILE_DIR])) {
$tmpPath = $this->_parameters[Import::FIELD_NAME_IMG_FILE_DIR];
} else {
$tmpPath = $dirAddon . '/' . $this->_mediaDirectory->getRelativePath('import');
}
$tmpPath = $this->getImportDir();

if (!$fileUploader->setTmpDir($tmpPath)) {
throw new LocalizedException(
Expand Down Expand Up @@ -2168,6 +2277,28 @@ protected function _saveMediaGallery(array $mediaGalleryData)
return $this;
}

/**
* Remove old media gallery items.
*
* @param array $itemsToRemove
* @return $this
*/
protected function _removeOldMediaGalleryItems(array $itemsToRemove)
{
if (empty($itemsToRemove)) {
return $this;
}

$itemsToRemove = array_merge(...$itemsToRemove);
if (empty($itemsToRemove)) {
return $this;
}

$this->mediaProcessor->removeOldMediaItems($itemsToRemove);

return $this;
}

/**
* Save product websites.
*
Expand Down Expand Up @@ -2210,6 +2341,9 @@ protected function _saveProductWebsites(array $websiteData)
* Stock item saving.
*
* @return $this
* @throws \Magento\Framework\Exception\CouldNotSaveException
* @throws \Magento\Framework\Exception\InputException
* @throws \Magento\Framework\Validation\ValidationException
*/
protected function _saveStockItem()
{
Expand Down Expand Up @@ -2791,6 +2925,7 @@ private function _customFieldsMapping($rowData)
* Validate data rows and save bunches to DB
*
* @return $this|AbstractEntity
* @throws LocalizedException
*/
protected function _saveValidatedBunches()
{
Expand Down Expand Up @@ -2930,6 +3065,7 @@ private function isNeedToChangeUrlKey(array $rowData): bool
* Get product entity link field
*
* @return string
* @throws \Exception
*/
private function getProductEntityLinkField()
{
Expand All @@ -2945,6 +3081,7 @@ private function getProductEntityLinkField()
* Get product entity identifier field
*
* @return string
* @throws \Exception
*/
private function getProductIdentifierField()
{
Expand All @@ -2961,6 +3098,7 @@ private function getProductIdentifierField()
*
* @param array $labels
* @return void
* @throws \Exception
*/
private function updateMediaGalleryLabels(array $labels)
{
Expand All @@ -2974,6 +3112,7 @@ private function updateMediaGalleryLabels(array $labels)
*
* @param array $images
* @return $this
* @throws \Exception
*/
private function updateMediaGalleryVisibility(array $images)
{
Expand Down
Loading