Responsive Image shows an Error in Views

I think this is the UI module. Daniel will correct me if wrong!

I confirm, face the same issue. Thanks

Hi

We have different solutions to resolve this issue. You can see further details below

1- Make the field(responsive_image_style) as un-required
If admin user delete the all options for Responsive image style then empty select box will display. User can submit the form without selecting option.

2- Assign default value NULL
I think this is more sustainable because instead of An illegal message this shows the required field message which make more sense. I have attached the patch for this. Thanks

Rerolled the patch, please review.

Just retested this and I can still reproduce this.

The fix changes the error to 'Responsive image style field is required.'

The current fix is in responsive image, so setting the component to that.

This still needs tests, so back to needs work.

@themetman Thanks for the triage! I think this looks good, the 'required' message is much better then the 'illegal choice' error.

The patch looks fine, but like you said we really need a test for this.

@themetman great opportunity to learn a new skill then :)

I'd just go for something like this.

This Patch #23 works perfect for me.

1. +++ b/core/modules/responsive_image/tests/src/Functional/ViewsIntegrationTest.php
   @@ -0,0 +1,66 @@
   + * Tests the integration of responsive image Views.
   

   s/Views/with Views/

2. +++ b/core/modules/responsive_image/tests/src/Functional/ViewsIntegrationTest.php
   @@ -0,0 +1,66 @@
   +  public static $testViews = ['entity_test_row'];
   

   Nit: missing inheritdoc.

+++ b/core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
@@ -138,7 +138,7 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
-      '#default_value' => $this->getSetting('responsive_image_style'),
+      '#default_value' => $this->getSetting('responsive_image_style') ?: NULL,

Would a better fix be to change the default_setting to have a NULL instead of this check?

That would be more inline of how getSetting works. Also as we can't provide a default setting here how about just removing responsive_image_style from the default settings?

Also #28 needs addressing.

@alexpott, addressed #28.

Updated patch with all changes addressed in #28 & #30.
Also added an interdiff.

Above patch addresses the issues mentioned in #28 and #30 as required. Applies cleanly and RTBC good to go!

+++ b/core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
@@ -138,7 +138,7 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
-      '#default_value' => $this->getSetting('responsive_image_style'),
+      '#default_value' => NULL,

This would mean that the selected value is not selected when you look at this form after subitting. I think @alexpott meant setting the default value in \Drupal\responsive_image\Plugin\Field\FieldFormatter\ResponsiveImageFormatter::defaultSettings to NULL instead of an empty string.

So here we go with that plus test coverage for the correct value getting set (which fails with #35)

Ok so setting the default to NULL doesn't work because that doesn't match the config schema. Just removing the setting from defaults (thus making sure it's NULL) doesn't work either, then nothing get saved.

So the ?: NULL seems the best solution, so back to that.

Also redid #28.2, $testViews is never set in any parent class (silly, I know) so @inheritdoc doesn't work there, but it was missing a docblock before, so changed that to a normal docblock.

No idea why the added test for saving the value fails in #37, passes fine locally, lets see what the bot makes of this.

I have one really small nit to pick, sorry @Lendude

+++ b/core/modules/responsive_image/tests/src/Functional/ViewsIntegrationTest.php
@@ -0,0 +1,94 @@
+  public static $modules = ['views', 'views_ui', 'responsive_image', 'field', 'image', 'file', 'entity_test', 'breakpoint', 'responsive_image_test_module'];

This array is over 80 cols wide, and should be broken up onto multiple lines.

@borisson_ no worries, that will teach me to remember to 'Reformat code' before uploading a patch!

Wow, that was a super quick response! The rest of the patch already looked solid to me. Setting to RTBC.

+1 for RTBC, looks good to me.

unrelated fails

Committed and pushed ed99d34b12 to 8.6.x and 3c256be771 to 8.5.x. Thanks!

Thanks @Lendude for investigating my suggestion and finding out it does not work.

Crediting @borisson_ and @Wim Leers for reviews.