- 🇺🇸United States smustgrave
This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.
Could you please provide steps/configuration taken to recreate this.
This will need a test case showing the issue.Thanks
- 🇺🇸United States jenlampton
I was able to recreate this by uploading a .webp image into an image/media reference field, and then rendering it using an image style.
- Status changed to Needs review
about 2 years ago 2:35pm 3 March 2023 - 🇷🇺Russia niklan Russia, Perm
I was able to extract the broken color profile from the image and apply it to another one and trigger that error in tests. The broken color profile called sRGB IEC61966-2.1. I'm attaching it as well, so you can test it by yourself on any image (by injecting it using CLI tools or image editors like GIMP, Krita, etc).
I'm also switch versions to 10.1.x, because patch from #8 won’t apply on 10.0.x branch. And I focus initially on future branch, if it's needed it can be backported later on.
- 🇷🇺Russia niklan Russia, Perm
Updated issue summary with steps to reproduce this problem.
- 🇷🇺Russia niklan Russia, Perm
Sorry for noise, patch in #20 contains .icc profile by accident. This one is clean patches.
- 🇷🇺Russia niklan Russia, Perm
Gosh 🤯
Anyway, since I need to create patches again, I generated a test image using
convert
instead of GIMP. It is smaller, because there is no GIMP metadata in it.🤞
The last submitted patch, 22: drupal-3261924-22-test-only.patch, failed testing. View results →
- 🇷🇺Russia Chi
Personally, I would not create tests for suppression operators. Such tests are too tricky.
Also, I think it needs a comment explaining what kind of errors could happen without the suppression. - 🇷🇺Russia niklan Russia, Perm
Added comment for suppression call.
Disabled tests, because the only difference from #22 is comment.
- Status changed to RTBC
about 2 years ago 9:36pm 20 March 2023 - 🇦🇺Australia jannakha Brisbane!
#26 works! no more warning on d 9.5.5/php 8.1.1
thank you! - Status changed to Needs work
about 2 years ago 8:36am 21 March 2023 - 🇳🇿New Zealand quietone
The issue title is used as the commit message and it should state what this issue resolves instead of being a long error message.
Do we really want to suppress all warnings?
- Status changed to Needs review
over 1 year ago 4:20pm 12 July 2023 - last update
over 1 year ago 2,159 pass - 🇺🇸United States neclimdul Houston, TX
Better title.
Do we really want to suppress all warnings?
I had the same question, but I'm not sure what options we're really given. I suspect this is probably OK though because real problems will throw and exception which gets caught and logged. At least in the 10.x versions of this patch.
That said, the block we're adjusting is generalized for all image formats but the comment clearly only addresses the impact on PNGs. Very bikesheddy but maybe we should probably adjust the documentation to match the scope of the code not the scope of this issue. Something like this?
// Suppress warnings from image creation because there is a possibility // that source images contain data that would trigger non-fatal warnings // we can't handle. For example, a PNG saved with 'sRGB IEC61966-2.1' // color profile has 'iCCP' chunk data which is not recognized on most // systems and will cause libpng to trigger warnings but otherwise do not // affect image processing.
- Status changed to Needs work
over 1 year ago 9:13pm 17 July 2023 - 🇺🇸United States smustgrave
Looking at #25 and should the test go into ToolkitGdTest?
Also reading the last comment 31 think expanding to match makes sense. Least I can't think of a reason not to.
39:31 38:47 Running- last update
over 1 year ago Patch Failed to Apply - 🇺🇸United States ricksta
I confirm the patch in 34 🐛 Avoid warning from imagecreatefrompng when loading png with obscure iCCP profiles Needs work works for Drupal 10.
- last update
over 1 year ago 29,672 pass - Status changed to RTBC
over 1 year ago 11:16pm 20 October 2023 - 🇺🇸United States mark_fullmer Tucson
Based on the comment from #35, I'm changing the status to RTBC.
- Status changed to Needs review
over 1 year ago 7:02pm 10 November 2023 - 🇺🇸United States neclimdul Houston, TX
Based on the rest of the comments and the lack of tests don't think this is actually ready. There aren't even tests on the last couple patches.
re: #32 what the heck is that test. Its not in the system module, its in the wrong location based on the namespace of the class, the file name doesn't align with the tested class? Something weird going on there but it does look like that's where existing tests are.
- last update
over 1 year ago Custom Commands Failed - Status changed to Needs work
over 1 year ago 8:18pm 10 November 2023 The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- last update
over 1 year ago 29,686 pass - last update
over 1 year ago 29,686 pass - First commit to issue fork.
- Merge request !5800#3261924 Avoid warning from imagecreatefrompng when loading png with obscure iCCP profiles → (Closed) created by elaman
- Merge request !5801#3261924 Avoid warning from imagecreatefrompng when loading png with obscure iCCP profiles → (Open) created by elaman
- Status changed to Needs review
over 1 year ago 2:04pm 18 December 2023 - Status changed to Needs work
over 1 year ago 2:47pm 20 December 2023 - 🇺🇸United States smustgrave
1 small feedback item.
Hiding patches for clarity.
- 🇺🇦Ukraine Taran2L Lviv
@smustgrave I disagree, the dictionary already has all the wierd words ...
- Status changed to Needs review
over 1 year ago 8:44am 21 December 2023 - Status changed to RTBC
over 1 year ago 2:30pm 21 December 2023 - 🇺🇸United States smustgrave
There was 1 error: 1) Drupal\KernelTests\Core\Image\ToolkitGdTest::testIncorrectIccpSrgbProfile with data set "PNG with iCCP profile" ('core/tests/fixtures/files/ima...le.png') PHPUnit\Framework\Exception: PHP Warning: imagecreatefrompng(): gd-png: libpng warning: iCCP: known incorrect sRGB profile in /builds/issue/drupal-3261924/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php on line 271 Warning: imagecreatefrompng(): gd-png: libpng warning: iCCP: known incorrect sRGB profile in /builds/issue/drupal-3261924/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php on line 271 /builds/issue/drupal-3261924/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684 /builds/issue/drupal-3261924/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684 /builds/issue/drupal-3261924/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651 /builds/issue/drupal-3261924/vendor/phpunit/phpunit/src/TextUI/Command.php:144 /builds/issue/drupal-3261924/vendor/phpunit/phpunit/src/TextUI/Command.php:97 ERRORS! Tests: 97, Assertions: 1197, Errors: 1.
Verified test coverage using test-only feature.
All feedback has been addressed.
@Taran2L as far as threads, as the opener of the thread I use to be able to close myself but that no longer seems to be the case. Think only the person who opened the MR can resolve threads now.
- 🇳🇿New Zealand quietone
I'm triaging RTBC issues → . I read the IS, the comments and the MR. Thanks for the issue summary. As someone who doesn't work with images it was very clear and easy to understand the problem. I didn't find any unanswered questions.
I read the MR (not a code review) and there is minor work to do on the comments.
- Status changed to Needs work
over 1 year ago 7:01am 31 December 2023 - 🇳🇿New Zealand quietone
Oh sorry, @neclimdul, thank you for answering my question in #31.
- last update
over 1 year ago 25,813 pass, 1,797 fail 57:38 55:38 Running- last update
over 1 year ago 25,783 pass, 1,835 fail - last update
over 1 year ago 25,761 pass, 1,823 fail - Status changed to Needs review
about 1 year ago 10:53am 8 January 2024 - 🇺🇦Ukraine Taran2L Lviv
Feedback has been addressed
Also, attaching a static patch from the latest changes in the MR (for composer patching purposes)
- Status changed to Needs work
about 1 year ago 2:44pm 8 January 2024 - Status changed to Needs review
about 1 year ago 4:42pm 8 January 2024 - Status changed to RTBC
about 1 year ago 2:29pm 10 January 2024 - 🇦🇺Australia nigelcunningham Geelong
Attaching static version of MR for composer patch.
- Status changed to Needs work
about 1 year ago 6:25pm 3 February 2024 - 🇬🇧United Kingdom longwave UK
The test doesn't work for me locally. With or without the change to GDToolkit I get:
Testing Drupal\KernelTests\Core\Image\ToolkitGdTest ..E 3 / 3 (100%) Time: 00:03.934, Memory: 4.00 MB There was 1 error: 1) Drupal\KernelTests\Core\Image\ToolkitGdTest::testIncorrectIccpSrgbProfile with data set "PNG with iCCP profile" ('core/tests/fixtures/files/ima...le.png') PHPUnit\Framework\Exception: libpng warning: iCCP: known incorrect sRGB profile
- 🇺🇦Ukraine Taran2L Lviv
@longwave this is very weird - it does not work on my local as well ... but it seems it does work on CI, weird
I think the value of the test is that error is being suppressed.
- 🇷🇺Russia niklan Russia, Perm
Hello again. I see some folks not very happy with silencing all the errors from that call. I understand that as well, it can be helpful sometimes. Without it, I won't be found this issue as well. Maybe we wrap this call
$image = @$function($this->getSource());
by a condition?We have already configuration `system.logging` with `error_level` setting. Maybe we will do it like:
if ($error_level === 'verbose') { $image = $function($this->getSource()); } else { $image = @$function($this->getSource()); }
This allows a developer to find out that there is a problem, but it won't be logged and showed on production instances. It might be looking ugly, but it solves most of the concerns here. Currently, as I remember correctly (I've already fixed that images), this will be logged on production on every over page with a broken image when it's processed for the first time (if no style yet created). In some cases, it can create a lot of useless noise in the logging system.
- 🇺🇸United States generalredneck Texas, USA 🇺🇸
It IS possible to handle warnings, and therefore forego suppressing them all, but it would require setting up an error handler to do it. That said, if we are worried about particular image profiles, is there a way to whitelist some and check to see if the image is what we want? Is that too process intensive?
Trying to throw some fresh ideas at this that I didn't see in the comments. I do like Niklan's thoughts though.
- 🇺🇸United States neclimdul Houston, TX
I think the concerns might be a bit overstated. Actual problems will trigger real exceptions that get logged. This just avoids warnings that shouldn't be triggered. If I remember correctly they're warnings triggered by the underlying libraries so they don't actually follow PHP's documentation for the method.
- First commit to issue fork.
- 🇦🇺Australia VladimirAus Brisbane, Australia
Resolved MR conflict and added patches for D10 and D11.
- 🇺🇸United States rraney
Just curious about Patch 65. What's all the garbage text at the end of the file?
- 🇺🇸United States generalredneck Texas, USA 🇺🇸
That's a new file... png based on the diff.
diff --git a/core/tests/fixtures/files/image-test-iccp-profile.png b/core/tests/fixtures/files/image-test-iccp-profile.png new file mode 100644 index 0000000000000000000000000000000000000000..29cb8945f8e4c1ae40de24bf888715e4d870a164 --- /dev/null +++ b/core/tests/fixtures/files/image-test-iccp-profile.png @@ -0,0 +1,24 @@ +‰PNG
- 🇦🇺Australia VladimirAus Brisbane, Australia
Trying to bring all changed into new branch as pull from 11.x returns too many merge conflict. 🤷♀️
Also, changing branch to 11.0.x - 🇦🇺Australia VladimirAus Brisbane, Australia
VladimirAus → changed the visibility of the branch 3261924-avoid-warning-from-d11.0 to hidden.
- 🇷🇺Russia niklan Russia, Perm
I encountered the same issue again, and there was a lot of noise in the logs about it. If you just want to fix images and avoid patching the core system, you can use the following script (adapt it to your needs):
#!/usr/bin/env bash # Fixes iCCP: known incorrect sRGB profile cd web/sites/default/files || exit find . -type f -name '*.png' -exec mogrify \{\} \;
This tool will go through all your PNG images and re-save them with fixes for any issues found. However, it fixes more than just the iCCP profile, so use it with caution. I haven't faced any issues yet.
If you know exactly which images have problems, it's best to run the command directly on those files, like this (you can play with broken files from that issue):
mogrify filename.png
I recommend testing this tool locally before applying it to your live files. Alternatively, you could run the entire process locally and then sync the changes using rsync.
Please note that this command may take some time to complete, because it will open and save each image, regardless of whether it has any issues or not. The more and bigger in size files you have, the slower the process will be.
It utilizes the Mogrify binary provided with the ImageMagick package.
- 🇳🇿New Zealand quietone
Fixes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.
- Status changed to Needs review
9 months ago 1:46pm 17 July 2024 - Status changed to Needs work
9 months ago 3:02pm 17 July 2024 - 🇪🇪Estonia tormi Tallinn
Marked this ticket as possibly related: https://www.drupal.org/project/drupal/issues/3424941#comment-15760118 🐛 GDToolkit::load() doesn't check value, returned from executing $function Needs work
- 🇩🇪Germany Anybody Porta Westfalica
Confirming this issue still exists and I agree muting it would make sense, as users can't do anything about it in most cases.
- Status changed to Needs review
7 months ago 12:37pm 16 September 2024 - Status changed to Needs work
7 months ago 1:41pm 16 September 2024 - 🇮🇳India nitesh624 Ranchi, India
There is one more warning occurring in image system https://www.drupal.org/project/drupal/issues/3474274 💬 GDToolkit::load() error: 'iCCP: too many profiles' Active
- 🇮🇳India nitesh624 Ranchi, India
I am also getting error during test run locally
PHPUnit\Framework\Exception: PHP Deprecated: image_type_to_extension(): Passing null to parameter #1 ($image_type) of type int is deprecated in web/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Resize.php on line 63
- 🇺🇸United States smustgrave
Ran test-only feature
1) Drupal\KernelTests\Core\Image\ToolkitGdTest::testIncorrectIccpSrgbProfile with data set "PNG with iCCP profile" ('core/tests/fixtures/files/ima...le.png') PHPUnit\Framework\Exception: PHP Warning: imagecreatefrompng(): gd-png: libpng warning: iCCP: known incorrect sRGB profile in /builds/issue/drupal-3261924/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php on line 194 Warning: imagecreatefrompng(): gd-png: libpng warning: iCCP: known incorrect sRGB profile in /builds/issue/drupal-3261924/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php on line 194 ERRORS! Tests: 96, Assertions: 1185, Errors: 1. Exiting with EXIT_CODE=2
Shows current coverage.
Hiding the patches so it's clear just the MR, and resolved all threads except the one. Since we are suppressing with '@' not sure a CR would be needed? Will let committer decide that one.
But rest of the feedback appears to be addressed.
- Status changed to RTBC
4 months ago 3:09am 20 December 2024 - 🇬🇧United Kingdom catch
MR looks good to me, but unfortunately needs a rebase.
- Status changed to Needs work
about 1 month ago 4:00am 2 March 2025 - First commit to issue fork.
- 🇺🇸United States weekbeforenext Asheville, NC
Merged in the latest from origin.
- 🇬🇧United Kingdom catch
Committed/pushed to 11.x, thanks!
Did my best with commit credit but this is a long issue with a lot of commenters on it so apologies if someone was overlooked.
Automatically closed - issue fixed for 2 weeks with no activity.