Problem/Motivation

At the moment it is possible to set/getThirdPartySettings on the image style, but the effects that are part of that style do not have any visibility on them. There are use cases where a setting at the ImageStyle level should be accessed by its ImageEffects. Also, there are cases where one effect in the sequence needs to hand over some setting to an effect coming later in the sequence.

Examples:

  • In a ImageStyle::transformDimensions() call, an effect determines dimensions based on Exif rotation information, reading the info from the file via the $uri parameter. That effect should 'flag' that autorotation has been performed already, because since the Exif metadata is not updated back to the source file, if another instance of the same effect is present in the effect collection, it will change again the dimensions. So that 'flag' would prevent the second effect instance to execute.
  • #204497: Configurable background color when cropping to larger dimensions, and in particular comment #204497-82: Configurable background color when cropping to larger dimensions
  • Textimage sets a TPS in the ImageStyle, being the 'background color' of the image. That is used by 'Textimage text' effects part of the style to deal with cases where the image needs to extended in size, so that the color used to fill in the extended part of the image be consistent across all the effects. Also see #2098247: Provide a hook for ImageStyle::createDerivative.

Proposed resolution

Introduce an ImageEffectWithRuntimeVariablesInterface and trait so that effects that need to access the parent ImageStyle can do so, and can also set arbitary 'runtime variables' that can be accessed by further effects, and whose scope is limited within the execution of a ImageStyle::createDerivative, ::transformDimensions, or ::getDerivativeExtension call. Change the ImageEffectPluginCollection so to inject the runtime variables and the image style id to relevant effect plugins on initialization.

Remaining tasks

User interface changes

None.

API changes

Introduce a new ImageEffectWithRuntimeVariablesInterface with relevant methods.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Feature request because provides visibility of ImageStyle to its ImageEffects so that effects can access style's third party settings.
Issue priority Normal because this is only to allow contrib to leverage the image style third party settings interface.
Prioritized changes The main goal of this issue is expanding capabilities of the ImageStyle/ImageEffects system, allowing contrib to leverage the image style third party settings interface.
Disruption Not disruptive, will just allow contrib to leverage the image style third party settings interface, no changes to core effects.

Original report by @zuuperman

There are some use cases for effects, that require the style information to be given as argument to the effect callbacks. This is mostly the case when the effect settings can change per file.

For example: http://drupal.org/node/1250506#comment-4889248

The same goes for dimension callbacks, but i will update this in #1364670: ImageStyle::transformDimensions unable to deal with all effects.

CommentFileSizeAuthor
#58 1826362-58.patch16.07 KBJo Fitzgerald
#58 interdiff-57-58.txt2.83 KBJo Fitzgerald
#57 1826362-57.patch16.44 KBmondrake
#50 1826362-50.patch16.42 KBmondrake
#47 1826362-47.patch16.4 KBmondrake
#47 interdiff_45-47.txt15.51 KBmondrake
#45 1826362-image_runtime-45.patch17.04 KBmondrake
#45 interdiff_43-45.txt5.09 KBmondrake
#43 interdiff_15-43.txt10.73 KBmondrake
#43 1826362-image_style-43.patch13.84 KBmondrake
#34 1826362-34.patch8.17 KBmondrake
#27 1826362-image_style-27.patch8.68 KBmondrake
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,103 pass(es). View
#27 interdiff_22-27.txt1.66 KBmondrake
#22 interdiff.txt4.47 KBtim.plunkett
#22 1826362-image_style-22.patch8.51 KBtim.plunkett
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,986 pass(es). View
#15 1826362-image_style-15.patch8.78 KBtim.plunkett
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,830 pass(es). View
#15 interdiff.txt11.81 KBtim.plunkett
#14 1826362-14.patch6.17 KBmondrake
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,876 pass(es). View
#11 1826362-11.patch4.57 KBmondrake
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,499 pass(es). View
#9 image_style-1826362-9.patch10.94 KBmondrake
PASSED: [[SimpleTest]]: [MySQL] 58,986 pass(es). View
#3 1826362-3-info-for-dimensions.patch8.94 KBzuuperman
PASSED: [[SimpleTest]]: [MySQL] 46,440 pass(es). View
#1 1826362-1-style-as-argument.patch5.63 KBzuuperman
FAILED: [[SimpleTest]]: [MySQL] 46,393 pass(es), 0 fail(s), and 7 exception(s). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

zuuperman’s picture

