Problem

  • Image effects have a set of defined properties, as well as metadata and callbacks, but Image module processes them in a rather funky way.

Proposed solution

  • Convert image effects into plugins, with defined properties, settings, and method callbacks.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Definitely needed, and should be fairly simple.

Also, this is a perfect use case for #1764380: Merge PluginSettingsInterface into ConfigurableInterface - note that the proposed PluginWithSettingsBase class is already in core (within field module currently), as part of "widgets/formatters as plugins")

andypost’s picture

@yched do you mean that issue should be postponed while #1764380: Merge PluginSettingsInterface into ConfigurableInterface fixed?
There's confirmation (delete) forms for effects, not sure that this solvable with "settings" approach

A kind of current views-plugins has similar code

tim.plunkett’s picture

I think this should follow the pattern of Views displays and Format filters (#1868772: Convert filters to plugins) by having ImageStyle use a PluginBag (http://drupal.org/node/1878416).
I'm working on too many of these at once right now to dive into another, but if blocks or formats get in soon I can work on this

tim-e’s picture

Assigned: Unassigned » tim-e
effulgentsia’s picture

Issue tags: -Plugin system

@tim-e: it's been a while since #4. Do you still want this assigned to you?

effulgentsia’s picture

Title: Convert image effects into plugins and/or configurables » Convert image effects into plugins
Issue tags: +Plugin system

Restoring tag I didn't mean to remove. Also shortening title per #3.

effulgentsia’s picture

Assigned: tim-e » Unassigned
Issue tags: -Configuration system, -Plugin system, -Configurables +Plugin-conversion

No answer to #5, so unassigning. Creating a new tag for remaining plugin conversions. There's very few pseudo plugin systems left in D8 core that haven't been converted. If you know of any, please find the issue (or open one if it's not already there) and tag it.

claudiu.cristea’s picture

Related: #2027423: Make image style system flexible

Will provide a patch today.

andypost’s picture

Suppose better to build the issue on top of #2027423: Make image style system flexible
Is there any sandbox for ?

claudiu.cristea’s picture

Just create one at https://drupal.org/sandbox/claudiucristea/2028715. It has only the 8.x branch right now

alexpott’s picture

Priority: Normal » Critical
alexpott’s picture

Priority: Critical » Normal

Oops got confused by @timplunkett

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

I apparently confused this and image toolkits, I forgot this was undone. :(

tim.plunkett’s picture

Status: Active » Needs review
FileSize
46.9 KB

Work in progress, just posting something.

Status: Needs review » Needs work

The last submitted patch, image-effect-1821854-14.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
54.66 KB
77.18 KB

Finished the basic conversion, now going to revisit how image effects are stored in image styles.

Status: Needs review » Needs work

The last submitted patch, image-effect-1821854-16.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
32.18 KB
98.68 KB

More fixes. Form converted. PluginBag up next.

Status: Needs review » Needs work

The last submitted patch, image-effect-1821854-18.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
115.71 KB

Lost track of my interdiffs, sorry.

Status: Needs review » Needs work

The last submitted patch, image-effect-1821854-20.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
790 bytes
115.81 KB

This only required a trivial fix to the test, not to the code.

effulgentsia’s picture

+++ b/core/modules/image/image.admin.inc
@@ -69,54 +70,53 @@ function image_style_form($form, &$form_state, $style) {
+  $effects = Drupal::service('plugin.manager.image.effect')->getDefinitions();
+  uasort($effects, '_image_effect_definitions_sort');

Should we add the sort as a method on the manager, or even make it intrinsic to getDefinitions()?

+++ b/core/modules/image/lib/Drupal/image/Annotation/ImageEffect.php
@@ -0,0 +1,72 @@
+ * Define information about image effects provided by a module.
+ *
+ * This hook enables modules to define image manipulation effects for use with
+ * an image style.
+ *
+ * @return
+ *   An array of image effects. This array is keyed on the machine-readable
+ *   effect name. Each effect is defined as an associative array containing the
+ *   following items:
+ *   - "label": The human-readable name of the effect.
+ *   - "effect_callback": The function to call to perform this image effect.
+ *   - "dimensions_callback": (optional) The function to call to transform
+ *     dimensions for this effect.
+ *   - "help": (optional) A brief description of the effect that will be shown
+ *     when adding or configuring this image effect.
+ *   - "form_callback": (optional) The name of a function that will return a
+ *     $form array providing a configuration form for this image effect.
+ *   - "summary_theme": (optional) The name of a theme function that will output
+ *     a summary of this image effect's configuration.
+ *
+ * @see hook_image_effect_info_alter()

We don't include this on other Annotation objects. The @return especially is wrong.

+++ b/core/modules/image/lib/Drupal/image/Form/ImageEffectAddForm.php
@@ -0,0 +1,36 @@
+/**
+ * @todo.
+ */
+class ImageEffectAddForm extends ImageEffectFormBase {

Here and for other classes, need to fill in these todos with docs.

+++ b/core/modules/image/lib/Drupal/image/ImageEffectBag.php
@@ -0,0 +1,113 @@
+    // If the tip was initialized before, just return.
+    if (isset($this->pluginInstances[$instance_id])) {

Copy/paste error for the comment. Speaking of, there seems to be a lot of generic code in this class. Can some of it move to a base class?

Status: Needs review » Needs work
Issue tags: -Plugin-conversion

The last submitted patch, image-effect-1821854-22.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Plugin-conversion

#22: image-effect-1821854-22.patch queued for re-testing.

Berdir’s picture

\locale\Tests\LocaleUpdateCronTest failed, seen that before, looks like a random fail but weren't able to reproduce yet, maybe a timing thing.

tim.plunkett’s picture

This is still a rough WIP, needs review is only for the bot.
Will be done soon-ish.

tim.plunkett’s picture

FileSize
45.74 KB
121.58 KB

Okay, now this is ready for a real review.

tim.plunkett’s picture

tim.plunkett’s picture

Tagging since this is actually part of #1971384: [META] Convert page callbacks to controllers

tim.plunkett’s picture

FileSize
1.76 KB
121.62 KB

Fixing a couple things dawehner and alexpott pointed out in IRC.

tim.plunkett’s picture

dawehner’s picture

+++ b/core/modules/image/image.admin.incundefined
@@ -69,54 +70,55 @@ function image_style_form($form, &$form_state, $style) {
+      '#markup' => check_plain($effect->label()),

Nitpick: use String::checkpPlain

+++ b/core/modules/image/lib/Drupal/image/Form/ImageEffectEditForm.phpundefined
@@ -0,0 +1,52 @@
+    // If the weight is not specified in the URL, use the default.
+    if ($this->imageEffect->getWeight() !== '' && !$request->query->has('weight')) {
+      $form['weight']['#value'] = $this->imageEffect->getWeight();
+    }

+++ b/core/modules/image/lib/Drupal/image/Form/ImageEffectFormBase.phpundefined
@@ -0,0 +1,123 @@
+      '#value' => $request->query->has('weight') ? intval($request->query->get('weight')) : count($this->imageStyle->getEffects()),

I don't really understand why these two code parts can't be moved together into one ....

+++ b/core/modules/image/lib/Drupal/image/Form/ImageEffectFormBase.phpundefined
@@ -0,0 +1,123 @@
+    if (!$this->imageEffect->hasForm()) {
+      return new RedirectResponse(url('admin/config/media/image-styles/manage/' . $image_style->id(), array('absolute' => TRUE)));
+    }

It seems to be that a thrown exception seems to be better. Maybe a 404 one.

+++ b/core/modules/image/lib/Drupal/image/Form/ImageEffectFormBase.phpundefined
@@ -0,0 +1,123 @@
+    $form['#attached']['css'][drupal_get_path('module', 'image') . '/css/image.admin.css'] = array();

Do you know whether there is a follow up to provide a library for that css file already?

+++ b/core/modules/image/lib/Drupal/image/Plugin/ImageEffect/CropImageEffect.phpundefined
@@ -0,0 +1,84 @@
+        'left-top'      => t('Top') . ' ' . t('Left'),
+        'center-top'    => t('Top') . ' ' . t('Center'),
+        'right-top'     => t('Top') . ' ' . t('Right'),
+        'left-center'   => t('Center') . ' ' . t('Left'),
+        'center-center' => t('Center'),
+        'right-center'  => t('Center') . ' ' . t('Right'),
+        'left-bottom'   => t('Bottom') . ' ' . t('Left'),
+        'center-bottom' => t('Bottom') . ' ' . t('Center'),
+        'right-bottom'  => t('Bottom') . ' ' . t('Right'),

It would be cool to fix this odd indentation directly.

claudiu.cristea’s picture

It would be cool to fix this odd indentation directly.

Not only indentation. Each corner is present 3 times and center 5 times. t() is called 17 times when it should be called only 5 times :)

$top = t('Top');
$left = t('Left');
...
     'left-top' => $top . ' ' . $left,
tim.plunkett’s picture

FileSize
6.12 KB
121.53 KB

I honestly think those should be t('Center Left') so there is proper context, but I'm not changing it to that or changing it to reuse of a t()'d variable without checking with Gabor first. I did remove the whitespace, and addressed the rest of @dawehner's feedback.

There is no CSS follow-up that I know of.

tim.plunkett’s picture

I've rerolled this on top of #2027423: Make image style system flexible as well, since it all touched the same code and that was RTBC.

tim.plunkett’s picture

FileSize
166.19 KB

Here is the combined patch, just to ensure I did it right.

Status: Needs review » Needs work
Issue tags: -FormInterface, -WSCCI-conversion, -Plugin-conversion

The last submitted patch, image-1821854-37.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review

#37: image-1821854-37.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +FormInterface, +WSCCI-conversion, +Plugin-conversion

The last submitted patch, image-1821854-37.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
167.55 KB
1.36 KB

Some new calls to image_style_url in rdf tests.
Surely this is more than normal?

tim.plunkett’s picture

Status: Needs review » Postponed

Yeah I fixed that in the blocker issue. Ill post a link to the sandbox when I'm at my computer.

tim.plunkett’s picture

This is being worked on in http://drupalcode.org/sandbox/tim.plunkett/1698392.git/shortlog/refs/hea... and being rerolled on top of #2027423: Make image style system flexible
I'll post a new patch as soon as that's in.

tim.plunkett’s picture

Status: Postponed » Needs review
FileSize
119.31 KB

That went in. This has no blockers now!

tim.plunkett’s picture

Tweaked some naming to mimic the new names of things after the blocker went in, and updated hook_image_effect_info_alter().

I've attached another version of the patch without the renaming of ieid to uuid and name to id.
It's non-functional, it just is to cut down on patch size (10% or so).

ParisLiakos’s picture

great cleanup!
i mostly went through this, got some points to make:

+++ b/core/modules/image/image.api.phpundefined
@@ -11,52 +11,10 @@
- *   - "help": (optional) A brief description of the effect that will be shown
- *     when adding or configuring this image effect.
...
-    'help' => t('Resize an image to an exact set of dimensions, ignoring aspect ratio.'),

+++ b/core/modules/image/lib/Drupal/image/Annotation/ImageEffect.phpundefined
@@ -0,0 +1,53 @@
+   * The human-readable name of the type.
+   *
+   * @ingroup plugin_translatable
+   *
+   * @var \Drupal\Core\Annotation\Translation
+   */
+  public $help = '';

+++ b/core/modules/image/lib/Drupal/image/Plugin/ImageEffect/ScaleImageEffect.phpundefined
@@ -0,0 +1,88 @@
+ *   help = @Translation("Scaling will maintain the aspect-ratio of the original image. If only a single dimension is specified, the other dimension will be calculated.")

this has wrong docs, probably copied-pasted from the $label property.
While we are here though: can we change that to "description" ? i know this is just conversion, just asking though, it never hurts, so feel free to just say no:P

+++ b/core/modules/image/lib/Drupal/image/Annotation/ImageEffect.phpundefined
@@ -0,0 +1,53 @@
+  public $no_form = FALSE;

+++ b/core/modules/image/lib/Drupal/image/ImageEffectBase.phpundefined
@@ -0,0 +1,126 @@
+  public function hasForm() {
+    return !$this->pluginDefinition['no_form'];
+  }

i also dont like "no_form" which is introduced here..Maybe call it configurable? or dont have it at all, default to FALSE on the hasForm base and let each plugin individually set it to TRUE overriding it if needed?

+++ b/core/modules/image/lib/Drupal/image/Form/ImageEffectFormBase.phpundefined
@@ -0,0 +1,125 @@
+      '#value' => $request->query->has('weight') ? intval($request->query->get('weight')) : $this->imageEffect->getWeight(),

could we switch this intval() to (int) since we are here if possible?

tim.plunkett’s picture

FileSize
14.63 KB
119.99 KB

I hated how I did no_form/hasForm() as well, thanks to discussing it earlier today I realized I should mimic Actions and have a separate interface.
Also made the other changes.

Status: Needs review » Needs work

The last submitted patch, image-1821854-47.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
855 bytes
119.98 KB

!!!

ParisLiakos’s picture

Status: Needs review » Needs work

looks great now, thanks:D

I did some manual testing, all looks good..i think only those nitpicks are left:

+++ b/core/modules/image/lib/Drupal/image/Form/ImageEffectAddForm.phpundefined
@@ -0,0 +1,79 @@
+   * @return array|\Symfony\Component\HttpFoundation\RedirectResponse
...
+  public function buildForm(array $form, array &$form_state, Request $request = NULL, ImageStyleInterface $image_style = NULL, $image_effect = NULL) {

+++ b/core/modules/image/lib/Drupal/image/Form/ImageEffectEditForm.phpundefined
@@ -0,0 +1,47 @@
+   * @return array|\Symfony\Component\HttpFoundation\RedirectResponse
+   *   The form structure.
...
+  public function buildForm(array $form, array &$form_state, Request $request = NULL, ImageStyleInterface $image_style = NULL, $image_effect = NULL) {

+++ b/core/modules/image/lib/Drupal/image/Form/ImageEffectFormBase.phpundefined
@@ -0,0 +1,126 @@
+      throw new NotFoundHttpException();

needs updating to reflect that we through 404 now, which i agree btw

+++ b/core/modules/image/lib/Drupal/image/ImageEffectInterface.phpundefined
@@ -0,0 +1,102 @@
+  public function applyEffect($image);

maybe typehint?

+++ b/core/modules/image/lib/Drupal/image/Plugin/Core/Entity/ImageStyle.phpundefined
@@ -77,7 +79,12 @@ class ImageStyle extends ConfigEntityBase implements ImageStyleInterface {
+  /**
+   * @var \Drupal\image\ImageEffectBag
+   */
+  protected $effectsBag;

probably needs some boilerplate

+++ b/core/modules/image/lib/Drupal/image/Plugin/ImageEffect/ResizeImageEffect.phpundefined
@@ -0,0 +1,79 @@
+ * Contains \Drupal\image\Plugin\ImageEffect\Resize.
...
+class ResizeImageEffect extends ImageEffectBase implements ConfigurableImageEffectInterface {

wrong classname in Contains docs

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageStyleFlushTest.phpundefined
@@ -100,11 +100,11 @@ function testFlush() {
diff --git a/core/modules/image/tests/image_module_test.module b/core/modules/image/tests/image_module_test.module

+++ /dev/nullundefined
diff --git a/core/modules/image/tests/image_module_test.info.yml b/core/modules/image/tests/modules/image_module_test/image_module_test.info.yml
diff --git a/core/modules/image/tests/modules/image_module_test/image_module_test.module b/core/modules/image/tests/modules/image_module_test/image_module_test.module

+1

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.17 KB
119.39 KB

Thanks for the review!

Those other docblocks were duplicated when I was passing different args, now they're the same so I can delete it.

I can't typehint on applyEffect() until #2033669: Image file objects should be classed. It's handled there.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

cool, only docs change so i believe will come back green. RTBC imo

tim.plunkett’s picture

dawehner’s picture

#51: image-1821854-51.patch queued for re-testing.

tim.plunkett’s picture

tim.plunkett’s picture

Priority: Normal » Critical
catch’s picture

Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs review
Issue tags: -Approved API change

Sorry why is this critical? I don't mind if we ship 8.x with some info hooks.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Approved API change

Status change was probably unintentional.

catch’s picture

effulgentsia’s picture

Issue tags: -Approved API change

Agreed this shouldn't have the "Approved API change" tag without a core committer actually approving it. But I agree with #58 that there's nothing in #57 that explains why this shouldn't be RTBC. In other words, the patch appears to be reviewed and tested by the community, but whether it's eligible for D8 inclusion requires core committer decision.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

Someone else can go through and untag the rest and unassign me from all of them.

xjm’s picture

Issue tags: +API change

Let's get the API change part tagged, though.

claudiu.cristea’s picture

tim.plunkett’s picture

FileSize
119.83 KB

Rerolled for #1987712: Convert file_download() to a new style controller (just conflicts in services.yml and routing.yml)

alexpott’s picture

Title: Convert image effects into plugins » Change notice: Convert image effects into plugins
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record, +Approved API change

Let's get this is in... whilst this is not release blocking and we could release Drupal 8 with this as an info hook I think that image effects are a natural fit with the plugin system and this increases the consistency across core. Furthermore, it opens up the possibility of completing #2033669: Image file objects should be classed which is a good thing. Using stdClass should be frowned on now that Drupal has embraced OO... it's like using arrays for APIs... in the long run unmaintainable.

Committed a48f0d8 and pushed to 8.x. Thanks!

claudiu.cristea’s picture

claudiu.cristea’s picture

effulgentsia’s picture

Issue tags: -blocker

The patch was committed, so this issue isn't a blocker for anything anymore.

slashrsm’s picture

Assigned: Unassigned » slashrsm

Working on change notice.

slashrsm’s picture

Title: Change notice: Convert image effects into plugins » Convert image effects into plugins
Assigned: slashrsm » Unassigned
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record +Configuration system, +Plugin system, +Configurables
slashrsm’s picture

Remove unintentionally added tags....

Gábor Hojtsy’s picture

Status: Fixed » Needs work

I see there are at least two key name changes in the image styles config .yml file and no updates to the schema of those have happened. Would be important that we keep the schema updated as config structure is changing so we don't leave the schema system broken. Yeah, we don't have test coverage to ensure all the data has schema coverage but that is not yet possible due to outstanding coverage for some plugins / views formatters, etc. So we need to rely on human attention for now that it is kept up to date.

Gábor Hojtsy’s picture

Status: Needs work » Fixed

Don't know how did I miss this, but the schema was actually updated in the patch... http://drupalcode.org/project/drupal.git/commitdiff/a48f0d889c399db88287...

Automatically closed -- issue fixed for 2 weeks with no activity.