Add test for editing image effect when configuration form is Ajax enabled

Nice catch! I would store the plugin in $form_state instead. That will remove the need for using magic serialization methods.

Can you test this again without the Serializable fixes? Like a tests-only patch? AJAX forms have been changed a lot recently, this may be obsolete.

Confirmed that this is still a bug per the testing above. We can fix this in a patch release.

This looks great, I just found a few details.

The main problem to I found on this patch is the short array syntax and the old syntax is mixed, I think we should use one or the other but just one, (I would prefer the short syntax).

1. +++ b/core/modules/image/src/Form/ImageEffectFormBase.php
   @@ -148,4 +149,53 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +    $serviceIds = [];
   

   I think here it should be $service_ids = [];

2. +++ b/core/modules/image/src/Tests/ImageAdminStylesTest.php
   @@ -276,6 +276,47 @@ function testStyle() {
   +    $effect_edit = array(
   

   In this patch in another line we used the array's short syntax but here no, I think we should use it here as well to keep the consistency in the patch.

3. +++ b/core/modules/image/src/Tests/ImageAdminStylesTest.php
   @@ -276,6 +276,47 @@ function testStyle() {
   +    $edit = array(
   

   short syntax too.

4. +++ b/core/modules/image/src/Tests/ImageAdminStylesTest.php
   @@ -276,6 +276,47 @@ function testStyle() {
   +    $this->assertRaw(t('Style %name was created.', array('%name' => $style_label)));
   

   Array Short syntax as well.

5. +++ b/core/modules/image/src/Tests/ImageAdminStylesTest.php
   @@ -276,6 +276,47 @@ function testStyle() {
   +    $this->drupalPostForm($style_path, array('new' => 'image_module_test_ajax'), t('Add'));
   

   short syntax.

6. +++ b/core/modules/image/src/Tests/ImageAdminStylesTest.php
   @@ -276,6 +276,47 @@ function testStyle() {
   +    $this->drupalPostForm($style_path, array('new' => 'image_module_test_ajax'), t('Add'));
   

   short syntax.

7. +++ b/core/modules/image/src/Tests/ImageAdminStylesTest.php
   @@ -276,6 +276,47 @@ function testStyle() {
   +    $style = $style = ImageStyle::load($style_name);
   

   This looks weird, is there a reason why we are doing this?

8. +++ b/core/modules/image/tests/modules/image_module_test/src/Plugin/ImageEffect/AjaxTestImageEffect.php
   @@ -0,0 +1,92 @@
   +    return array(
   

   short syntax.

9. +++ b/core/modules/image/tests/modules/image_module_test/src/Plugin/ImageEffect/AjaxTestImageEffect.php
   @@ -0,0 +1,92 @@
   +    $form['test_parameter'] = array(
   

   short syntax.

10. +++ b/core/modules/image/tests/modules/image_module_test/src/Plugin/ImageEffect/AjaxTestImageEffect.php
    @@ -0,0 +1,92 @@
    +    $form['ajax_refresh'] = array(
    

    short syntax.

11. +++ b/core/modules/image/tests/modules/image_module_test/src/Plugin/ImageEffect/AjaxTestImageEffect.php
    @@ -0,0 +1,92 @@
    +        'callback' => array($this, 'ajaxCallback'),
    

    short syntax.

12. +++ b/core/modules/image/tests/modules/image_module_test/src/Plugin/ImageEffect/AjaxTestImageEffect.php
    @@ -0,0 +1,92 @@
    +    $form['ajax_value'] = array(
    

    short syntax.

13. +++ b/core/modules/image/tests/modules/image_module_test/src/Plugin/ImageEffect/AjaxTestImageEffect.php
    @@ -0,0 +1,92 @@
    +    $item = array(
    

    short syntax.

I'm +1 on adding the test.

I reviewed this again and I just found one really small thing.

1. +++ b/core/modules/image/tests/modules/image_module_test/src/Plugin/ImageEffect/AjaxTestImageEffect.php
   @@ -0,0 +1,90 @@
   +use Drupal\Core\Image\ImageInterface;
   +use Drupal\image\ConfigurableImageEffectBase;
   +
   +
   +/**
   + * Provides a test effect using Ajax in the configuration form.
   + *
   

   Let's remove this extra line here.

Without that extra line this is RTBC to me.

1. +++ b/core/modules/image/src/Tests/ImageAdminStylesTest.php
   @@ -292,6 +292,47 @@ function testStyle() {
   +   * Test for editing Ajax enabled effect forms.
   

   Tests editing Ajax-enabled image effect forms.

2. +++ b/core/modules/image/src/Tests/ImageAdminStylesTest.php
   @@ -292,6 +292,47 @@ function testStyle() {
   +  function testAjaxEnabledEffectForm() {
   

   public function

3. +++ b/core/modules/image/tests/modules/image_module_test/src/Plugin/ImageEffect/AjaxTestImageEffect.php
   @@ -0,0 +1,89 @@
   +      '#type'  => 'button',
   +      '#value' => $this->t('Ajax refresh'),
   +      '#ajax'  => ['callback' => [$this, 'ajaxCallback']],
   ...
   +      '#id'   => 'ajax-value',
   +      '#type'   => 'item',
   ...
   +      '#type'   => 'item',
   

   Weird extra spaces here, should only be one

Also can you post a second patch that includes a revert of the core issue that fixed this, so we know for sure the test is testing the right stuff?

Committed 822a21f and pushed to 8.0.x and 8.1.x. Thanks!