Skip to content

Product::addImageToMediaGallery throws Exception #6803

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.

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

Closed
R4c00n opened this issue Sep 29, 2016 · 53 comments
Closed

Product::addImageToMediaGallery throws Exception #6803

R4c00n opened this issue Sep 29, 2016 · 53 comments
Assignees
Labels
bug report Component: Catalog Fixed in 2.2.x The issue has been fixed in 2.2 release line Fixed in 2.3.x The issue has been fixed in 2.3 release line help wanted Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release

Comments

@R4c00n
Copy link

R4c00n commented Sep 29, 2016

Hi folks,
I'm trying to add some images to a product, but it's not working like expected.

Preconditions

Magento Version 2.1.1
PHP Version 7.0

Steps to reproduce

$product = $this->productFactory->create()
            ->setName($productName)
            ->setStatus($productStatus)
            ->setSku($productSku)
$product->setAttributeSetId($product->getDefaultAttributeSetId());

$product->addImageToMediaGallery($file, [
                'image',
                'small_image',
                'thumbnail',
            ], false, false);
$this->productRepository->save($product);

Expected result

Image gets added

Actual result

Exception gets thrown:

Notice: Undefined index: media_type in /var/www/magento.fissler.local/vendor/magento/module-catalog/Model/Product.php on line 2527

There's a workaround for this issue, using the Product::save method instead of the ProductRepositoryInterface::save method, but because it's a deprecated method I would like to avoid this.

@R4c00n
Copy link
Author

R4c00n commented Sep 29, 2016

I've encountered an additional bug, when using the Product::save-workaround:

function addImages() {
    $product = $this->productFactory->create()
            ->setName($productName)
            ->setStatus($productStatus)
            ->setSku($productSku)
    $product->setAttributeSetId($product->getDefaultAttributeSetId());

    $product->addImageToMediaGallery($file, [
                'image',
                'small_image',
                'thumbnail',
            ], false, false);
    $product->save()

    // Fake the behavior of ProductRepository::save
    return $this->productRepository->get($product->getSku());
}

$product = $this->addImages();
$this->productRepository->save($product);

The ProductRepository::save call removes all media gallery entries.

@sparrowek
Copy link

The same problem for me - strange is that is was working previously but it stopped after base url for media files was changed

@maciejtrybula
Copy link

The same problem here, any chance to fix it?

@eduard-kistner
Copy link

Same here, 3 months gone and no response ?!
@magento-team Please fix that, otherwise this method makes absolutely no sense if we cant use it the preferred ( new ) way.

@KrystynaKabannyk
Copy link

hello @sparrowek, @szpadyzor, @eduard-kistner, could you please clarify which versions do you use?

@maciejtrybula
Copy link

@KrystynaKabannyk version 2.1.1, PHP 7.0, like in 1st post.

@eduard-kistner
Copy link

Hi @KrystynaKabannyk,

shure:

  • Magento ver. 2.1.2
  • PHP 5.6.27-0+deb8u1 ( on Docker Dev box )

In my humble opinion the only missing part is that in
Magento\Catalog\Model\Product\Gallery\Processor -> addImage(...)
there will be no media_type set in the $mediaGalleryData array elements.

The workaround with deprecated save() ( mainly ) works for me.
So i get the images in backend but not assigned to image / thumbnail etc. ( havent debugged it yet ).

@eduard-kistner
Copy link

I did some more research as the change mentioned above does not work completely well.

It will seems to fit as no more exception will be thrown, but no images will be shown on product side then.
So there is some more ( confusing ) stuff going on when the ProductRepository saves an product.

The main Problem seems then to be that the method processMediaGallery() will not recognize entries which correspond to an file.
If the image is new it only will add entries to the product which have the content key set.
Also there seems to be lot of setting / unsetting of product media items, which maybe have there use case but i could not find them.

In the end i have created an module to fix that.
Its possible that i have not taken all cases into account ( hopefully @magento-team will do complete refactoring ) but for me it works.

@mariusjp
Copy link
Contributor

mariusjp commented Mar 1, 2017

Also ran into this problem today.
I don't want to fallback to $product->save(); so @eduard-kistner thx for your module!
I ran into a similar issue when saving categories:
http://magento.stackexchange.com/questions/161021/category-attribute-not-saving-with-repointerface

If this is not in it's correct place here, I'm sorry

@quickshiftin
Copy link
Contributor

