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

Add all images of a product to the sitemap, instead of only the cover image #107

Merged

Conversation

@Pixep
Copy link

commented Feb 10, 2019

This change adds an <image:image> field for each image of a product. All images still have the same label.
Having multiple <image:image> for a single node in a sitemap is authorized by the schema version 0.9.

@Pixep Pixep referenced this pull request Feb 10, 2019
@MathiasReker

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2019

@Pixep

This comment has been minimized.

Copy link
Author

commented Feb 11, 2019

I just noticed that the sitemap uses the product name and description, but not the "legend" that can be defined per-image on products, in the BO.

                        'title_img' => htmlspecialchars(strip_tags($product->name)),
                        'caption' => htmlspecialchars(strip_tags($product->meta_description)),

I guess my next change will be to actually expose that legend if defined, instead of the product's info, that would make more sense. But that's another story.

EDIT: Created issue #108

@Pixep

This comment has been minimized.

Copy link
Author

commented Mar 2, 2019

Is any reviewer available?

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2019

Sorry for the delay 🙄

@marionf

This comment has been minimized.

Copy link

commented Mar 5, 2019

Hello @Pixep

I have this notice when I click on the cronjob link

capture d ecran_1118

@Pixep

This comment has been minimized.

Copy link
Author

commented Mar 5, 2019

Hello @Pixep

I have this notice when I click on the cronjob link

Hi @marionf, thanks for the feedback.
I will jump on that. On what branch did you have that issue exactly?

@marionf

This comment has been minimized.

Copy link

commented Mar 6, 2019

Thank you, it was on the branch of your PR

@Pixep

This comment has been minimized.

Copy link
Author

commented Mar 7, 2019

Thank you, it was on the branch of your PR

I meant was it Prestashop v1.7.5.1, latest develop or another? I couldn't repro on 1.7.5.0 and 1.7.5.1 yet, but I will retry with a wider range of versions/configurations.

@marionf

This comment has been minimized.

Copy link

commented Mar 7, 2019

It was on the latest develop

Add all images of a product to the sitemap, instead of only the cover…
… image

This change adds an <image> field for each image of a product. All images still have the same label.

@Pixep Pixep force-pushed the Pixep:feature/multiple-images-for-products branch from 2f8129c to ddea096 Mar 10, 2019

@Pixep

This comment has been minimized.

Copy link
Author

commented Mar 10, 2019

@marionf : I still wasn't able to reproduce the issue, but I made an attempted fix. If you have more details regarding the exact commit and environment (or docker image), I will give it a try again.

I amended my commit to strictly test for image/images indexes, L.672, which seemed to match the exception captured and should prevent related issues even in strict environments.

@Pixep

This comment has been minimized.

Copy link
Author

commented Mar 13, 2019

@PierreRambaud : The code is ready for review when you are! See comment above. The labels can need to be changed to Waiting for approval. Thanks :)

gsitemap.php Outdated Show resolved Hide resolved
@PierreRambaud

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

@Pixep Thanks, I add a little comment :)

@Pixep

This comment has been minimized.

Copy link
Author

commented Mar 22, 2019

@PierreRambaud Fixed! :)

@PierreRambaud PierreRambaud merged commit dd810e8 into PrestaShop:dev Jul 7, 2019

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2019

Thanks @Pixep

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.