Skip to content

Use new image feature - #1087

Open
Hlavtox wants to merge 1 commit into
PrestaShop:2.xfrom
Hlavtox:use-new-image-feature
Open

Use new image feature#1087
Hlavtox wants to merge 1 commit into
PrestaShop:2.xfrom
Hlavtox:use-new-image-feature

Conversation

@Hlavtox

@Hlavtox Hlavtox commented Jul 29, 2026

Copy link
Copy Markdown
Contributor
Questions Answers
Description? Puts the new PrestaShop/PrestaShop#41977 feature to use. Adds new image dimension for manufacturers and uses it on the product page.
Type? new feature
BC breaks? no
Deprecations? no
Fixed ticket?
Sponsor company TRENDO s.r.o.
How to test? See that the new image doesn't have whitespace anymore.

@github-project-automation github-project-automation Bot moved this to Ready for review in PR Dashboard Jul 29, 2026
SharakPL
SharakPL previously approved these changes Jul 29, 2026
@ps-jarvis ps-jarvis added the Waiting for QA Status: Action required, Waiting for test feedback label Jul 29, 2026
@ps-jarvis ps-jarvis moved this from Ready for review to To be tested in PR Dashboard Jul 29, 2026
@Hlavtox

Hlavtox commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

@Codencode @kpodemski @tblivet Guys, one more thing we could improve in the image fitment feature.

Right now, width and height of the image type are passed into the FO, but in case of images set to "bound", they will not contain right values. I think a good solution would be to @getimagesize of the real thumbnail. WDYT?

@SharakPL

SharakPL commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

The change is required but I suspect this change would cause a performance drop on long product lists. Isn't width="auto" height="auto" enough for bound thumbs?

To prevent layout shifts we could add aspect-ratio to parent link based on the dimensions from image settings.

For singular images like the manufacturer on the product page @getimagesize should be ok.

@Codencode

Copy link
Copy Markdown
Member

I would avoid using getimagesize() at rendering time, as it would introduce additional filesystem reads for every image and could become a performance issue, especially on product listings.

Using width: auto and height: auto could be an option, but only as CSS properties. The HTML width and height attributes are expected to contain numeric values. However, this would prevent image distortion without necessarily solving layout shifts, since the browser would still not know the actual image dimensions in advance.

Ideally, the actual generated dimensions should be stored when the thumbnail is created. They could then be exposed by the image presenter without having to inspect each file again on every request.

Codencode
Codencode previously approved these changes Jul 29, 2026
@SharakPL

Copy link
Copy Markdown
Contributor

I agree. Storing image dimensions in DB would be the best option.

BTW. a reminder to also consider a fix for #34665 while on this subject.

@Hlavtox

Hlavtox commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

@Codencode I tested it once and the dimensions retrieval take like 0.00002 ms or something. I will do some measurements.

I agree. Storing image dimensions in DB would be the best option.

You would need to store dimensions of all thumbnails, it would insanely complicated and slower than getting the image header.

@Codencode

Codencode commented Jul 29, 2026

Copy link
Copy Markdown
Member

@Hlavtox

We could consider retrieving the dimensions of just one image (for example, the original one) and then calculating the dimensions of all the other images from it. I think this approach would be preferable to retrieving the dimensions of every single image.

Another option would be to store the dimensions of only one image and derive the others through calculation. This shouldn't be an overly complex change: we would only need to store width and height by adding two fields to the image table.

What do you think?

@Hlavtox

Hlavtox commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

So, I did some measurements.

Test 1