I am unable to get this working either, having tried a variety of approaches. Here's what I have tried:

  • Using $productRepo->save($product)
  • Using $productRepo->save($product) with @eduard-kistner module
  • Using $product->save() followed by $productRepo->save($product)
  • Using $product->save() followed by $productRepo->save($product) with @eduard-kistner module

Nothing has worked. I'm on Magento 2.1.5, PHP 7.0.15

The only thing I seem to be doing differently than others is setting the $move parameter to true, but I don't think that should matter, based on the comments of Magento\Catalog\Model\Product\Gallery\Processor::addImage. I tried setting $move to false to no avail as well.

What happens for me is an image is associated with the product, but none of 'roles' are set in the admin when I view the product.
image-roles-not-set

If I set the roles manually in the admin, after a programmatic load, it seems to work. Strangely the admin doesn't seem to be using Magento\Catalog\Model\Product\Gallery\Processor::addImage, because I put a die statement at the top of the function and the save operation in the admin went through without a problem.

That said, I haven't taken the time to break out the debugger to see what kind of workaround might actually work, though I'm sure there is one lurking behind a debug session.

@eduard-kistner
Copy link

@quickshiftin
Updated to 2.1.5 and with PHP 7.0.15 / nginx but could not find an problem with setting the media attribute like
$product->addImageToMediaGallery('/path/to/image.jpg', ['image', 'small_image', 'thumbnail', 'swatch_image'], true, false);

But maybe we should not spam this ticket ;)

@quickshiftin
Copy link
Contributor

quickshiftin commented Mar 29, 2017

@eduard-kistner
I just did a fresh install of Magento 2.1.5 PHP 7.0.8 / apache. Without your extension I'm still getting the error message Notice: Undefined index: media_type. Once I install your extension, setting media attributes still does not work.

I went ahead an made a module to demonstrate the problem. Maybe there's a glaring error in there you could point out :)

@JacobDrummond
Copy link

@veloraven, this issue is present in Magento CE 2.1.6.

Do you require any more replication guidance to get this labeled as "acknowledged" and moved to an internal ticket?

@jbhamilton
Copy link

Any update on this?

@maciejtrybula
Copy link

maciejtrybula commented May 11, 2017

I am sorry all of You to hear that, but You all do it wrong.
And it is not a bug.
Just simple debbuging will let You know how it work.

First You have to create media entry:

const MEDIA_TYPE_IMAGE = 'image';

protected function createMediaImagesEntries($images)
    {
        $entries = [];

        foreach ($images as $imagePath) {
            /** @var \Magento\Catalog\Api\Data\ProductAttributeMediaGalleryEntryInterfaceFactory $mediaEntry **/
            $mediaEntry = $this->entryFactory->create();

            $mediaEntry->setMediaType(self::MEDIA_TYPE_IMAGE);

            $entries[] = $mediaEntry;
        }

        return $entries;
    }

Then You have to set it on product:

$entries = $this->createMediaImagesEntries($images);
$product->setMediaGalleryEntries($entries);

and then You have to add image to media gallery:

$product->addImageToMediaGallery($filepath, null, true, false);

@eduard-kistner
Copy link

@szpadyzor

Seriously, are you just trolling?
The mentioned code will IMO not work when i will use \Magento\Catalog\Api\ProductAttributeMediaGalleryManagementInterface for $this->entryFactory->create(); as the create method needs an product sku and an existing \Magento\Catalog\Api\Data\ProductAttributeMediaGalleryEntryInterface object.

Sp maybe you can clarify what methods exactly you are using?

@maciejtrybula
Copy link

@eduard-kistner sry Man, my bad:

should be \Magento\Catalog\Api\Data\ProductAttributeMediaGalleryEntryInterfaceFactory instead of \Magento\Catalog\Api\ProductAttributeMediaGalleryManagementInterface. I am correcting it.

@eduard-kistner
Copy link

@szpadyzor

Thanks for the clarification but your code does also throw the mentioned exception ( tested with Magento 2.1.7 ).
The Problem is and always was the addImageToMediaGallery() method and it doesn't get better if you set more entries ( which will be empty ).

I will try to give an step by step example what i mean:

  1. In your createMediaImagesEntries() method you are creating an array of empty Instances of \Magento\Catalog\Model\Product\Gallery\Entry class.
  2. You set this array via the product class setMediaGalleryEntries() method.
    This method will convert the Gallery/Entry instances to arrays which is generally fine ( if its clean is another question ) and set this multidimensional array as products media_gallery['images'].
  3. Now you are using $product->addImageToMediaGallery($filepath, null, true, false); to set the real image.
    But this will not use the data of the Gallery/Entry which you set previous, it could not do that!
    And it could not do that because the code in Magento\Catalog\Model\Product\Gallery\Processor::addImage() on line 181 will add an new array to products media_gallery['images'] data.
  4. In the end you have kind of this array set ( no Instances of any class will ever be set to the product data ):
