Simplify image toolkit form logic, removing unneeded checks.

Thanks for create the issue, I was working on this already, here is the first patch.

Thanks! :)

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.

+++ b/core/modules/system/image.gd.inc
@@ -38,6 +38,16 @@ function image_gd_settings() {
+ * Form submission handler for image_gd_settings().
+ * @see system_image_toolkit_settings_submit
+ */
+function image_gd_settings_submit($form, &$form_state) {

1) Missing newline between summary and @see.

2) Missing () after function name in @see.

+++ b/core/modules/system/system.admin.inc
@@ -1854,14 +1854,13 @@ function system_file_system_settings() {
- * @see system_settings_form()
+ * @see system_config_form()

This should point to the submit handler instead.

+++ b/core/modules/system/system.admin.inc
@@ -1854,14 +1854,13 @@ function system_file_system_settings() {
   if (count($toolkits_available) == 0) {
-    variable_del('image_toolkit');

It looks like we need to retain this emptying-out of system.image:toolkit.

+++ b/core/modules/system/system.admin.inc
@@ -1872,12 +1871,16 @@ function system_image_toolkit_settings() {
-      '#default_value' => variable_get('image_toolkit', $current_toolkit),
+      '#default_value' => config('system.image')->get('toolkit'),

Let's put initiate a $config = config('system.image'); at the beginning of the function.

+++ b/core/modules/system/system.admin.inc
@@ -1872,12 +1871,16 @@ function system_image_toolkit_settings() {
   else {
-    variable_set('image_toolkit', key($toolkits_available));
+    $form['image_toolkit'] = array(
+      '#type' => 'value',
+      '#value' => $current_toolkit,
+    );
+    config('system.image')->set('toolkit', key($toolkits_available))->save();
   }

hmmm, this looks wonky. Did you intend to assign

$current_toolkit = key($toolkits_available);

?

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:

  '#access' => count($toolkits_available) > 1,

+++ b/core/modules/system/system.admin.inc
@@ -1885,8 +1888,23 @@ function system_image_toolkit_settings() {
+  // Allow toolkit to save its own seetings.
+  $function = 'image_' . $current_toolkit . '_settings_submit';
+  if (function_exists($function)) {
+    $function($form, $form_state);
+  }

+++ b/core/modules/system/system.install
@@ -1997,6 +1997,19 @@ function system_update_8014() {
+  update_variables_to_config('system.image', array(
+    'image_toolkit' => 'toolkit',
+    'image_jpeg_quality' => 'gd.jpeg_quality',
+  ));

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.

I 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.

  if (count($toolkits_available) == 1) {
    // Set the image toolkit to the one only available.
    $config->set('toolkit', key($toolkits_available))->save();
  }

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)

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageEffectsTest.php
@@ -23,7 +23,7 @@ class ImageEffectsTest extends ToolkitTestBase {
-    parent::setUp('image_test');
+    parent::setUp();

+++ b/core/modules/system/lib/Drupal/system/Tests/Image/ToolkitTestBase.php
@@ -19,7 +19,7 @@ abstract class ToolkitTestBase extends WebTestBase {
-    parent::setUp('image_test');
+    parent::setUp(array('image', 'image_test'));

I do not really understand these changes, or rather, why they are relevant for this patch?

+++ b/core/modules/system/system.admin.inc
@@ -1854,30 +1854,32 @@ function system_file_system_settings() {
+  if (count($toolkits_available) == 1) {
+    // Set the image toolkit to the one only available.
+    $config->set('toolkit', key($toolkits_available))->save();
   }

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.

+++ b/core/modules/system/system.admin.inc
@@ -1886,7 +1888,23 @@ function system_image_toolkit_settings() {
+  $function = 'image_' . image_get_toolkit() . '_settings_submit';

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?

image_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

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageEffectsTest.php
@@ -23,7 +23,7 @@ class ImageEffectsTest extends ToolkitTestBase {
-    parent::setUp('image_test');
+    parent::setUp();

+++ b/core/modules/system/lib/Drupal/system/Tests/Image/ToolkitTestBase.php
@@ -19,7 +19,7 @@ abstract class ToolkitTestBase extends WebTestBase {
-    parent::setUp('image_test');
+    parent::setUp(array('image', 'image_test'));

As I wrote in #4...

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)

This caused me a serious waste of time... and the fix seemed trivial and correct. Happy to create a separate patch if you like.

After 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.

1. Enable image_test module (this is a test module - easiest way is using drush)
2. Visit admin/config/media/image-toolkit
3. Select the test toolkit and press save
4. Select the GD toolkit and press save.

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.

For work on converting image toolkits to plugins see #1664844: Convert image toolkits into plugins

start with a reroll of #8.

While 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.

As per #13

#11: drupal-image_toolkit-1712258-11.patch queued for re-testing.

This 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.

If 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.

#17 could be merged into #11 to make form smaller

@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.

As no changes in form structure it's just a clean-up that's ready

Makes sense. Committed/pushed to 8.x.