ImageEffects of the same image style should be able to pass variables between them

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?

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

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.

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?

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.

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'.

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

#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.

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

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?

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.

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.

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

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?

Added Beta Phase Evaluation in the issue summary.

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).

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.

Created draft CR https://www.drupal.org/node/2386353

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.

@fietserwin thanks, CR updated with your comment.

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.

Rerolled.

Changed to feature request, and updated IS.

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

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.

@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.

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.

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.

Let's change the issue title too.

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.

+++ 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).

Doing #44, with tests.

+++ 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

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).

Re-rollled

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

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

Re-roll.

Correct coding standards errors.

Related, #2986669: Split ImageStyle into the config entity and a separate event-based image processing service

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.