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

POC multiple image types (jpg, webp, avif) #29930

Closed

Conversation

matthieu-rolland
Copy link
Contributor

@matthieu-rolland matthieu-rolland commented Oct 7, 2022

This is a proof of concept, it's not meant to be merged

How to test:

  • Install prestashop, with this PR and php 8.1

  • execute the two following SQL queries, this will simulate that the checkboxes webp and avif have been checked:

    • INSERT INTO ps_configuration (id_shop_group, id_shop, name, value, date_add, date_upd) VALUES (null, null, 'PS_ADDITIONAL_IMAGE_QUALITY_WEBP', '1', NOW(), NOW());
    • INSERT INTO ps_configuration (id_shop_group, id_shop, name, value, date_add, date_upd) VALUES (null, null, 'PS_ADDITIONAL_IMAGE_QUALITY_AVIF', '1', NOW(), NOW());
  • in the BO's image administration page, regenerate all images

  • In FO, visit a product page, and see the dump, a new sources array appears in by_size 's different formats:

Capture d’écran du 2022-10-07 15-57-34

TODO

@nicosomb, here are some informations that will help you

  • Do not use this PR as is, it's just a proof of concept, there's code that could be refactored, since I had little time I remained by legacy's logic when there's actually quite some refactoring needed

  • I just noticed that when checking Delete previous images, it only works for jpg, not for webp (and not for avif), I think it can be fixed in the AdminImagesController in the _deleteOldImages method, there's a regex that doesn't take webp and avif into account.

  • Investigate problems with AVIF:

    • We use PHP's GD extension, and this extension is only compatible with AVIF with php 8.1 and above... and also, libavif-dev package must be installed on the OS, and GD must be compiled with AVIF support, see explanations here
    • Another way would be to use ImageMagick, there's an extension for PHP, but then it means adding a new mandatory dependency for prestashop, I don't know if we can do that in a minor prestashop version? That would need to be discussed with @MatShir
  • Adding the checkboxes

    • You can add the checkboxes for additional image type support in AdminImagesController's constructor, see how the form is built using the legacy HelperForm component, it is documented here
    • New fields's value will be stored in the PS_CONFIGURATION table, here are the keys I used: PS_ADDITIONAL_IMAGE_QUALITY_WEBP and PS_ADDITIONAL_IMAGE_QUALITY_AVIF, feel free to change this if you want 👍

@prestonBot prestonBot added Bug fix Type: Bug fix BC break Type: Introduces a backwards-incompatible break labels Oct 7, 2022
@matthieu-rolland matthieu-rolland changed the title init POC multiple image types (jpg, webp, avif) Oct 7, 2022
@matthieu-rolland matthieu-rolland removed Bug fix Type: Bug fix BC break Type: Introduces a backwards-incompatible break labels Oct 7, 2022
@PrestaShop PrestaShop deleted a comment from prestonBot Oct 7, 2022
Copy link
Contributor

@Amoifr Amoifr left a comment

Choose a reason for hiding this comment

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

Just a comment as an "hello" from outside !

@@ -92,7 +92,7 @@ public function getAllProductImages(array $product, Language $language)
$productInstance,
$image['id_image']
), $image);

dump($image);
Copy link
Contributor

Choose a reason for hiding this comment

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

A good dump is always better than long discuss ;-)

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 this pull request may close these issues.

3 participants