Skip to content

Commit 6744fdb

Browse files
bubasumabubasuma
committed
MC-38890: Product Images are not updated correctly via API when files from pub/media/ do not exist but the records do exist in gallery media tables
- Fix images are not updated if existing images are not in pub/media/catalog/ and new images names match the existing images names
1 parent f472536 commit 6744fdb

File tree

3 files changed

+114
-15
lines changed

3 files changed

+114
-15
lines changed

‎app/code/Magento/Catalog/Model/Product/Gallery/UpdateHandler.php

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -77,26 +77,26 @@ protected function processDeletedImages($product, array &$images)
7777
{
7878
$filesToDelete = [];
7979
$recordsToDelete = [];
80-
$picturesInOtherStores = [];
8180
$imagesToDelete = [];
82-
83-
foreach ($this->resourceModel->getProductImages($product, $this->extractStoreIds($product)) as $image) {
84-
$picturesInOtherStores[$image['filepath']] = true;
81+
$imagesToNotDelete = [];
82+
foreach ($images as $image) {
83+
if (empty($image['removed'])) {
84+
$imagesToNotDelete[] = $image['file'];
85+
}
8586
}
8687

87-
foreach ($images as &$image) {
88+
foreach ($images as $image) {
8889
if (!empty($image['removed'])) {
8990
if (!empty($image['value_id'])) {
9091
if (preg_match('/\.\.(\\\|\/)/', $image['file'])) {
9192
continue;
9293
}
9394
$recordsToDelete[] = $image['value_id'];
94-
$imagesToDelete[] = $image['file'];
95-
$catalogPath = $this->mediaConfig->getBaseMediaPath();
96-
$isFile = $this->mediaDirectory->isFile($catalogPath . $image['file']);
97-
// only delete physical files if they are not used by any other products and if this file exist
98-
if ($isFile && !($this->resourceModel->countImageUses($image['file']) > 1)) {
99-
$filesToDelete[] = ltrim($image['file'], '/');
95+
if (!in_array($image['file'], $imagesToNotDelete)) {
96+
$imagesToDelete[] = $image['file'];
97+
if ($this->canDeleteImage($image['file'])) {
98+
$filesToDelete[] = ltrim($image['file'], '/');
99+
}
100100
}
101101
}
102102
}
@@ -107,6 +107,19 @@ protected function processDeletedImages($product, array &$images)
107107
$this->removeDeletedImages($filesToDelete);
108108
}
109109

110+
/**
111+
* Check if image exists and is not used by any other products
112+
*
113+
* @param string $file
114+
* @return bool
115+
*/
116+
private function canDeleteImage(string $file): bool
117+
{
118+
$catalogPath = $this->mediaConfig->getBaseMediaPath();
119+
return $this->mediaDirectory->isFile($catalogPath . $file)
120+
&& $this->resourceModel->countImageUses($file) <= 1;
121+
}
122+
110123
/**
111124
* @inheritdoc
112125
*

‎dev/tests/integration/testsuite/Magento/Catalog/Model/Product/Gallery/UpdateHandlerTest.php

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -507,6 +507,92 @@ public function testDeleteWithMultiWebsites(): void
507507
$this->assertArrayNotHasKey($secondStoreId, $imageRolesPerStore);
508508
}
509509

510+
/**
511+
* Check that product images should be updated successfully regardless if the existing images exist or not
512+
*
513+
* @magentoDataFixture Magento/Catalog/_files/product_with_image.php
514+
* @dataProvider updateImageDataProvider
515+
* @param string $newFile
516+
* @param string $expectedFile
517+
* @param bool $exist
518+
* @return void
519+
*/
520+
public function testUpdateImage(string $newFile, string $expectedFile, bool $exist): void
521+
{
522+
$product = $this->getProduct(Store::DEFAULT_STORE_ID);
523+
$images = $product->getData('media_gallery')['images'];
524+
$this->assertCount(1, $images);
525+
$oldImage = reset($images) ?: [];
526+
$this->assertEquals($oldImage['file'], $product->getImage());
527+
$this->assertEquals($oldImage['file'], $product->getSmallImage());
528+
$this->assertEquals($oldImage['file'], $product->getThumbnail());
529+
$path = $this->mediaDirectory->getAbsolutePath($this->config->getBaseMediaPath() . $oldImage['file']);
530+
$tmpPath = $this->mediaDirectory->getAbsolutePath($this->config->getBaseTmpMediaPath() . $oldImage['file']);
531+
$this->assertFileExists($path);
532+
$this->mediaDirectory->getDriver()->copy($path, $tmpPath);
533+
if (!$exist) {
534+
$this->mediaDirectory->getDriver()->deleteFile($path);
535+
$this->assertFileDoesNotExist($path);
536+
}
537+
// delete old image
538+
$oldImage['removed'] = 1;
539+
$newImage = [
540+
'file' => $newFile,
541+
'position' => 1,
542+
'label' => 'New Image Alt Text',
543+
'disabled' => 0,
544+
'media_type' => 'image'
545+
];
546+
$newImageRoles = [
547+
'image' => $newFile,
548+
'small_image' => 'no_selection',
549+
'thumbnail' => 'no_selection',
550+
];
551+
$product->setData('media_gallery', ['images' => [$oldImage, $newImage]]);
552+
$product->addData($newImageRoles);
553+
$this->updateHandler->execute($product);
554+
$product = $this->getProduct(Store::DEFAULT_STORE_ID);
555+
$images = $product->getData('media_gallery')['images'];
556+
$this->assertCount(1, $images);
557+
$image = reset($images) ?: [];
558+
$this->assertEquals($newImage['label'], $image['label']);
559+
$this->assertEquals($expectedFile, $product->getImage());
560+
$this->assertEquals($newImageRoles['small_image'], $product->getSmallImage());
561+
$this->assertEquals($newImageRoles['thumbnail'], $product->getThumbnail());
562+
$path = $this->mediaDirectory->getAbsolutePath($this->config->getBaseMediaPath() . $product->getImage());
563+
// Assert that the image exists on disk.
564+
$this->assertFileExists($path);
565+
}
566+
567+
/**
568+
* @return array[]
569+
*/
570+
public function updateImageDataProvider(): array
571+
{
572+
return [
573+
[
574+
'/m/a/magento_image.jpg',
575+
'/m/a/magento_image_1.jpg',
576+
true
577+
],
578+
[
579+
'/m/a/magento_image.jpg',
580+
'/m/a/magento_image.jpg',
581+
false
582+
],
583+
[
584+
'/m/a/magento_small_image.jpg',
585+
'/m/a/magento_small_image.jpg',
586+
true
587+
],
588+
[
589+
'/m/a/magento_small_image.jpg',
590+
'/m/a/magento_small_image.jpg',
591+
false
592+
]
593+
];
594+
}
595+
510596
/**
511597
* Check product image link and product image exist
512598
*

‎dev/tests/integration/testsuite/Magento/Catalog/Model/Product/UpdateProductWebsiteTest.php

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
use Magento\Catalog\Api\Data\ProductInterface;
1111
use Magento\Catalog\Api\ProductRepositoryInterface;
1212
use Magento\Catalog\Model\ResourceModel\Product\Website\Link;
13-
use Magento\Framework\Exception\NoSuchEntityException;
13+
use Magento\Framework\Exception\CouldNotSaveException;
1414
use Magento\Framework\ObjectManagerInterface;
1515
use Magento\Store\Api\WebsiteRepositoryInterface;
1616
use Magento\TestFramework\Helper\Bootstrap;
@@ -82,10 +82,10 @@ public function testUnassignProductFromWebsite(): void
8282
*/
8383
public function testAssignNonExistingWebsite(): void
8484
{
85-
$messageFormat = 'The website with id %s that was requested wasn\'t found. Verify the website and try again.';
85+
$messageFormat = 'The product was unable to be saved. Please try again.';
8686
$nonExistingWebsiteId = 921564;
87-
$this->expectException(NoSuchEntityException::class);
88-
$this->expectExceptionMessage((string)__(sprintf($messageFormat, $nonExistingWebsiteId)));
87+
$this->expectException(CouldNotSaveException::class);
88+
$this->expectExceptionMessage((string)__($messageFormat));
8989
$this->updateProductWebsites('simple2', [$nonExistingWebsiteId]);
9090
}
9191

0 commit comments

Comments
 (0)