Will replace images during CSV import

[raivisdejus]

Description

Built on top of #21146

This PR adds image deletion so after import product will have images specified in the CSV. New images will be added, existing will not be duplicated and images not mentioned in the CSV will be removed

Fixed Issues

#14398: Images Not Replaced via CSV Import

Manual testing scenarios (*)

1. Import product CSV using Add/Update method multiple times
2. Check if image duplication happens or not

There's many testing scenario's actually. Some of them being:

• Use HTTP references in images
• Same filename different file
• Different filename same file
• Same file path for multiple products
• Change image labels/titles, ensure they're updated
• Manipulate thumbnail, small image and base image tags, and ensure it's updated properly after import
• etc.

[magento-engcom-team]

Hi @raivisdejus. 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-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[markalston]

Thanks! Been bashing my head against these image duplicates for a while. Hopefully you can get this accepted.

[larsroettig]

Hi @raivisdejus,
you must cover this with integration tests also the current tests are broken by this change see:

https://travis-ci.org/magento/magento2/builds/508902746?utm_source=github_status&utm_medium=notification

Also, there is to lang value name at
https://app.codacy.com/app/Bartlomiejsz/magento2/pullRequest?prid=3299387

[larsroettig]

Hi @raivisdejus,
you must cover this with integration tests also the current tests are broken by this change see:

[larsroettig]

Hi @raivisdejus, there is not coverage by an extra test case pls add a test

[raivisdejus]

@larsroettig I have added a test case to test the update and removal of old media items. Also adjusted some of the existing tests. Please let me know if the tests need to be more explicit or cover something else. Any other comments are also welcome.

[sidolov]

Hi @raivisdejus , the tests are failing, looks like failures caused by provided implementation. Could you please take a look?

[raivisdejus]

Current Travis test is failing with error exec(): Unable to fork [tasklist.exe /fi 'PID eq 8731' /fo CSV /nh 2>&1 at this function Magento\Catalog\Model\Product\Type\PriceWithDimensionTest::testGetPriceFromIndexer()

It seems it could be test environment, config or resources related. At some free moment I will debug it further, but maybe you @sidolov and @larsroettig have some ideas on what could be wrong or who to ask for assistance.

[galanis-a]

Hello, has anyone created a patch with this pr? I tried to use it with composer patch and it fails in multiple places of the patch. Tried it on Magento 2.3.1

Thanks

[raivisdejus]

@galanis-a My solution for this problem in a real life project was to override the Magento\CatalogImportExport\Model\Import\Product in a custom module with di.xml.

Patch was not tested but it might fail as the patch is formed against the upstream file in 2.3-develop branch and that might be different from the upstream file in 2.3.1. So to use the patch approach I recommend porting changes in the patch to the file you have and then creating the patch or using the file as override.

[galanis-a]

Thanks @raivisdejus , will try it!

[densen45]

I have created a custom module that overrides the Magento\CatalogImportExport\Model\Import\Product and the Magento\CatalogImportExport\Model\Import\Product\MediaGalleryProcessor for Magento 2.3.2 through preferences in di.xml.
The issue seems to be solved.

If you want to do the same, please note the following traps:

• You can't just use the code of the two files referenced in this PR because at least Product has changed in 2.3.2. Instead you have to manually apply the changes to the files that override them in your custom module.
• My usual workflow for overriding entire classes with a preference is as follows: copy the original file to my new custom module, adapt the namespace, let the copied class extend the original file, if necessary update parent::__construct(...) calls, create the preference entry in the di.xml, check if everything still works, do my custom code modifications, remove all the code I don't rely on (everything that is not declared as private) and which I did not modified. The Magento\CatalogImportExport\Model\Import\Product uses not-fully qualified name references to e.g. the \Magento\CatalogImportExport\Model\Import\Product\StoreResolver in its __construct function. Make sure to use fully qualified names in your custom module, otherwise the classes cannot be found.
• When testing if it is still (after applying this fix) possible to add new product images through the import, please don't just rename an existing image of the product and wonder why it is not added to the product. This fix relies on hashes of the images to decide equality. Thus, you have to provide a second image.

[slavvka]

Hey @raivisdejus. Are you going to continue working on this PR?

[raivisdejus]

@slavvka If someone else can finish this, I would be only happy.

Last time I tried to resolve issues in tests I was getting error in unrelated places that I do not know immediate solution for.
Travis test was failing with error exec(): Unable to fork [tasklist.exe /fi 'PID eq 8731' /fo CSV /nh 2>&1 at this function Magento\Catalog\Model\Product\Type\PriceWithDimensionTest::testGetPriceFromIndexer()

I would like to see this finished, so some helping pointers to the right direction would be welcome

[raivisdejus]

@slavvka Also some hints on semantic version test failing would be welcome. Can I assume version bump? or should the failing code be fixed?

[slavvka]

@magento run all tests

[slavvka]

Hey @raivisdejus That time we had issues with Travis tests so you encountered the mentioned error. Now we started to use our internal test builds so they should show correct results. Please merge the latest mainline code and the tests will trigger automatically. After that we can see what issues you need to fix.

[densen45]

Your code only removes the images of the last product because the $galleryItemsToRemove variable is reassigned for each product.

I propose the use of array_merge as follows:

$galleryItemsToRemove = \array_merge($galleryItemsToRemove, \array_diff(\array_keys($existingImages[$rowSku]), $uploadedFiles));

[sidolov]

@raivisdejus , I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

[m2-assistant]

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

[skapin]

Did you even publish a fix for the duplicate solution in your last magento released ?
I still have the exact same trouble like a lot of people, and you close a working PR without watching the importance of this. Can you fix the trouble at Magento or no ?