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
- 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.
Comment | File | Size | Author |
---|---|---|---|
#64 | image-1821854-64.patch | 119.83 KB | tim.plunkett |
#51 | image-1821854-51.patch | 119.39 KB | tim.plunkett |
#51 | interdiff.txt | 3.17 KB | tim.plunkett |
#49 | image-1821854-49.patch | 119.98 KB | tim.plunkett |
#49 | interdiff.txt | 855 bytes | tim.plunkett |
Comments
Comment #1
yched CreditAttribution: yched commentedDefinitely 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")
Comment #2
andypost@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
Comment #3
tim.plunkettI 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
Comment #4
tim-e CreditAttribution: tim-e commentedComment #5
effulgentsia CreditAttribution: effulgentsia commented@tim-e: it's been a while since #4. Do you still want this assigned to you?
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedRestoring tag I didn't mean to remove. Also shortening title per #3.
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedNo 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.
Comment #8
claudiu.cristeaRelated: #2027423: Make image style system flexible
Will provide a patch today.
Comment #9
andypostSuppose better to build the issue on top of #2027423: Make image style system flexible
Is there any sandbox for ?
Comment #10
claudiu.cristeaJust create one at https://drupal.org/sandbox/claudiucristea/2028715. It has only the 8.x branch right now
Comment #11
alexpottComment #12
alexpottOops got confused by @timplunkett
Comment #13
tim.plunkettI apparently confused this and image toolkits, I forgot this was undone. :(
Comment #14
tim.plunkettWork in progress, just posting something.
Comment #16
tim.plunkettFinished the basic conversion, now going to revisit how image effects are stored in image styles.
Comment #18
tim.plunkettMore fixes. Form converted. PluginBag up next.
Comment #20
tim.plunkettLost track of my interdiffs, sorry.
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedThis only required a trivial fix to the test, not to the code.
Comment #23
effulgentsia CreditAttribution: effulgentsia commentedShould we add the sort as a method on the manager, or even make it intrinsic to getDefinitions()?
We don't include this on other Annotation objects. The @return especially is wrong.
Here and for other classes, need to fill in these todos with docs.
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?
Comment #25
Berdir#22: image-effect-1821854-22.patch queued for re-testing.
Comment #26
Berdir\locale\Tests\LocaleUpdateCronTest failed, seen that before, looks like a random fail but weren't able to reproduce yet, maybe a timing thing.
Comment #27
tim.plunkettThis is still a rough WIP, needs review is only for the bot.
Will be done soon-ish.
Comment #28
tim.plunkettOkay, now this is ready for a real review.
Comment #29
tim.plunkettOpened #2033383: Provide a default plugin bag
Comment #30
tim.plunkettTagging since this is actually part of #1971384: [META] Convert page callbacks to controllers
Comment #31
tim.plunkettFixing a couple things dawehner and alexpott pointed out in IRC.
Comment #32
tim.plunkettFollow-up #2033669: Image file objects should be classed (optional)
Comment #33
dawehnerNitpick: use String::checkpPlain
I don't really understand why these two code parts can't be moved together into one ....
It seems to be that a thrown exception seems to be better. Maybe a 404 one.
Do you know whether there is a follow up to provide a library for that css file already?
It would be cool to fix this odd indentation directly.
Comment #34
claudiu.cristeaNot 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 :)Comment #35
tim.plunkettI 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.
Comment #36
tim.plunkettI'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.
Comment #37
tim.plunkettHere is the combined patch, just to ensure I did it right.
Comment #39
larowlan#37: image-1821854-37.patch queued for re-testing.
Comment #41
larowlanSome new calls to image_style_url in rdf tests.
Surely this is more than normal?
Comment #42
tim.plunkettYeah I fixed that in the blocker issue. Ill post a link to the sandbox when I'm at my computer.
Comment #43
tim.plunkettThis 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.
Comment #44
tim.plunkettThat went in. This has no blockers now!
Comment #45
tim.plunkettTweaked 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).
Comment #46
ParisLiakos CreditAttribution: ParisLiakos commentedgreat cleanup!
i mostly went through this, got some points to make:
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
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?
could we switch this intval() to (int) since we are here if possible?
Comment #47
tim.plunkettI 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.
Comment #49
tim.plunkett!!!
Comment #50
ParisLiakos CreditAttribution: ParisLiakos commentedlooks great now, thanks:D
I did some manual testing, all looks good..i think only those nitpicks are left:
needs updating to reflect that we through 404 now, which i agree btw
maybe typehint?
probably needs some boilerplate
wrong classname in Contains docs
+1
Comment #51
tim.plunkettThanks 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.
Comment #52
ParisLiakos CreditAttribution: ParisLiakos commentedcool, only docs change so i believe will come back green. RTBC imo
Comment #53
tim.plunkettBlocks #2033669: Image file objects should be classed
Comment #54
dawehner#51: image-1821854-51.patch queued for re-testing.
Comment #55
tim.plunkettAs part of #2044203: [meta] Convert info hooks to plugins
Comment #56
tim.plunkettComment #57
catchSorry why is this critical? I don't mind if we ship 8.x with some info hooks.
Comment #58
tstoecklerStatus change was probably unintentional.
Comment #59
catchDoesn't look like it #2044203: [meta] Convert info hooks to plugins
Comment #60
effulgentsia CreditAttribution: effulgentsia commentedAgreed 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.
Comment #61
tim.plunkettSomeone else can go through and untag the rest and unassign me from all of them.
Comment #62
xjmLet's get the API change part tagged, though.
Comment #63
claudiu.cristeaRelated #2046279: Convert image_style_form() to a new form controller.
Comment #64
tim.plunkettRerolled for #1987712: Convert file_download() to a new style controller (just conflicts in services.yml and routing.yml)
Comment #65
alexpottLet'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!
Comment #66
claudiu.cristeaFollow up? #2062573: Add method defaultConfiguration() in ConfigurablePluginInterface
Comment #67
claudiu.cristeaAnd a related bug #2063403: Fatal when editing a nonexistent image effect.
Comment #68
effulgentsia CreditAttribution: effulgentsia commentedThe patch was committed, so this issue isn't a blocker for anything anymore.
Comment #69
slashrsm CreditAttribution: slashrsm commentedWorking on change notice.
Comment #70
slashrsm CreditAttribution: slashrsm commentedChange record: http://drupal.org/node/2072699.
Comment #71
slashrsm CreditAttribution: slashrsm commentedRemove unintentionally added tags....
Comment #72
Gábor HojtsyI 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.
Comment #73
Gábor HojtsyDon't know how did I miss this, but the schema was actually updated in the patch... http://drupalcode.org/project/drupal.git/commitdiff/a48f0d889c399db88287...