Cast store config image opacity to int (#3727)

[Tomasz-Silpion]

Description (*)

This PR fixes PHP 8.1 exception Argument #9 ($pct) must be of type int, string given in lib/Varien/Image/Adapter/Gd2.php:504 thrown when custom image opacity is set in admin configuration.

Related Pull Requests

Fixed Issues (if relevant)

1. Fixes PHP 8.1 TypeError: imagecopymerge(): Argument #9 ($pct) must be of type int in GD when custom watermark image is applied #3727

Manual testing scenarios (*)

1. Ensure that you are working with PHP 8.1 or newer.
2. Go to System -> Configuration -> Product Image Watermarks.
3. Add watermark images and set every opacity field to any value.
4. Check if PDP works properly images are properly rendered with or without the adjustment.

Questions or comments

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)
• Add yourself to contributors list

[kiatng]

Is it better to fix it at here

magento-lts/app/code/core/Mage/Catalog/Helper/Image.php

Lines 490 to 500 in 11eca9d

	*
	* @param int $imageOpacity
	* @return $this
	*/
	public function setWatermarkImageOpacity($imageOpacity)
	{
	$this->_watermarkImageOpacity = $imageOpacity;
	$this->_getModel()->setWatermarkImageOpacity($imageOpacity);
	return $this;
	}

It may be reset by some custom code to null.

[Tomasz-Silpion]

I agree where this approach actually needs adjustment of some other lines as well c982305

[sreichel]

Id prefer the original PR ...

... and may later change it to public function setWatermarkImageOpacity(int $imageOpacity)

[fballiano]

why adding string if we cast to int later?

[Tomasz-Silpion]

Because Mage::getStoreConfig would pass string there.

So either let's go back to original concept with cast before Mage::getStoreConfig (let's vote with ❤️) or new one with casting to int inside setWatermarkImageOpacity (let's vote with 🎉).

[sreichel]

There are several places where Mage::getStoreConfig is cast to int.

Why not add a helper method similiar to getStoreConfigFlag ... ?

public static function getStoreConfigAsInt(string $path, $store = null): int
{
    return (int) self::getStoreConfig($path, $store);
}

[sreichel]

@kiatng @fballiano pls share your opinion :)

[sreichel]

why adding string if we cast to int later?

I think adding string would be correct, b/c string is "valid" too.

Without it code analyze may say "unnecessary casting too int" b/c from Docs only int is expected.

[kiatng]

Sorry for late reply, been very busy. getStoreConfigAsInt() and getStoreConfigAsFloat() is a good idea. I will support a PR for it. However, I have reservation on imposing strict type on existing functions such as public function setWatermarkImageOpacity(int $imageOpacity), it'll break BC.

I think adding string to @param int|string $imageOpacity is correct.

[edit] May be it'll be even better to specify the type in system.xml and return the right type with existing getStoreConfig() or a new function getStoreConfigStrictType().

[sreichel]

Ill create a PR at the weekend. Thanks :)

edit: i like the idea with system.xml, but for tools like phpstan it guess its more clean to have methods with return types. (and is does not add more complexity to getStoreConfig()).

[sreichel]

Think it can be closed.

[fballiano]

should have been fixed by #3736