Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Follow-up to #2359443: Allow creating image derivatives from an Image object
Problem/Motivation
From #2359443-17: Allow creating image derivatives from an Image object:
+++ b/core/modules/image/src/Entity/ImageStyle.php
@@ -309,6 +320,23 @@ public function createDerivative($original_uri, $derivative_uri) {
+ // Apply the effects to the image object.
+ foreach ($this->getEffects() as $effect) {
+ $effect->applyEffect($image);
+ }
...
+ return TRUE;
Only now I see this crap. So ImageEffectInterface::applyEffect() does return a success flag but we are not using it here. We just return TRUE regardless if the effects were applied correct. Is there a good reason for this? I don't remember. If there's a reason, we need to document it, otherwise we need something like:
if (!$effect->applyEffect($image)) {
return FALSE;
}
Proposed resolution
Remaining tasks
- discuss what to do
- write a patch
User interface changes
API changes
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | drupal-2630224-5-image-effect-check-return.patch | 546 bytes | Sweetchuck |
| #2 | drupal-2630224-image-effect-check-return.patch | 898 bytes | Sweetchuck |
Comments
Comment #2
SweetchuckComment #3
SweetchuckComment #4
mondrakeThank you.
1. This needs a test - to proof how an effect returning false from ::applyEffect is now not influencing ::createDerivative, and that with the patch here will return false too.
2.
I like the idea of logging the failure in ::createDerivative. However, at the moment we will end up with two log entries, since (at least core) effects also log failure individually. I think in this patch we should be removing this, and only have logging in ::create Derivative.
In other words, example from ScaleAndCropImageEffect:
could become
3.
I am not sure about this concept of 'position' of the effect in the style. It's not giving any input to users, since this is not shown anywhere else. We may rather present the 'weight', even if that's also not usually shown to users (only if js is not enabled).
Comment #5
SweetchuckWithout the logging.
Test is still missing.
Comment #6
mondrake@Sweetchuck actually my suggestion is to keep logging in ::createDerivative like you did in #2, and remove the logging from each of the core effects.
Sorry if I was not clear.
Comment #7
SweetchuckI think on the image effect level a handler can log more accurate error message what the problem was.
To reorganize the error message logging could be a separate issue.
Comment #8
mondrakeThe point is that if the effect is not logging a failure, currently there will be no evidence of anything going wrong (that is the case, already, for effects provided by contrib module Image Effects). So here having logging in ::createDerivative will ensure that the failure is logged, at least once. AFAICS, the core effects all have the same pattern for the log entry, and all info should be available in ::createDerivative. Then if an effect wants to log something extra, it could always do so. But yes, that's my suggestion, we need more input here.
Comment #9
mondrakeTagging