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.
Files: 
CommentFileSizeAuthor
#64 image-1821854-64.patch119.83 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,215 pass(es).
[ View ]
#51 image-1821854-51.patch119.39 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,185 pass(es).
[ View ]
#51 interdiff.txt3.17 KBtim.plunkett
#49 image-1821854-49.patch119.98 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,909 pass(es).
[ View ]
#49 interdiff.txt855 bytestim.plunkett
#47 image-1821854-47.patch119.99 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,826 pass(es), 59 fail(s), and 456 exception(s).
[ View ]
#47 interdiff.txt14.63 KBtim.plunkett
#45 image-1821854-45.patch119.22 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,849 pass(es).
[ View ]
#45 interdiff.txt10.91 KBtim.plunkett
#45 image-1821854-45-easyreview-do-not-test.patch107.37 KBtim.plunkett
#44 image-1821854-44.patch119.31 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,846 pass(es).
[ View ]
#41 image-plugin.interdiff.txt1.36 KBlarowlan
#41 image-plugin.patch167.55 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 56,862 pass(es).
[ View ]
#37 image-1821854-37.patch166.19 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,842 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#35 image-1821854-35.patch121.53 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,829 pass(es).
[ View ]
#35 interdiff.txt6.12 KBtim.plunkett
#31 image-effect-1821854-31.patch121.62 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,763 pass(es).
[ View ]
#31 interdiff.txt1.76 KBtim.plunkett
#28 image-effect-1821854-28.patch121.58 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,882 pass(es).
[ View ]
#28 interdiff.txt45.74 KBtim.plunkett
#22 image-effect-1821854-22.patch115.81 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 56,650 pass(es).
[ View ]
#22 interdiff.txt790 byteseffulgentsia
#20 image-effect-1821854-20.patch115.71 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,648 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#18 image-effect-1821854-18.patch98.68 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,559 pass(es), 2 fail(s), and 108 exception(s).
[ View ]
#18 interdiff.txt32.18 KBtim.plunkett
#16 image-effect-1821854-16.patch77.18 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,742 pass(es), 3 fail(s), and 9 exception(s).
[ View ]
#16 interdiff.txt54.66 KBtim.plunkett
#14 image-effect-1821854-14.patch46.9 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,427 pass(es), 51 fail(s), and 26 exception(s).
[ View ]

Comments

Definitely needed, and should be fairly simple.

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

@yched do you mean that issue should be postponed while #1764380: Merge PluginSettingsInterface into ConfigurablePluginInterface 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

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

Assigned:Unassigned» tim-e

Issue tags:-Plugin system

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

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

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

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.

Related: #2027423: Make image style system flexible

Will provide a patch today.

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

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

Priority:Normal» Critical

Priority:Critical» Normal

Oops got confused by @timplunkett

Assigned:Unassigned» tim.plunkett

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

Status:Active» Needs review
StatusFileSize
new46.9 KB
FAILED: [[SimpleTest]]: [MySQL] 56,427 pass(es), 51 fail(s), and 26 exception(s).
[ View ]

Work in progress, just posting something.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new54.66 KB
new77.18 KB
FAILED: [[SimpleTest]]: [MySQL] 56,742 pass(es), 3 fail(s), and 9 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new32.18 KB
new98.68 KB
FAILED: [[SimpleTest]]: [MySQL] 56,559 pass(es), 2 fail(s), and 108 exception(s).
[ View ]

More fixes. Form converted. PluginBag up next.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new115.71 KB
FAILED: [[SimpleTest]]: [MySQL] 56,648 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Lost track of my interdiffs, sorry.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new790 bytes
new115.81 KB
PASSED: [[SimpleTest]]: [MySQL] 56,650 pass(es).
[ View ]

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

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

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

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

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

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

StatusFileSize
new45.74 KB
new121.58 KB
PASSED: [[SimpleTest]]: [MySQL] 56,882 pass(es).
[ View ]

Okay, now this is ready for a real review.

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

StatusFileSize
new1.76 KB
new121.62 KB
PASSED: [[SimpleTest]]: [MySQL] 56,763 pass(es).
[ View ]

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

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

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

<?php
$top
= t('Top');
$left = t('Left');
...
    
'left-top' => $top . ' ' . $left,
?>

StatusFileSize
new6.12 KB
new121.53 KB
PASSED: [[SimpleTest]]: [MySQL] 56,829 pass(es).
[ View ]

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.

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.

StatusFileSize
new166.19 KB
FAILED: [[SimpleTest]]: [MySQL] 56,842 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new167.55 KB
PASSED: [[SimpleTest]]: [MySQL] 56,862 pass(es).
[ View ]
new1.36 KB

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

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.

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.

Status:Postponed» Needs review
StatusFileSize
new119.31 KB
PASSED: [[SimpleTest]]: [MySQL] 56,846 pass(es).
[ View ]

That went in. This has no blockers now!

StatusFileSize
new107.37 KB
new10.91 KB
new119.22 KB
PASSED: [[SimpleTest]]: [MySQL] 56,849 pass(es).
[ View ]

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

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?

StatusFileSize
new14.63 KB
new119.99 KB
FAILED: [[SimpleTest]]: [MySQL] 56,826 pass(es), 59 fail(s), and 456 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new855 bytes
new119.98 KB
PASSED: [[SimpleTest]]: [MySQL] 56,909 pass(es).
[ View ]

!!!

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

Status:Needs work» Needs review
StatusFileSize
new3.17 KB
new119.39 KB
PASSED: [[SimpleTest]]: [MySQL] 57,185 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

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

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

Priority:Normal» Critical

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.

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

Status change was probably unintentional.

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.

Assigned:tim.plunkett» Unassigned

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

Issue tags:+API change

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

StatusFileSize
new119.83 KB
PASSED: [[SimpleTest]]: [MySQL] 57,215 pass(es).
[ View ]

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

Title:Convert image effects into pluginsChange 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!

Issue tags:-blocker

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

Assigned:Unassigned» slashrsm

Working on change notice.

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

Remove unintentionally added tags....

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.

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.