dweeves / magmi-git

Magmi GitHub
364 stars 307 forks source link

media_gallery_reset not working in multi-store setup #336

Open zzrough opened 8 years ago

zzrough commented 8 years ago

Since #329 the situation regarding image in multi-store setup has greatly improved. I had all store view images positions set to 0, and now they are correctly incremented from 1 to N.

I'm using media_gallery_reset=1 because I need to have a different order from time to time for a given store view.

My media_gallery scope is global and thus:

Both seems to join on the same data and in the same way. Is this normal that they do no work on the same stores ?

I feel they should both use $this->getItemStoreIds($item, $gattrdesc["is_global"]) and I should change my media_gallery attribute scope to store.

I can provide a PR for this.

dweeves commented 8 years ago

I would be interested by a PR ! thx for your work

zzrough commented 8 years ago

After more testing, I see the media gallery attribute scope is not respected (custom backend and not the EAV one so it always goes down to the store scope, whatever the attribute scope is set to -- "global" or "store view").

Now, coming back to magmi, the actual behavior is to:

So I've got two questions for the PR:

(1) I'd be tempted do the opposite of what I suggested earlier: use getItemStoreIds (without passing $attrdesc["is_global"] to have $scope = 0) for resetGallery so it's the complement of addImageToGallery and only clears at the store level.

But it seems to me the intent of media_reset_gallery was also to delete the images (as it clears emgv and emg tables). Is this by design? What do you want the flag to be used for:

Deleting from emg seems weird to me since if you remove a value from emgv+emg, there might be leftovers for other stores that would use it (in emgv).

Also, endImport cleans up unused images in emg, which goes in the same direction: not delete from emg in resetGallery, but only clearing emgv for the specified store, and this would be the real complement of addImageToGallery.

(2) maybe you'd like a more backward compatible solution? We could keep media_gallery_reset=1 and fix it so it deletes the gallery fully (all admin+store views so no leftover entries -- if this is what you wanted from the start) and media_gallery_reset=2 to only reset the specified stores emgv entries as I suggest.

I, for one, would just go for (1) (using getItemStoreIds and only clearing the emgv table) and not (2) (adding new values for media_gallery_reset), but this is really your call after all.

dweeves commented 8 years ago

The intent for media_gallery_reset is to indeed remove images from the gallery. the idea is to enable to rebuild a whole new gallery for existing items. The afterImport is not directly linked to media_gallery_reset, it's a standard cleanup procedure , maybe does the same "job" as resetGallery would do if gallery is effectively reset. The main issue with media_gallery is that depending on the magento plugins on frontend, the base images may or may not need to be included in it (especially for pretty image display extensions in item view) The magento standard template display requires base images not to be included in gallery (otherwise would duplicate a base image in the multi image thubnails to swith image display)

It seems that magento "scope" management is indeed clumsy when it comes to images. Magmi tries to do its best to keep database as clean as possible.

Thx a lot for contributing.

Seb

2015-10-07 10:53 GMT+02:00 Stéphane Démurget notifications@github.com:

After more testing, I see the media gallery attribute scope is not respected (custom backend and not the EAV one so it always goes down to the store scope, whatever the attribute scope is set to -- "global" or "store view").

Now, coming back to magmi, the actual behavior is to:

  • respects the attribute scope in resetGallery (but it is not even respected by Magento Core in the first place)
  • respects the item stores in addImageToGallery

So I've got two questions for the PR:

(1) I'd be tempted do the opposite of what I suggested earlier: use getItemStoreIds (without passing $attrdesc["is_global"] to have $scope = 0) for resetGallery so it's the complement of addImageToGallery and only clears at the store level.

But it seems to me the intent of media_reset_gallery was also to delete the images (as it clears emgv and emg tables). Is this by design? What do you want the flag to be used for:

  • reset customisations (title, position, exclude) for the given item stores (emgv table)?
  • or also delete the image locations (from emg table)?

Deleting from emg seems weird to me since if you remove a value from emgv+emg, there might be leftovers for other stores that would use it (in emgv).

Also, endImport cleans up unused images in emg, which goes in the same direction: not delete from emg in resetGallery, but only clearing emgv for the specified store, and this would be the real complement of addImageToGallery.

(2) maybe you'd like a more backward compatible solution? We could keep media_gallery_reset=1 and fix it so it deletes the gallery fully (all admin+store views so no leftover entries -- if this is what you wanted from the start) and media_gallery_reset=2 to only reset the specified stores emgv entries as I suggest.

I, for one, would just go for (1) (using getItemStoreIds and only clearing the emgv table) and not (2) (adding new values for media_gallery_reset), but this is really your call after all.

— Reply to this email directly or view it on GitHub https://github.com/dweeves/magmi-git/issues/336#issuecomment-146121790.

bjoern-tantau commented 8 years ago

The dangerous thing with the way resetGallery is currently set up is it will remove all images from gallery first, thus also removing every label through foreign key cascades. This is problematic if some images should remain with their corresponding labels in some store which is not in the current CSV for whatever reason.

On the other hand, if resetGallery only removes the values from emgv for the current store the endImport cleanup won't "accidently" remove unwanted images, because values for them remain.

I'm still puzzling out the last bit. I'll open a PR when I'm done.

mblarsen commented 8 years ago

@zzrough @bjoern-tantau also see https://github.com/dweeves/magmi-git/pull/483 (including the comments to the PR.