magento/magento2#4711: Improve error reporting for products images import[forwardport].

Author: nmalevanec

app/code/Magento/CatalogImportExport/Model/Import/Product.php

@@ -2047,6 +2047,7 @@ protected function uploadMediaFiles($fileName, $renameFileOff = false)
             $res = $this->_getUploader()->move($fileName, $renameFileOff);
             return $res['file'];
         } catch (\Exception $e) {
+            $this->_logger->critical($e);
             return '';
         }
     }

app/code/Magento/CatalogImportExport/Test/Unit/Model/Import/ProductTest.php

@@ -1194,6 +1194,65 @@ public function testParseAttributesWithWrappedValuesWillReturnsLowercasedAttribu
         $this->assertArrayNotHasKey('PARAM2', $attributes);
     }
 
+    /**
+     * Test that errors occurred during importing images are logged.
+     *
+     * @param string $fileName
+     * @param bool $throwException
+     * @dataProvider uploadMediaFilesDataProvider
+     */
+    public function testUploadMediaFiles(string $fileName, bool $throwException)
+    {
+        $exception = new \Exception();
+        $expectedFileName = $fileName;
+        if ($throwException) {
+            $expectedFileName = '';
+            $this->_logger->expects($this->once())->method('critical')->with($exception);
+        }
+        $fileUploaderMock = $this
+            ->getMockBuilder(\Magento\CatalogImportExport\Model\Import\Uploader::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $fileUploaderMock
+            ->expects($this->once())
+            ->method('move')
+            ->willReturnCallback(
+                function ($name) use ($throwException, $exception) {
+                    if ($throwException) {
+                        throw $exception;
+                    }
+                    return ['file' => $name];
+                }
+            );
+        $this->setPropertyValue(
+            $this->importProduct,
+            '_fileUploader',
+            $fileUploaderMock
+        );
+        $actualFileName = $this->invokeMethod(
+            $this->importProduct,
+            'uploadMediaFiles',
+            [$fileName]
+        );
+        $this->assertEquals(
+            $expectedFileName,
+            $actualFileName
+        );
+    }
+
+    /**
+     * Data provider for testUploadMediaFiles.
+     *
+     * @return array
+     */
+    public function uploadMediaFilesDataProvider()
+    {
+        return [
+            ['test1.jpg', false],
+            ['test2.jpg', true],
+        ];
+    }
+
     public function getImagesFromRowDataProvider()
     {
         return [

dev/tests/integration/testsuite/Magento/CatalogImportExport/Model/Import/ProductTest.php

@@ -24,7 +24,7 @@
 use Magento\Framework\Filesystem;
 use Magento\ImportExport\Model\Import;
 use Magento\Store\Model\Store;
-use Magento\UrlRewrite\Model\UrlRewrite;
+use Psr\Log\LoggerInterface;
 
 /**
  * Class ProductTest
@@ -60,11 +60,20 @@ class ProductTest extends \Magento\TestFramework\Indexer\TestCase
      */
     protected $objectManager;
 
+    /**
+     * @var LoggerInterface|\PHPUnit_Framework_MockObject_MockObject
+     */
+    private $logger;
+
     protected function setUp()
     {
         $this->objectManager = \Magento\TestFramework\Helper\Bootstrap::getObjectManager();
+        $this->logger = $this->getMockBuilder(LoggerInterface::class)
+            ->disableOriginalConstructor()
+            ->getMock();
         $this->_model = \Magento\TestFramework\Helper\Bootstrap::getObjectManager()->create(
-            \Magento\CatalogImportExport\Model\Import\Product::class
+            \Magento\CatalogImportExport\Model\Import\Product::class,
+            ['logger' => $this->logger]
         );
         parent::setUp();
     }
@@ -657,6 +666,21 @@ public function testSaveMediaImage()
         $this->assertEquals('Additional Image Label Two', $additionalImageTwoItem->getLabel());
     }
 
+    /**
+     * Test that errors occurred during importing images are logged.
+     *
+     * @magentoDataIsolation enabled
+     * @magentoAppIsolation enabled
+     * @magentoDataFixture mediaImportImageFixture
+     * @magentoDataFixture mediaImportImageFixtureError
+     * @SuppressWarnings(PHPMD.ExcessiveMethodLength)
+     */
+    public function testSaveMediaImageError()
+    {
+        $this->logger->expects(self::once())->method('critical');
+        $this->importDataForMediaTest('import_media.csv', 1);
+    }
+
     /**
      * Copy fixture images into media import directory
      */
@@ -715,6 +739,30 @@ public static function mediaImportImageFixtureRollback()
         $mediaDirectory->delete('catalog');
     }
 
+    /**
+     * Copy incorrect fixture image into media import directory.
+     */
+    public static function mediaImportImageFixtureError()
+    {
+        /** @var \Magento\Framework\Filesystem\Directory\Write $mediaDirectory */
+        $mediaDirectory = \Magento\TestFramework\Helper\Bootstrap::getObjectManager()->get(
+            \Magento\Framework\Filesystem::class
+        )->getDirectoryWrite(
+            DirectoryList::MEDIA
+        );
+        $dirPath = $mediaDirectory->getAbsolutePath('import');
+        $items = [
+            [
+                'source' => __DIR__ . '/_files/magento_additional_image_error.jpg',
+                'dest' => $dirPath . '/magento_additional_image_two.jpg',
+            ],
+        ];
+        foreach ($items as $item) {
+            copy($item['source'], $item['dest']);
+        }
+    }
+
+
     /**
      * Export CSV string to array
      *
@@ -1563,11 +1611,13 @@ public function testProductWithWrappedAdditionalAttributes()
     }
 
     /**
-     * Import and check data from file
+     * Import and check data from file.
      *
      * @param string $fileName
+     * @param int $expectedErrors
+     * @return void
      */
-    private function importDataForMediaTest($fileName)
+    private function importDataForMediaTest(string $fileName, int $expectedErrors = 0)
     {
         $filesystem = $this->objectManager->create(\Magento\Framework\Filesystem::class);
         $directory = $filesystem->getDirectoryWrite(DirectoryList::ROOT);
@@ -1605,7 +1655,7 @@ private function importDataForMediaTest($fileName)
         $this->assertTrue($errors->getErrorsCount() == 0);
 
         $this->_model->importData();
-        $this->assertTrue($this->_model->getErrorAggregator()->getErrorsCount() == 0);
+        $this->assertTrue($this->_model->getErrorAggregator()->getErrorsCount() == $expectedErrors);
     }
 
     /**