Status: Active » Needs review
FileSize
5.63 KB
FAILED: [[SimpleTest]]: [MySQL] 46,393 pass(es), 0 fail(s), and 7 exception(s). View

and a patch

Status: Needs review » Needs work

The last submitted patch, 1826362-1-style-as-argument.patch, failed testing.

zuuperman’s picture

Status: Needs work » Needs review
FileSize
8.94 KB
PASSED: [[SimpleTest]]: [MySQL] 46,440 pass(es). View

Tests had to be changed for the new argument.

Status: Needs review » Needs work

The last submitted patch, 1826362-3-info-for-dimensions.patch, failed testing.

zuuperman’s picture

Status: Needs work » Needs review
zuuperman’s picture

#3: 1826362-3-info-for-dimensions.patch queued for re-testing.

claudiu.cristea’s picture

We can inject the image style object directly in ImageEffectInterface::applyEffect() as second argument and then pass the style in ImageStyle::createDerivative():

    foreach ($this->getEffects() as $effect) {
      $effect->applyEffect($image, $this);
    }

@zuuperman, can you provide a patch?

claudiu.cristea’s picture

Status: Needs review » Needs work
mondrake’s picture

Issue summary: View changes
FileSize
10.94 KB
PASSED: [[SimpleTest]]: [MySQL] 58,986 pass(es). View

New patch, along the lines of #7. Had to be built from scratch since a lot changed since #3...

mondrake’s picture

Status: Needs work » Needs review
mondrake’s picture

Title: Add style to be applied as an argument for effect callbacks » ImageStyle to be accessible from ImageEffect plugins
FileSize
4.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,499 pass(es). View

Completely new patch. Instead of passing ImageStyle to the effect methods, here I am proposing to inject the ImageStyle to the ImageEffect plugin when it gets instantiated, in a protected property. It was noted in #2098247-11: Provide a hook for ImageStyle::createDerivative that image effects may be executed independently from it being part of a style, and this approach would allow for that. Also, after the introduction of ThirdPartySettingsTrait in ImageStyle, this approach would allow to get such settings accessible from the effects.

fietserwin’s picture

This is better, because passing it to the apply method, also means passing it to the dimensions and extension callback. Moreover, it is quite normal for a child to know its parent.

