Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
The service definitions are already in core.services.yml instead of system, we should move the classes as well.
Comment | File | Size | Author |
---|---|---|---|
#25 | move-toolkit-2048827-25.patch | 14.41 KB | claudiu.cristea |
#25 | interdiff.txt | 537 bytes | claudiu.cristea |
#23 | move-toolkit-2048827-23.patch | 14.28 KB | claudiu.cristea |
#14 | move-toolkit-2048827-14.patch | 17.49 KB | claudiu.cristea |
#14 | interdiff.txt | 876 bytes | claudiu.cristea |
Comments
Comment #1
tim.plunkettComment #3
tim.plunkettSome tests were manually creating the image toolkit manager, no point.
Comment #4
claudiu.cristeaThe patch is moving the interface and the manager to Drupal\Core but still keep the GD implementation in the system module. An explanation may be the fact that GD is the implementation shipped with Drupal. But I see the general tests left in system (ToolkitTestBase & ToolkitTest). Shouldn't those be moved to Core as well?
Comment #5
claudiu.cristeaJust ignore my latest comment (no location for tests).
I reworked a little bit and rerolled this. I'm taking this so that I can track. It was hard to get an interdiff.
Comment #6
mondrakeimage.toolkit.manager
toplugin.manager.image_toolkit
?Comment #7
claudiu.cristeaThere's no need to switch to "needs work".
All plugin manager services are prefixed with
plugin.manager.
. It's simply a naming consistency reason and here we switch image tookit plugin manager to this standard. See how other plugin manager services are named.Well, in #4 I asked the same thing but I also found the answer there. It makes perfectly sense to provide the image system foundation in core. And then the implementation is provided by modules. But because Drupal is shipped with a default toolkit implementation (GD) and that one has to be installed by default when Drupal is installed, it is provided by
system.module
. Other toolkits may be implemented in contrib modules, for symmetry the default implementation should be provided in the same way, by a module.Comment #8
tim.plunkettRemoved some of the out of scope changes. This is a simple move of the API, let's keep it that way. Any moving of GS can be done in a separate issue.
Comment #9
mondrakeSimple move of API files / change of namespaces. Feedback addressed. RTBC for me.
Comment #10
mondrakeOops no, sorry, this is a regression - missed a pull maybe?
Comment #11
claudiu.cristeaThis is a regression. The "type" key has been introduced recently, in #2066219: Decouple image type from image extension and "extension" was dropped.
Comment #12
claudiu.cristeaSorry. Cross-posting on the same issue :)
Comment #13
claudiu.cristeaBut anyway, @tim.plunkett, what was out-of-scope in #5? I cannot see without an interdiff.
Comment #14
claudiu.cristeaFixed #10 (#11).
Comment #15
tim.plunkettInjecting the config factory, mostly.
Comment #16
claudiu.cristea@tim.plunkett
Oh, yes, totally forgot about that :)
Comment #17
mondrakeSo, RTBC now.
Comment #18
claudiu.cristeaFollowup #2105621: Inject config factory service into image toolkit manager.
Comment #19
tim.plunkettCrosspost
Comment #20
webchickSo, yay for removing "Plugin" from the class location. As a module dev, I need to know that there are image toolkits and how to work with them, I do not need to know nor care how image toolkits are implemented under the hood.
But then boo, because then we *undo* that very same change in the machine name by adding "plugin.manager" to it. :(
Can we back out that change? If this really is a standard in core, I'd like to challenge it, because it's violating the Law of Demeter.
Otherwise, looks good!
Comment #21
webchickComment #22
dawehner#2100313: Move non-core services out of core.services.yml
Comment #23
claudiu.cristeaDropped service renaming as it's not in the scope. We need this in asap otherwise we'll be forced to do too many rerolls in issues from #2105863: [meta] Images, toolkits and operations.
If renaming the manager is important I will open a new issue only for that.
Comment #25
claudiu.cristeaOuch!
Comment #26
mondrakeback to RTBC
Comment #27
webchickAwesome, thank you!
Committed and pushed to 8.x.
Comment #28
claudiu.cristeaThank you. Unfortunately we forgot to move
\Drupal\system\Annotation\ImageToolkit
too along with this relocation. Shame!Opened a followup in #2108077: ImageToolkit annotation object left under system module.