Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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().
Comment | File | Size | Author |
---|---|---|---|
#13 | image-2251223-13.patch | 19.19 KB | tim.plunkett |
#4 | image-2251223-4.patch | 19.84 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettComment #2
tim.plunkettOkay, worked that out.
Comment #4
tim.plunkettComment #5
tim.plunkettThe 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.
Comment #6
mondrakecan 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.
Comment #7
tim.plunkettI 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).
Comment #8
mondrakeOK then, I'll try and test it and see to adjust Textimage accordingly. Setting back to CNR for anyone else to review.
Comment #9
tstoecklerIs there a specific reason for providing this? I.e. FormBase provides an empty validateForm() but no submitForm().
Why not pass $form here?
Comment #10
tim.plunkett1) 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:
And if we pass in the full form, it will be duplicated inside $form['data'].
Comment #11
mondrakeSo 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.
Comment #12
tim.plunkettI left some @todos in there, need to fix those.
Comment #13
tim.plunkettRerolled for PSR-4. It'd be nice to get this in.
Comment #14
mondrakeRTBC per comments in #11, also a leftover todo was removed.
Comment #16
mondrake13: image-2251223-13.patch queued for re-testing.
Comment #17
mondrakeRTBC if it comes back green. Several testbot failures these days :(
Comment #18
alexpottCommitted 6bf9738 and pushed to 8.x. Thanks!
Comment #20
tim.plunkettThanks!