+++ b/core/modules/image/src/ImageEffectPluginCollection.php
@@ -15,12 +17,37 @@
+  public function __construct(ImageEffectManager $manager, ImageStyleInterface $image_style, array $configurations = array()) {
+    parent::__construct($manager, $configurations);

given that

It was noted in #2098247-11: Provide a hook for ImageStyle::createDerivative that image effects may be executed independently from it being part of a style

should we define NULL as default for the ImageStyleInterface parameter?

tim.plunkett’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

This IS is not clear why image effects need this, or how this is a regression from D7.

  1. +++ b/core/modules/image/src/Entity/ImageStyle.php
    @@ -347,7 +347,7 @@ public function getEffect($effect) {
    -      $this->effectsCollection = new ImageEffectPluginCollection($this->getImageEffectPluginManager(), $this->effects);
    +      $this->effectsCollection = new ImageEffectPluginCollection($this->getImageEffectPluginManager(), $this, $this->effects);
    

    $this should be $this->id().

    Please see BlockPluginCollection and SearchPluginCollection. They do not contain an entire entity, just the entity ID. Additionally, only the search one sets the ID on the plugin, the block implementation only uses it internally.

  2. +++ b/core/modules/image/src/ImageEffectBase.php
    @@ -39,6 +40,13 @@
    +  protected $imageStyle = NULL;
    
    @@ -70,6 +78,21 @@ public static function create(ContainerInterface $container, array $configuratio
    +  public function setImageStyle(ImageStyleInterface $image_style) {
    ...
    +  public function getImageStyle() {
    

    This belongs on ConfigurableImageEffectBase, not here. And ensure the methods are added to ConfigurableImageEffectInterface.

  3. +++ b/core/modules/image/src/ImageEffectPluginCollection.php
    @@ -15,12 +17,37 @@
    +    $effect_plugin->setImageStyle($this->imageStyle);
    

    Finally, see SearchPluginCollection::initializePlugin(), it checks an additional interface that has setSearchPageId(), so that *all* search plugins aren't tied to search pages. This approach would help with "that image effects may be executed independently from it being part of a style" from above. And, it will be run on init, not on get.

mondrake’s picture

Status: Needs work » Needs review
FileSize
6.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,876 pass(es). View

Thanks @tim.plunkett

my intent at this stage is just to allow ImageEffects to access ThirdPartySettings set at the ImageStyle level, during the creation of image derivatives. It is not a regression. This issue is currently categorised as 'Feature request', so it should be postponed, but I would suggest it to be changed to 'task' and try to push it in 8.0, as it would look to me just as a natural completion of the introduction of ThirdPartySettings in the image style entity.

At the moment it is possible to set/getThirdPartySetting on the image style, but they are not visible to the effects.

For this reasons,

re #13.1 - $this is there because effects should be able to call set/getThirdPartySettings on their actual instance of the image style entity.

re #13.2 - set/getImageStyle here is relevant for any effect - not just for the configurable ones. I added the methods to ImageEffectInterface.

re #13.3 - moved the setter to initializePlugin

Apologies no interdiff :(

If we agree on above, I will update issue summary and change from 'feature request' to 'task'.

tim.plunkett’s picture

FileSize
11.81 KB
8.78 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,830 pass(es). View

Here's what I had in mind. Unlike the previous patch, it does not needlessly couple all image effects to their image styles.

mondrake’s picture

#15 thank you, looks good.

The only question I still have is about passing $this->id() vs $this to the ImageEffectPluginCollection.

My use case (exemplified) is the following: I have a style with 3 effects. The first effect will call, at runtime, setThirdPartySetting() on the ImageStyle to set a color to be used throughout the style as the background of the derivative image. The other two effects will call getThirdPartySetting() on the ImageStyle to fetch the color set up by the first effect.

  • Passing $this->id() like in #15 means that each effect will load its own instance of the ImageStyle entity from storage. So, to achieve my use case I would need to save the ImageStyle entity to storage on the first effect after calling setThirdPartySetting (or alternatively, implement a hook_image_style_presave() to fetch the color from the effect and make it persistent on the ImageStyle).
  • Passing $this, conversely, will make the 'parameter passing' between effects simpler because effects will just set/getThirdPartySettings on the same ImageStyle instance. Still it would be possible to save ImageStyle back to storage at any point if necessary.

I think both ways can work, was just sharing for thought.

mondrake’s picture

Category: Feature request » Task
Issue summary: View changes
Issue tags: -Needs issue summary update
tim.plunkett’s picture

If an effect modifies the style, the whole system is broken.

fietserwin’s picture

I am not sure which way I prefer. I think it all comes down to whether or not an ImageEffect should only exist within the context of an ImageStyle. If so, there's no need for an additional interface and trait and #14 does the job well. If not, #15 might be better from an engineering point of view, but both #14 (perhaps added with some checking to make it more robust) and #15 do the job (would this limit ImageStyleAware effects to ImageStyles or is the same checking on imageStyle(Id) being set still needed/)

Besides this question, #16 poses a real problem with the patch of #15: does "ImageStyle::load();" return the same instance and does it always return an instance, even if a style does not come from storage but is created in code? I would not call the whole system broken when an effect accesses/alters the thirdPartySettings of its parent. This is where these settings were introduced for.

Thus, if we can agree on the answer to the question if "ImageEffects should only exist within the context of an ImageStyles" we can choose a patch:
yes: #14: RTBC
no: #15: NW (loading via the id does not seem correct, pass the instance).

Unlike my previous ideas, the noted comment, I now think an image effect should be tied to an image style. Outside the context of an image style, do not use image effects but use the operations directly on the image (as the image file upload resize already does).

What do others think?

tim.plunkett’s picture

Our plugins should work outside of the code that currently uses them. Image effects are a nice wrapper around the actual operations they represent, especially the ones that are configurable. There could be any number of usages outside of image styles for these.

We've gone to great lengths to not couple our plugins to their current usages, let's please continue that.

fietserwin’s picture

Status: Needs review » Needs work

OK, so we choose to be able to use image effects in other contexts as well. In that case we continue with the patch from #15, but it is NW to address the concern mentioned in #16. @mondrake: if you change it, I will review and RTBC.

BTW: Why are these ThirdPartySettings on the ImageStyle and not on the Image that is passed around anyway? Or do we also need to know which ImageStyle is currently executing (or some other characteristics from ImageStyle) ? Because I am thinking about issue #2168511: Allow to pass save options to ImageInterface::save where we will need access to the image to pass info from ImageEffect to ImageToolkit, so thee we could use ThirdPartySettings as well, unless we say that we want to have separate save options, as that is not third party but toolkit settings.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
8.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,986 pass(es). View
4.47 KB

\Drupal\Core\Config\Entity\ThirdPartySettingsInterface is specifically for config entities.

mondrake’s picture

Issue summary: View changes

Thanks @tim.plunkett for #22, I was thinking on the same line.

I agree on comment in #18, effects should not save the style back to storage at derivative creation runtime, because other requests would get an instance specific setup from config storage which is not a good idea. Did not think enough when writing #16.

#2168511: Allow to pass save options to ImageInterface::save has quite an old patch there, things changed in the meantime as we have now an ImageToolkitOperation that can manage image format conversion - the only open point there IMO is about allowing to change JPEG image quality from the config setting. But let's continue there.

RTBC from my point of view, but I cannot because of the earlier patches. @fietserwin?

mondrake’s picture

Issue summary: View changes

Added Beta Phase Evaluation in the issue summary.

mondrake’s picture

Issue summary: View changes
fietserwin’s picture

Status: Needs review » Needs work

I found some minor, documentation related, points:

  1. +++ b/core/modules/image/src/ImageStyleAwareInterface.php
    @@ -0,0 +1,25 @@
    +   * @param string $image_style
    +   *   The image style.
    +   *
    

    Is now a \Drupal\image\ImageStyleInterface (no longer a string), so type hinting can/should also be used here and in the trait.

  2. +++ b/core/modules/image/src/ImageStyleAwareTrait.php
    @@ -0,0 +1,44 @@
    + * Provides an implementation for a image-style-aware object.
    + *
    

    ... for an image-style-aware ...

  3. +++ b/core/modules/image/src/ImageStyleAwareTrait.php
    @@ -0,0 +1,44 @@
    +   * Implements \Drupal\image\ImageStyleAwareInterface::setImageStyle().
    +   */
    

    Implements is not a formalized phpdoc tag. Thus no parameter list, no return type and no pop-up documentation in my IDE nor in generated documentation. I assume that {@inheritdoc} is also a bit strange in a trait, but does another tag exists that can be used here? Perhaps {@inheritdoc} plus the textual comment (after all,@inheritdoc is an inline tag and phpdoc allows text around it (I think it is just the Drupal standards that don't).

#22:thanks: thus no ThirdPartySettings on Image, in that issue we will stay with what it is now (plus reroll).

mondrake’s picture

FileSize
1.66 KB
8.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,103 pass(es). View

1. Done
2. Done
3. I checked ThirdPartySettingsInterface and ThirdPartySettingsTrait and there the docs are repeated in both interface and trait. Did the same here.

We probably need a change record, I'll draft one.

mondrake’s picture

Status: Needs work » Needs review
mondrake’s picture

fietserwin’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, also @tim.plunkett for guarding the Drupal architectural parts that I (we) do not know in detail. The CR also looks good, though you might emphasize that this feature is normally not needed as image effects have their own settings/configuration storage. Only in specific cases where the settings are shared with other effects/components this comes in handy.

mondrake’s picture

@fietserwin thanks, CR updated with your comment.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

We need to improve the beta evaluation to state exactly how

The main goal of this issue is completing capabilities of the ImageStyle/ImageEffects system.

is allowed at this point.

mondrake’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Postponed
mondrake’s picture

Status: Postponed » Needs review
FileSize
8.17 KB

Rerolled.

mondrake’s picture

Category: Task » Feature request
Issue summary: View changes
Issue tags: -Needs issue summary update

Changed to feature request, and updated IS.

mondrake’s picture

Issue summary: View changes
mondrake’s picture

Status: Needs review » Reviewed & tested by the community

#27 was RTBC, and #34 is a plain reroll. Setting back to RTBC, the IS was updated according #32.

tim.plunkett’s picture

This goes against the entire premise of plugins being decoupled from their parent.
Please won't fix, or go back to the implementation in #15.
See also #20.

mondrake’s picture

@tim.plunkett the patch in #34 is practically same as yours in #22, just minor changes in #26 and reroll. Can you confirm you'd rather

won't fix, or go back to the implementation in #15

?

Thanks.

tim.plunkett’s picture

Yeah I tried to remember why I wrote #22. I cannot for the life of me figure that out. But #15/#20 match my current thoughts as well.

mondrake’s picture

Status: Reviewed & tested by the community » Needs work

OK then, I am setting this back to NW.

For me #15 is not much help, since each effect would open separately an ImageStyle instance from storage.

What I miss is what @claudiu.cristea commented in #2098247-2: Provide a hook for ImageStyle::createDerivative (emphasis mine):

Definitely I see the need that effects of the same image style should be able to pass variables between them. I just want to find a more modern way to do this without placing hook invocations all over the code.

Any help in this sense appreciated.

mondrake’s picture

Title: ImageStyle to be accessible from ImageEffect plugins » ImageEffects of the same image style should be able to pass variables between them

Let's change the issue title too.

mondrake’s picture

Status: Needs work » Needs review
FileSize
13.84 KB
10.73 KB

This patch builds on patch in #15, adding a pooled storage of 'runtime variables' (a MemoryStorage object) on the collection that is passed to relevant image effects. Image effects can then get/set values from that pool.

The image style id here thus becomes 'one' of the runtime variables that can be passed on to effects, with the difference that this is always initialised within the collection.

The actual 'parent' image style can be retrieved by effects via the helper method ImageStyleAwareTrait::getImageStyle(), in a way similar to #15.

I think that this addresses the concern in #38 to keep ImageEffect plugins decoupled from their parent image style - the ImageStyleAwareTrait::hasRuntimeVariables() method should be used by implementing plugins to ensure that runtime variables (inititialised in the collection, but non-existing if the plugin is initiatied in isolation from it) exist before trying to access them.

Also, ImageStyleAware(Interface|Trait) seems no longer a good name. I would suggest ImageEffectWithRuntimeVariables(Interface|Trait).

Thanks for any review.

mondrake’s picture

Assigned: Unassigned » mondrake
Status: Needs review » Needs work
+++ b/core/modules/image/src/Entity/ImageStyle.php
@@ -319,6 +321,7 @@ public function transformDimensions(array &$dimensions, $uri) {
+    $this->getEffects()->resetRuntimeVariables();

Actually I would also need an option to prevent runtime variables to be reset before ::transformDimensions, ::getDerivativeExtension and ::createDerivative, so that parent code could do it instead (and pass variables to the first effect in the collection if that is needed).

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review
FileSize
5.09 KB
17.04 KB

Doing #44, with tests.

mondrake’s picture

Status: Needs review » Needs work
+++ b/core/modules/image/src/ImageStyleInterface.php
@@ -153,6 +153,29 @@ public function transformDimensions(array &$dimensions, $uri);
   /**
+   * Indicates to skip resetting runtime variables.
+   *
+   * Allows to skip resetting runtime variables at next call of any of
+   * ::createDerivative, ::transformDimensions, ::getDerivativeExtension
+   * methods. The setting is only relevant for next call, further calls of
+   * these methods will be resetting the variables, unless this method is
+   * called again.
+   *
+   * @param bool $skip
+   *   (optional) When TRUE, the runtime variables will not be reset at next
+   *   call of ::createDerivative, ::transformDimensions,
+   *   ::getDerivativeExtension. When FALSE, runtime variables will be reset.
+   *   Defaults to TRUE.
+   *
+   * @return $this
+   *
+   * @see ::createDerivative
+   * @see ::transformDimensions
+   * @see ::getDerivativeExtension
+   */
+  public function skipRuntimeEnvironmentReset($skip = TRUE);
+
+  /**

learnt in #2630242-6: Provide methods to retrieve EXIF image information via the Image object, we cannot add/change methods in existing interfaces, so this has to be done differently

mondrake’s picture

Status: Needs work » Needs review
FileSize
15.51 KB
16.4 KB

Moved method skipRuntimeEnvironmentReset from ImageStyleInterface to ImageEffectPluginCollectionInterface, so not to have BC-breaking changes to ImageStyleInterface.

Also, per #43, renamed ImageStyleAware(Interface|Trait) to ImageEffectWithRuntimeVariables(Interface|Trait).

mondrake’s picture

Component: image system » image.module
mondrake’s picture

Issue summary: View changes
mondrake’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mondrake’s picture

This is blocking Image Effect's #2748307: Aspect Switcher.

There we are trying to add an effect that will select another image style for execution, depending on the image orientation. But without the effect knowing the 'parent' image style, it's not possible to ensure prevent that the 'parent' style is selected as a 'substyle', which causes infinite loops, inconsistency in the config objects, and ultimately WSOD.

EDIT - wrong verb, strikethrough applied

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs reroll

Patch needs a reroll. Also, the IS needs updating since the beta evaluation phase is long over.

mondrake’s picture

Re-roll.

Jo Fitzgerald’s picture

Correct coding standards errors.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.