-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Add Imagick CMYK to SRGB conversion #28809
Add Imagick CMYK to SRGB conversion #28809
Conversation
Hi @tdgroot. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. |
The test is here, that's a fact. But the test is not testing what this problem is all about. |
I do not completely agree with you. While it would be very thorough to test if the image pixels are corrected, this is not completely Magento territory. That is, because Magento should not test if Imagick is doing its work. It IS Magento's responsibility to convert the color space when it detects CMYK color space, so that actually needs to be tested to verify the conversion takes place, which is to be seen by checking the |
The risk was set to |
/** | ||
* @var \Imagick | ||
*/ | ||
protected $_imageHandler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you need this var? It should be inherited from the parent class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It provides type information for within the ImageMagick class.
@magento run all tests |
Hi @sidolov, thank you for the review.
|
Hi @tdgroot, thank you for your contribution! |
Add Imagick CMYK to SRGB conversion
Description (*)
When Imagick adapter detects that the opened image has a CMYK colorspace, convert the colorspace to SRGB.
Related Pull Requests
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Not sure if the CMYK image added to the unit test has any copyrights to it. @ioweb-gr Can you confirm if the image can be used for this purpose?
Contribution checklist (*)