'media_gallery' => [
    'images' => [
        0 => [
            'value_id' = null,
            'file' = null,
            'label' = null,
            'position' = null,
            'disabled' = null,
            'types' = null,
            'media_type' = 'image',
            'content' = null,
        ],
        1 => [
            'file' = 'some_path',
            'label' = '',
            'position' = 1,
            'disabled' = 0,
         ]
    ]
]
  1. Therefore the Exception mentioned in this issue gets thrown.

So in my opinion its surly generally better to use the Gallery/Entry abstractions.
But then the $product->addImageToMediaGallery() method must be changed / declared deprecated / never used anymore.

Also as this method will be the one mostly used IMO its not an solution to shout that everyone is doing it false but to fix it the way everyone uses it ( or remove that ).

All in all @magento-team should refactor this as mentioned before ;)

@wintermute-84
Copy link

anyone were able to add an image to product programmatically?

@JacobDrummond
Copy link

@Dukeofvampires,

Yes:

1

Install this: https://github.com/DIE-KAVALLERIE/magento2-product-image-fix

2

/**
 * @var \Magento\Catalog\Model\ProductRepository
 */
protected $productRepository;

/**
 * @var MagentoProduct\Gallery\Processor
 */
protected $galleryProcessor;

...

// Delete existing Magento images from DB and filesystem
$existingMedia = $product->getMediaGalleryEntries();
if(is_array($existingMedia) && count($existingMedia) > 0) {
    foreach($existingMedia as $existingMediaItem) {
        $this->galleryProcessor->removeImage($product, $existingMediaItem['file']);
    }
    // @todo: delete images without saving here. Each Magento save is computationally expensive.
    $this->productRepository->save($product);
}

// Import new images
$product->setMediaGalleryEntries([]);
$newImages = [...];
foreach($newImages as $i => $newImage) {
    $imageRoles  = $i == 0 ? ['image', 'small_image', 'thumbnail'] : [];
	$product->addImageToMediaGallery($newImage['local_path'], $imageRoles, true, false);
}
$this->productRepository->save($product);

@wintermute-84
Copy link

Thank you, this will do as a temporary solution.
But I generally don't like the idea of overriding product repository :(

Also how magento itself is saving uploaded images through admin?
I'm currently trying to debug, but with all those ajax calls it's taking longer then expected.

@JacobDrummond
Copy link

@Dukeofvampires, as with most issues on magento2, there's probably a year-old fix in develop branch that hasn't been pushed to release :(

I don't know how Magento admin gets round it. Due to time constraints, I gave up debugging and applied @eduard-kistner's patch.

@wintermute-84
Copy link

@JacobDrummond
As far as I understood, magento itself is using resourced directly. Probably this way saving is not affected by Repository bug.

That's a problem with M2 using deprecated methods in it's core.

So probably I would prefer using patch as well.

@JacobDrummond
Copy link

@Dukeofvampires, not ResourceModel->save(), but Model->save(). Which is still pretty bad, considering AbstractModel->save() is deprecated.

I see this all the time throughout the codebase: Magento devs breaking their own rules. I would guess this core stuff was written before the coding standards were finalised.

@wintermute-84
Copy link

well, yes, thats what I meant. Technically Model->save(); is a shortcut to $this->_getResource()->save($this) ;)

I'm pretty sure most of this is present because lots of the code is ported from M1.

but it's better to stop flooding the issue. :)

@danyvega1990
Copy link

@quickshiftin I need to know if you find any solution to your problem. I have the same issue, image gets added to product but no image role it's set

@maciejtrybula
Copy link

@GraysonAstral still don't know why you want to overwrite core magento mechanism. I gave you working solution.

And what does it mean that "code in my project didn't work" ? Some exception is thrown or what?

@eduard-kistner
Copy link

Don't know why @szpadyzor comes again with not working code examples ( see my comment here ) but did ya tried https://github.com/DIE-KAVALLERIE/magento2-product-image-fix ?

@maciejtrybula
Copy link

I will tell you why, because @eduard-kistner never test my implementation, you wrote:
"The mentioned code will IMO not work"
I will not show you the whole code because it is property of my client.
Did you @eduard-kistner ever debug addImage method? Did you ever try to understand this exception? Do you know why it works from admin panel?

