ImageToolkit plugins can easily be made autowirable, but ImageToolkitOperation ones less so as its manager passes the toolkit (that is not a 'proper' service) to the operation and overrides the createInstance() method.
The Imagemagick toolkit requires several services in their operations, and it's currently not possible to autowire there.
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !13926
Comments
Comment #3
mondrakeComment #4
mondrakeComment #5
smustgrave commentedleft a comment. If agreed to do the deprecation then a simple CR maybe?
Comment #6
mondrakeComment #7
smustgrave commentedLooks good! Only moving to NW for manual rebase for ImageTest
Comment #8
mondrakerebased
Comment #9
mondrakeUhm, now an ImageToolkit may be autowireable, likely along with the ImageToolkitOperations it provides. But an ImageToolkitOperation provided by a separate module may provide more ImageToolkitOperations that will not be autowireable yet. Need to think its impact and in case provide BC.
Comment #10
mondrakeFound a way to make this simpler. We can autowire
ImageToolkitOperationBaseand make the $toolkit argument nullable - in this case the autowiring will pass a null, then we set the actual toolkit on the operation viasetToolkitin the manager.No need to deprecate anything anymore.
Comment #11
mondrakeActually - let's deprecate the $toolkit argument.
Comment #12
mondrakeComment #13
smustgrave commentedAll the typehints and return types in the tests was that needed for the autowire feature?
Comment #14
mondrakeIn ImageTest you mean? Yes, they were needed, can't remember exactly why but for sure we could no longer run the test just with mocking the interface, then GDToolkit had to be mocked instead and then all the rest came along.
Comment #15
smustgrave commentedImageTest was the one I meant, sorry should of mentioned.
Wasn't entirely sure how to best test so just applied the MR locally, cleared cache, and did image uploads with different styles being applied. Everything seemed to work.
Test-only ran here https://git.drupalcode.org/issue/drupal-3559481/-/jobs/7882164 which appears to line up with the tests aded
If this is RTBC too soon I apologize!
Comment #16
longwaveThe argument juggling here is a bit tricky to follow but I think it's correct; I'm not sure how many real world cases there are where the BC will be triggered anyway. Let's ship this in 11.4 and deal with any fallout if it happens.
Committed and pushed a69a313a746 to main and 7a1b1f74737 to 11.x. Thanks!
Comment #21
alexpottThis is causing errors in tests. We can't have readonly service properties on plugins because they cannot be set in a wakeup - see https://issue.pages.drupalcode.org/-/drupal-3564689/-/jobs/8238108/artif...
I do not know why this is not a consistent error yet but this MR is going to cause problems. We need to remove the readonly from the these properties and I think we need to be more careful about adding the readonly property.
Comment #22
mondrakeRevert?
Comment #23
alexpottAddressing the PHP 8.3 break in #3569870: Remove readonly properties from image toolkits