Skip to content
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

media_gallery_reset not working in multi-store setup #336

Open
zzrough opened this issue Oct 2, 2015 · 5 comments · May be fixed by #463
Open

media_gallery_reset not working in multi-store setup #336

zzrough opened this issue Oct 2, 2015 · 5 comments · May be fixed by #463

Comments

@zzrough
Copy link

zzrough commented Oct 2, 2015

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:

  • resetGallery is called with the admin store (respects the attribute scope with getItemStoreIds)
  • addImageToGallery is called with the item store views (does not respect the attribute scope but uses the item store views directly using getStoreIdsForStoreScope)

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
Copy link
Owner

dweeves commented Oct 2, 2015

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

@zzrough
Copy link
Author

zzrough commented Oct 7, 2015

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.

@dweeves
Copy link
Owner

dweeves commented Oct 7, 2015

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
#336 (comment).

@bjoern-tantau
Copy link

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.

bjoern-tantau added a commit to bjoern-tantau/magmi-git that referenced this issue Jul 13, 2016
@mblarsen
Copy link
Contributor

mblarsen commented Sep 2, 2016

@zzrough @bjoern-tantau also see #483 (including the comments to the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants