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

Remove function "GSITEMAP_CHECK_IMAGE_FILE" - too problematic #169

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

ShaiMagal
Copy link
Contributor

@ShaiMagal ShaiMagal commented Nov 16, 2023

Questions Answers
Description? Function GSITEMAP_CHECK_IMAGE_FILE in this module is crazy thing, and "every second" customer enable it without knowing impact, for example on performance (there is EVERY image "opened" via get_headers). If eshop have for example 100 000 images, every single images is analyzes, so, in most of cases error 500. But maybe worse is problem, they enable this, and in bigger eshops or slow webhosting it doesn't run to end, so sitemap is outdated years (I saw this about 5 times already). We should remove this. It's useless function.
Type? refacto
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #PrestaShop/PrestaShop#34572 + #PrestaShop/PrestaShop#32625 + #PrestaShop/PrestaShop#18508
How to test? Regenerate sitemap, check it went OK

@ShaiMagal ShaiMagal changed the title Remove function "GSITEMAP_CHECK_IMAGE_FILE" Remove function "GSITEMAP_CHECK_IMAGE_FILE" - too problematic Nov 16, 2023
Copy link
Contributor

@Hlavtox Hlavtox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to change the version in config.xml, but otherwise it's OK for me. ;-)

@ShaiMagal
Copy link
Contributor Author

@Hlavtox Ahhh, config.xml edit done :)

Copy link

@matthieu-rolland matthieu-rolland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like csfixer is blocking the PR

To fix this you can run this command from the root of your project and then commit and push the modifications:

php vendor/bin/php-cs-fixer fix

Copy link
Contributor

@Hlavtox Hlavtox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code style fix is needed, please avoid autoformatting in the IDE, because it's using a different style apparently.

Also, it creates extra code changes and making the PR harder to review. 👍

@ShaiMagal ShaiMagal marked this pull request as draft November 20, 2023 15:03
@ShaiMagal ShaiMagal marked this pull request as ready for review November 23, 2023 12:25
@ShaiMagal
Copy link
Contributor Author

@Hlavtox I rewrite changes manually from scratch.

@matthieu-rolland I don't have project for it (small changes).
Is there any other option how to fix it manully? I don't know, what to fix to be honest :) I see it's test "PHP tests / PHP-CS-Fixer (pull_request)" - but I don't know, what to change.

@Hlavtox Hlavtox added this to the 4.3.1 milestone Nov 25, 2023
Copy link
Contributor

@Hlavtox Hlavtox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad upgrade file name and method name. ;-)

@ShaiMagal
Copy link
Contributor Author

@Hlavtox fixed. Tests green now.

@Hlavtox
Copy link
Contributor

Hlavtox commented Nov 25, 2023

Nice, now, we just need to squash those commits so there is only 1. ;-)

@Hlavtox
Copy link
Contributor

Hlavtox commented Nov 29, 2023

Verified the behavior, everything works correctly, including the upgrade. :-)

Minor issues found when testing this - fixed here - PrestaShop/PrestaShop#34731.

@Hlavtox Hlavtox merged commit 882ebff into PrestaShop:dev Nov 29, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
4 participants