$images = Db::getInstance()->executeS('
  SELECT id_image
  FROM `' . _DB_PREFIX_ . 'image`
  LIMIT 1000
');

foreach ($images as $row) {
	$imagePath = implode(DIRECTORY_SEPARATOR, [
    rtrim(_PS_PRODUCT_IMG_DIR_, DIRECTORY_SEPARATOR),
    rtrim(Image::getImgFolderStatic((int) $row['id_image']), DIRECTORY_SEPARATOR),
  ]) . (int) $row['id_image'] . '-large_default.jpg';


  echo (int) $row['id_image'] . ' => ' . $imagePath . "<br/>";
}

1.162 ms

Test 2

$images = Db::getInstance()->executeS('
  SELECT id_image
  FROM `' . _DB_PREFIX_ . 'image`
  LIMIT 1000
');

foreach ($images as $row) {
	$imagePath = implode(DIRECTORY_SEPARATOR, [
    rtrim(_PS_PRODUCT_IMG_DIR_, DIRECTORY_SEPARATOR),
    rtrim(Image::getImgFolderStatic((int) $row['id_image']), DIRECTORY_SEPARATOR),
  ]) . "/" . (int) $row['id_image'] . '-medium_default.jpg';

	$size = @getimagesize($imagePath);
	if ($size !== false) {
		echo (int) $row['id_image'] . ' => ' . $size[0] . ' x ' . $size[1] . ' px' . "<br/>";
	} else {
		echo (int) $row['id_image'] . ' => Cannot load dimensions.' . "<br/>";
	}
}

85.508 ms

So, it takes 0,085 ms to get dimensions of one image.

@Codencode

Copy link
Copy Markdown
Member

The benchmark is reassuring, and getimagesize() may be perfectly acceptable in many cases.

My only concern is that filesystem access can behave differently depending on the environment, for example with virtual machines, Docker volumes, network storage, or slower hosting.

This could matter mainly for product images, since a single page may render many of them through product listings, homepage carousels, related products, or product-page recommendations.

For this reason, I was considering storing only the original product image dimensions in the image table. The dimensions of bound thumbnails could then be calculated from the original size and the image type configuration, without storing every generated size or repeatedly accessing the filesystem.

@tblivet tblivet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi 👋 sorry, I blocked the PR just to avoid an accidental merge before a few questions
get answered 👍 But it's nice to see the new fitment being used 🙂

Small question about removing width / height on the <img>: would it be possible
to keep them?

Without them the browser can't reserve space for the image, so it shifts the layout
when it loads (CLS). It's not really an issue in this specific case, but if bound
ends up being widely used across the theme, systematically dropping the dimensions
would become a habit worth avoiding.

For that to work, i believe both real values would need to be populated by the core with the actual generated dimensions, since today bySize returns the ones configured in the theme (98×98) rather than those of the generated file. WDYT ?

@Hlavtox

Hlavtox commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

@tblivet Exactly, if I use the width and height provided by the image type, I am not getting the real image dimensions.

Since the usage will be very low by default, I think we can start safely by adapting ImageRetriever to get the dimensions with @getimagesize, will it be OK for 9.2?

@tblivet

tblivet commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Well thank you @Hlavtox 🙂
So from the theme side, all we need is bySize returning the real dimensions.

@kpodemski maybe you can confirm whether it's OK to add this to the 9.2 scope?

tblivet
tblivet previously approved these changes Jul 30, 2026
@Hlavtox
Hlavtox dismissed stale reviews from tblivet, Codencode, and SharakPL via 5c48eb3 August 11, 2026 21:46
@Hlavtox
Hlavtox force-pushed the use-new-image-feature branch from 565eca9 to 5c48eb3 Compare August 11, 2026 21:46
@Hlavtox

Hlavtox commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

@tblivet The real dimensions for bound thumbnails are ready in PrestaShop/PrestaShop#42349. I restored the width and height attributes here so the theme can use them. 🚀

@Hlavtox
Hlavtox requested review from Codencode and SharakPL August 11, 2026 21:46
@Hlavtox
Hlavtox requested review from SharakPL and tblivet and removed request for SharakPL August 11, 2026 21:46

@tblivet tblivet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perfect 👌 Thank you @Hlavtox !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Waiting for QA Status: Action required, Waiting for test feedback

Projects

Status: To be tested

Development

Successfully merging this pull request may close these issues.

5 participants