Problem/Motivation

When plugins provide forms, they use PluginFormInterface.
ImageEffect predate this interface, we need to update to it.

Proposed resolution

Use existing interfaces

Remaining tasks

User interface changes

API changes

Switch from getForm() to buildConfigurationForm().

CommentFileSizeAuthor
#13 image-2251223-13.patch19.19 KBtim.plunkett
#4 image-2251223-4.patch19.84 KBtim.plunkett
#2 image-2251223-2.patch15.34 KBtim.plunkett
#1 image-2251223-1.patch9.3 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Related issues: +#2065485: Document that PluginFormInterface should use #process to solve nesting issues
FileSize
image-2251223-1.patch9.3 KB
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Issue summary: View changes
FileSize
image-2251223-2.patch15.34 KB

Okay, worked that out.

Status: Needs review » Needs work

The last submitted patch, 2: image-2251223-2.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
image-2251223-4.patch19.84 KB
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: tim.plunkett » Unassigned

The short story here is: ImageStyle::saveImageEffect was overpowered, and would prevent any plugins from manipulating their form values before saving.
Splitting that up makes for a cleaner set of steps, and more flexible plugins that are consistent with core.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs review » Needs work 
+++ b/core/modules/image/lib/Drupal/image/ImageEffectBag.php
@@ -24,29 +24,6 @@ public function &get($instance_id) {
-   * Updates the configuration for an image effect instance.
-   *
-   * If there is no plugin instance yet, a new will be instantiated. Otherwise,
-   * the existing instance is updated with the new configuration.
-   *
-   * @param array $configuration
-   *   The image effect configuration to set.
-   *
-   * @return string
-   *   The image effect UUID.
-   */
-  public function updateConfiguration(array $configuration) {

can we keep this method in ImageEffectBag? It comes handy (in contrib) to create dynamically at runtime image styles out of an array of effects. See e.g. Textimage.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

I moved that into ImageStyle::addImageEffect, where it actually belonged.

It wasn't actually properly updating the image effects, it was just adding them.

EDIT

We used to have ImageStyle::saveImageEffect(), which called ImageEffectBag::updateConfiguration().
I moved the innards of updateConfiguration to saveImageEffect, and then renamed saveImageEffect to addImageEffect, moving the save() call out.
That way each method actually does what it says, and the ImageEffectBag doesn't have to worry about generating UUIDs anymore (it shouldn't ever have).

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs work » Needs review

OK then, I'll try and test it and see to adjust Textimage accordingly. Setting back to CNR for anyone else to review.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented 
  1. +++ b/core/modules/image/lib/Drupal/image/ConfigurableImageEffectBase.php
@@ -0,0 +1,27 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function submitConfigurationForm(array &$form, array &$form_state) {
+  }

    Is there a specific reason for providing this? I.e. FormBase provides an empty validateForm() but no submitForm(). 

  2. +++ b/core/modules/image/lib/Drupal/image/Form/ImageEffectFormBase.php
@@ -77,7 +77,7 @@ public function buildForm(array $form, array &$form_state, ImageStyleInterface $
+    $form['data'] = $this->imageEffect->buildConfigurationForm(array(), $form_state);

    Why not pass $form here?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

1) The only reason is so that subclasses call parent::submitConfigurationForm, in case we need to add something. I don't care much either way.

2) Yep, that is borrowed directly from blocks. Because this is valid:

function buildConfigurationForm($form, &$form_state) {
  return $form;

And if we pass in the full form, it will be duplicated inside $form['data'].

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented

So I manually tested #4 thoroughly, and also managed to get Textimage work with it (it implements 3 effects, plus changed its use of ImageEffectBag::updateConfiguration to use ImageStyle::addImageEffect instead). All good. I'd say RTBC but leave it to @tstoeckler given discussion in #9 and #10.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: Unassigned » tim.plunkett
Status: Needs review » Needs work

I left some @todos in there, need to fix those.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
image-2251223-13.patch19.19 KB

Rerolled for PSR-4. It'd be nice to get this in.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs review » Reviewed & tested by the community

RTBC per comments in #11, also a leftover todo was removed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: image-2251223-13.patch, failed testing.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs work » Needs review

13: image-2251223-13.patch queued for re-testing.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs review » Reviewed & tested by the community

RTBC if it comes back green. Several testbot failures these days :(

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 6bf9738 and pushed to 8.x. Thanks!

  • Commit 6bf9738 on 8.x by alexpott: 
    Issue #2251223 by tim.plunkett: ConfigurableImageEffectInterface should...
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: tim.plunkett » Unassigned

Thanks!

Status: Fixed » Closed (fixed)

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