There is said:
Notice: Undefined index: media_type in /var/www/magento.fissler.local/vendor/magento/module-catalog/Model/Product.php on line 2527

To simply debug it, just check how it is done in admin panel.

Product on adminhtml is saved by ProductRepository;
There is in ProductRepository class, method save.
In this method in line 518 there is:
$this->processMediaGallery($product, $productDataArray['media_gallery_entries']);

Do you know what it does? It creates media gallery entries. In line 478 in ProductRepository:
$this->processNewMediaGalleryEntry($product, $newEntry);
In this method media_type is set, so thats why You should do it too if you want your code work.

Show me your previous implementation, and show me where did you set up media_type property.
Probably You cant beacause you didnt do it.

You can create new media entry it is better to do it because you can set not only images but videos or other media types.

Maybe you saw that processMediaGallery check that is there any $mediaGalleryEntries set.

Debug method save from ProductRepository class and understand it.

I will tell it again: You dont have to overwrite magento Gallery Processor just understand the code.
If you are adding image to media gallery it should have set galleryEntry with media_type.

Of course saving product by $product->getResource->save($product) (yes i know this is deprecated in 2.2) dont use save from ProductRepository that's why it work without exception. But you must use ProductRepository for saving product to stick to magento rules.

Debug it @eduard-kistner and then argue with me and lever my solution.

@eduard-kistner
Copy link

@szpadyzor
Did you ever read my post or tried my module?
Don't know why we always need 50 repos and 100 workarounds ( which mostly don't work or have incomplete examples ).

The mentioned module is long time on min. one Live instance and others also use it.

From now on i will simply ignore you till you have constructive informations.

@maciejtrybula
Copy link

maciejtrybula commented Mar 19, 2018

@eduard-kistner
You can ignore my answers but i will warn others developers to do not use your module.
Do you know why? Because you overwrite magento core files. It is not acceptable to do it, and you know why @eduard-kistner ? Because if i have modules that create plugins (Do you know what it is?) after, before and around method save from ProductRepository they will stop works. I dont even want to think what if someone have modules from other vendors that do something on ProductRepository save plugins. They will have to overwrite all modules to work with your poor module.
And for clarity, You create overwrite "module", You create workaround. I just gave good practice solution without overwriting anything.

And your module? With manual creating directories? Haha! It is good - how you said "trolling". Did you ever use continous integration? Did you ever have to create project from scrath multiple times? I dont want to even think about your solution if i will have to involve 20 developers to project and told them how to "install" your module. How many this kind of modules do you have in your project? 10, 20? With manually creating directories? Copy files from repository?
Missing only:
"Next step: copy files by ftp to your production environment."

@jignesh26
Copy link

Hello Folks,

I am trying to resolve this issue with flow the way it should work its taking time more than usual. this only for update from my side if any buddy resolved this or found any way to resolve this by stranded way Lets us know here please.

@magento-engcom-team
Copy link
Contributor

magento-engcom-team commented Oct 30, 2018

Hi @progreg. Thank you for working on this issue.
Looks like this issue is already verified and confirmed. But if your want to validate it one more time, please, go though the following instruction:

  • 1. Add/Edit Component: XXXXX label(s) to the ticket, indicating the components it may be related to.

  • 2. Verify that the issue is reproducible on 2.3-develop branch

    Details- Add the comment @magento-engcom-team give me 2.3-develop instance to deploy test instance on Magento infrastructure.
    - If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.x.
    - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!

  • 3. Verify that the issue is reproducible on 2.2-develop branch.

    Details- Add the comment @magento-engcom-team give me 2.2-develop instance to deploy test instance on Magento infrastructure.
    - If the issue is reproducible on 2.2-develop branch, please add the label Reproduced on 2.2.x

  • 4. If the issue is not relevant or is not reproducible any more, feel free to close it.

@magento-engcom-team
Copy link
Contributor

Hi @R4c00n. Thank you for your report.
The issue has been fixed in #18952 by @progreg in 2.3-develop branch
Related commit(s):

The fix will be available with the upcoming 2.3.1 release.

@magento-engcom-team
Copy link
Contributor

Hi @R4c00n. Thank you for your report.
The issue has been fixed in #18951 by @progreg in 2.2-develop branch
Related commit(s):

The fix will be available with the upcoming 2.2.8 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report Component: Catalog Fixed in 2.2.x The issue has been fixed in 2.2 release line Fixed in 2.3.x The issue has been fixed in 2.3 release line help wanted Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

No branches or pull requests