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()
Files: 
CommentFileSizeAuthor
#17 image_toolkit_unneeded_check.patch2.59 KBslv_
PASSED: [[SimpleTest]]: [MySQL] 54,735 pass(es).
[ View ]
#11 drupal-image_toolkit-1712258-11.patch6.49 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-image_toolkit-1712258-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 1712258-8.image_toolkit.patch7.01 KBShuairan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1712258-8.image_toolkit.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#7 4-7-interdiff.txt2.2 KBalexpott
#7 1712258-7.image_toolkit.patch6.98 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 39,804 pass(es).
[ View ]
#4 2-4-interdiff.txt5.84 KBalexpott
#4 1712258-4.image_toolkit.patch7.09 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 39,803 pass(es).
[ View ]
#2 1712258-2.image_toolkit.patch5.42 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Comments

Component:configuration system» image system

Status:Active» Needs review
StatusFileSize
new5.42 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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

Status:Needs review» Needs work

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.

Status:Needs work» Needs review
StatusFileSize
new7.09 KB
PASSED: [[SimpleTest]]: [MySQL] 39,803 pass(es).
[ View ]
new5.84 KB

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)

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new6.98 KB
PASSED: [[SimpleTest]]: [MySQL] 39,804 pass(es).
[ View ]
new2.2 KB

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.

StatusFileSize
new7.01 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1712258-8.image_toolkit.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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

Status:Needs review» Needs work

The last submitted patch, 1712258-8.image_toolkit.patch, failed testing.

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

StatusFileSize
new6.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-image_toolkit-1712258-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

start with a reroll of #8.

Status:Needs work» Needs review

Status:Needs review» Postponed

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.

Status:Postponed» Active

As per #13

Status:Active» Needs review
Issue tags:-API change, -Configuration system, -Config novice

Status:Needs review» Needs work
Issue tags:+API change, +Configuration system, +Config novice

The last submitted patch, drupal-image_toolkit-1712258-11.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.59 KB
PASSED: [[SimpleTest]]: [MySQL] 54,735 pass(es).
[ View ]

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

Title:Convert image toolkit form to configuration systemSimplify image toolkit form logic, removing unneeded checks.

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

Status:Needs review» Reviewed & tested by the community
Issue tags:-API change+clean-up

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

Status:Reviewed & tested by the community» Fixed

Makes sense. Committed/pushed to 8.x.

Automatically closed -- issue fixed for 2 weeks with no activity.