Fix watermark transparency with the ImageMagick adapter - #41138
Open
sebfie wants to merge 1 commit into
Open
Conversation
addSingleWatermark() typed $compositeChannels as bool, so the integer channel mask built in watermark() (134217711) was silently coerced to true and reached Imagick::compositeImage() as channel 1 (CHANNEL_RED). Only the red channel took part in the composite, turning transparent watermark areas into a solid colour block. Fixes magento#41137
|
Hi @sebfie. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description (*)
Magento\Framework\Image\Adapter\ImageMagick::watermark()builds an integer channel mask:and hands it to
addSingleWatermark(), whose signature typed it asbool. The file has nodeclare(strict_types=1), so PHP silently coerces134217711totrue, andImagick::compositeImage()receives channel1— i.e.Imagick::CHANNEL_RED. Only the red channel takes part in the composite and the remaining channels are written with out-of-band values, so the transparent areas of the watermark are replaced by a solid colour block (pure red with ImageMagick 7.1.2).Telling detail:
addTiledWatermark()immediately above receives the same argument without any type hint, which is exactly whytilepositioning still works. That asymmetry strongly suggests theboolis a typo.Introduced in c509a4e (#26036, a PHPCS-only refactor that split the too-long
watermark()method into the two helpers), shipped in 2.4.0 and present in every 2.4.x since, including2.4-develop. The GD2 adapter is not affected.This PR types the parameter as
intand corrects the two@param bool $compositeChannelsdocblocks.Related Pull Requests
None.
Fixed Issues (if relevant)
Manual testing scenarios (*)
dev/image/default_adaptertoIMAGEMAGICK.165x100, opacity100, positionBottom Right.rm -rf pub/media/catalog/product/cache && bin/magento catalog:images:resize && bin/magento cache:flushStandalone check, no Magento install required:
Questions or comments
The value written to the channels left out of the mask is not stable across ImageMagick builds, which would explain the different colours reported over the years. These two look like the same root cause and were both closed without a fix:
Happy to add an integration test covering watermark transparency for both adapters if you would like one.
Contribution checklist (*)