Update GdDriver.php #298
Update GdDriver.php #298
Conversation
Show errors on image manipulation. related topic : spatie/laravel-medialibrary#3778
|
This could be viewed as a breaking change. Could you add a method to enable/disable this? |
|
I can definitely add a method to enable or disable this behavior. That said, I’m wondering if this is really a breaking change. Currently, when there's an issue with the image, the @imagecreatefromstring call simply fails silently and stop everything else, which could lead to harder-to-debug problems down the line. Removing the error suppression would actually make issues more visible, which might be desirable in most cases. Wouldn't it be more consistent (and developer-friendly) to throw an exception or return an explicit error instead of failing silently? I'm happy to implement the toggle, but I wanted to raise the point in case improving error visibility is actually the preferred default behavior. |
|
I'm a bit afraid of people now getting error warnings where previously they would get none. But you're probably right that maybe we want to make this kind of situation more visible. Change it to throwing an exception indeed, then users can easily determine themselves how it should be handled. |
Throw exception instead of suppressing errors in imagecreatefromstring
src/Drivers/Gd/GdDriver.php
Outdated
| try { | ||
| $image = imagecreatefromstring($contents); | ||
| } catch (Throwable $e) { | ||
| throw CouldNotLoadImage::make($path); |
There was a problem hiding this comment.
The reason why it failed is probably in the original exception, best to pass that on to CouldNotLoadImage
|
Most of other MR has the same failed task. It Seems to not be related to my MR |
src/Drivers/Gd/GdDriver.php
Outdated
| $image = @imagecreatefromstring($contents); | ||
| try { | ||
| $image = imagecreatefromstring($contents); | ||
| } catch (Throwable $e) { |
There was a problem hiding this comment.
Do not use abbreviations: use $exception instead of $e
src/Drivers/Gd/GdDriver.php
Outdated
| try { | ||
| $image = imagecreatefromstring($contents); | ||
| } catch (Throwable $e) { | ||
| throw CouldNotLoadImage::make($e); |
There was a problem hiding this comment.
Could you also add the path to the exception?
|
Thanks! |
3 participants
Show errors on image manipulation. related topic : spatie/laravel-medialibrary#3778