Repository navigation
Fall back to the default image when an image cannot be generated - #1665
Conversation
The default image only replaced a source that could not be loaded. An image the driver fails to process, or a filter set that does not exist, still ended up as an error page in production. Outside of debug mode, both now redirect to the default image as well, and the exception is logged as a warning. The new controller.debug option defaults to the kernel debug mode, so failures stay visible while developing.
28342db to
c9c5bb4
Compare
dbu
left a comment
dbu
left a comment
There was a problem hiding this comment.
thanks, this seems a good idea to me.
i suggest 2 small improvements to the PR, otherwise looks correct to me.
| return null; | ||
| } | ||
|
|
||
| $this->logger->warning(\sprintf('Failed to create image for filter "%s", falling back to the default image. Message was "%s"', $filter, $exception->getMessage()), ['exception' => $exception]); |
There was a problem hiding this comment.
please don't use sprintf but logger context for the filter and message. the additional context with the exception is good, it makes the exception logged if the log level is high enough.
There was a problem hiding this comment.
Done: filter and message are context values now, with the exception alongside them.
There was a problem hiding this comment.
i think it would make sense to route this through createDefaultImageResponse as well, so that we get the logging and avoid duplication.
i am aware that there is a BC break that the exception is now thrown in debug mode, but i'd hope people do not run debug mode in production. if we are worried about it, we could add a flag to createDefaultImageResponse to overwrite the debug flag for a call.
There was a problem hiding this comment.
Routed through the helper, with an $evenInDebug flag rather than the BC break. This fallback predates the debug check and already applied in debug mode, so the flag keeps it exactly as it was while still getting the logging and dropping the duplication. Covered by a new test.
Pass the filter and the message as logger context instead of interpolating them into the message, as suggested. Route the NotLoadableException fallback through createDefaultImageResponse too, so it is logged and the code is not duplicated. That fallback predates the debug mode check and applied in debug mode as well, so the helper takes a flag to keep it that way rather than changing its behaviour.
|
Thanks for the quick review! First point applied: the filter and the message are logger context now, and the exception stays in there too. On the second one I took your fallback option rather than the BC break, because that fallback is older than this PR: So |
dbu
left a comment
dbu
left a comment
There was a problem hiding this comment.
makes sense, thanks!
i added a note that we want to remove the evenInDebug flag in the next major.
|
@dbu, I'm seeing a lot of critical errors in production. It's coming from bots hitting old URLs for filters that have since been deleted from config. Tracing it through: when a filter is totally unconfigured (not just missing default_image), createDefaultImageResponse() → DataManager::getDefaultImageUrl() → FilterConfiguration::get($filter) throws NonExistingFilterException again, and that second throw isn't caught - it escapes past the catch (NonExistingFilterException) block entirely instead of reaching the NotFoundHttpException fallback right below it. This only happens outside debug mode, since in debug the method returns early before calling getDefaultImageUrl. Looks like getDefaultImageUrl() (or its caller) needs to treat "filter not configured" as "no default image" rather than propagating the exception. |
|
Thanks a lot for tracing it, @trsteel88, and sorry for the trouble! That is my bug: the fallback did not account for |
Follows @dbu's design in #1622.
default_imageonly ever replaced a source image that could not be loaded. When the source is there but the driver cannot process it (the reporter's case: a video uploaded next to images, but also an SVG imagick will not rasterize), or when the requested filter set does not exist, the controller still turned that into an error page.The controller now takes a
debugflag, defaulting to%kernel.debug%:debug: truekeeps throwing, so a typo in a filter name or a file the driver chokes on stays visible while developing.debug: falseredirects to thedefault_imagefor those two cases as well, and logs the exception with thewarninglevel so it does not get silently swallowed. If no default image is configured, the exception is thrown as before.NotLoadableExceptionis untouched: it falls back to the default image regardless of the flag, as it always did.ControllerConfiggains a second constructor argument that defaults totrue, andImagineControllera fifth optional one for the logger, so both stay BC.While at it,
default_imageis documented for the first time. It has been around for 11 years and only appeared in the config dump.