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.
Issue Summary
Part of #1696224: Remove system_settings_form()
This issue focuses on converting the following variables to configuration:
- image_jpeg_quality in
image_gd_settings()
- image_toolkit in
system_image_toolkit_settings()
Comment | File | Size | Author |
---|---|---|---|
#17 | image_toolkit_unneeded_check.patch | 2.59 KB | slv_ |
#11 | drupal-image_toolkit-1712258-11.patch | 6.49 KB | disasm |
#8 | 1712258-8.image_toolkit.patch | 7.01 KB | Shuairan |
#7 | 4-7-interdiff.txt | 2.2 KB | alexpott |
#7 | 1712258-7.image_toolkit.patch | 6.98 KB | alexpott |
Comments
Comment #1
sunComment #2
dagmarThanks for create the issue, I was working on this already, here is the first patch.
Comment #3
sunThanks! :)
This issue was and still is assigned to @Shuairan, so I hope this patch isn't too different to his work and he'll be able to merge it into his work.
1) Missing newline between summary and @see.
2) Missing () after function name in @see.
This should point to the submit handler instead.
It looks like we need to retain this emptying-out of system.image:toolkit.
Let's put initiate a
$config = config('system.image');
at the beginning of the function.hmmm, this looks wonky. Did you intend to assign
?
In general, we might have to change the actual $form structure and processing logic here.
Essentially, the $form['image_toolkit'] element should always be part of the form, but using:
Given that the toolkit settings are separated from the particular toolkit's settings, I think we want to actually have two configuration objects; i.e.:
system.image
+
system.image.gd
I'm not 100% sure on that yet, but I think it makes sense.
Comment #4
alexpottI agree with @sun each toolkit should have it's own config file.
I've left the config->set in system_image_toolkit_settings() as this preserves the original functionality.
Additionally this patch fixes running the image tests under a minimal install as this did not work because the image module is not enabled as part of the profile (but it is in standard so the interesting thing here is that the testbots on qa.d.o are using the standard install)
Comment #5
sunI do not really understand these changes, or rather, why they are relevant for this patch?
ok... I'm afraid - this form is extremely wonky. Saving configuration on VIEW is an absolute no-go.
I wonder whether any tests are going to break if we change this condition to NOT save but instead change the #default_value accordingly?
In the end, the form can still be submitted when this condition is triggered, so we definitely need to set the #default_value, because the element might have a bogus/different #default_value but #access FALSE.
I expected to see this code using the submitted value for the toolkit.
This indirection is a bit concerning... I do not really see where image_get_toolkit() is actually loading any toolkit include files...?
Also, shouldn't the includes be loaded already, since the form constructor needs to call into the toolkit in the first place already?
Comment #6
alexpottimage_get_toolkits() does the include when it calls image_get_available_toolkits() which does this
module_invoke_all('image_toolkits');
which calls system_image_toolkits().I agree with you the image toolkit code is extremely wonky... and ripe for OO and plugin-ifcation.
As for
As I wrote in #4...
This caused me a serious waste of time... and the fix seemed trivial and correct. Happy to create a separate patch if you like.
Comment #7
alexpottAfter chatting to @sun on irc we felt that hiding the radio buttons when there is only one option is unnecessarily helpful.
Additionally, PHP's GD library is now required by the installer and is checked in system_requirements() so the current variable_del if there are no toolkits is no longer required.
Patch attached makes the appropriate changes.
However...
The following steps will lead to a PHP error if you use the patch from #4.
The patch attached fixes this by checking if the form value is set I think the UX kind of sucks... double saving... to change to an image toolkit and then change the select image toolkit's settings.
Comment #8
Shuairan CreditAttribution: Shuairan commentedChanged the patch to load always all image toolkit subforms and only show the current fieldset via form api states.
This improves the UX, but will produce inconsistend behaviour for JS-disabled browsers, because only the settings for the current selected toolkit will get saved (could be improved)
Besides it may be possible that an image toolkits overwrites the form-settings form another by using the same name for an form value, this means "image_jpeg_quality" should be "image_gd_jpeg_quality" to avoid that...
But this seems all to be very hacky, I also think implementing image toolkits as plugins would be a much better solution.
Comment #10
alexpottFor work on converting image toolkits to plugins see #1664844: Convert image toolkits into plugins
Comment #11
disasm CreditAttribution: disasm commentedstart with a reroll of #8.
Comment #12
disasm CreditAttribution: disasm commentedComment #13
disasm CreditAttribution: disasm commentedWhile working on this in windsprints, I got the patch rerolled above. I also agree with #8 that the current patch is very hacky with the ['#states']['visible']. After reviewing the issue linked in #10, I believe that this should be postponed until image toolkits is converted to a plugin. As such, I'm marking this status as postponed until further activity on #1664844: Convert image toolkits into plugins.
Comment #14
jibranAs per #13
Comment #15
gdd#11: drupal-image_toolkit-1712258-11.patch queued for re-testing.
Comment #17
slv_ CreditAttribution: slv_ commentedThis has been already fixed in #1664844: Convert image toolkits into plugins and no longer applies. Nor does the form_set_error when there are no toolkits present, since as alexpott pointed out in #7, GD library is required by the installer, so there will be always at least 1 toolkit available.
I'm attaching a patch removing those unneeded bits of code. Hope it helps.
Comment #18
disasm CreditAttribution: disasm commentedIf testbot likes the patch, I like the changes. It makes the code much cleaner. However; I don't see anything related to CMI in the patch. If the CMI is already handled in #1664844: Convert image toolkits into plugins we should probably change the issue title and update the issue summary to match what's actually being done in the patch. Something like simplify image toolkit form logic.
Comment #19
andypost#17 could be merged into #11 to make form smaller
Comment #20
slv_ CreditAttribution: slv_ commented@andypost, the thing is that #11 no longer applies, because this changes to convert the form to the configuration system were made already in #1664844: Convert image toolkits into plugins.
Changing the issue title as suggested by disasm.
Comment #21
andypostAs no changes in form structure it's just a clean-up that's ready
Comment #22
catchMakes sense. Committed/pushed to 8.x.