Can configure responsive image formatter to cause fatal error

First solution from googletorp have an issue : when you did not have created any responsive image style at all, and change the formatter of the field, you won't be able to save the page at all, since there is not even a default style to be selected.

I think, like everywhere else, responsive image should provide a default style from itselft when you enable it. It solves multiple problems :

• you can select this default if the user did not select anything else
• it can works as an example for users
• no more fatal error :)

The attached patch :

• Creates a default responsive image style, called "Default responsive style", using the Image style from the Image module (large, medium, thumbnail)
• Create default breakpoints, so that we won't be dependant of bartik, seven or toolbar (which are the other modules that creates breakpoints)
• Use this newly created style as the default one when you choose to display an image field with the "responsive image" formatter.

+++ b/core/modules/responsive_image/config/install/responsive_image.styles.default_responsive_style.yml
@@ -0,0 +1,29 @@
+  theme:
+    - bartik

According to your comment that's not needed anymore :)

thanks, I just removed this line.

Fixing typo on issue.

I'm not sure that I agree that a default responsive style is the best way to address this, as this default style is not likely to provide good defaults for the wide variety of image use cases where it might end up being selected.

Throwing an error after trying to save the selection of a responsive image formatter with no style selected seems like a much better solution.

if we want to throw an error when no style is selected, we also need to handle the case where no style at all was created in the system. And since we won't let user submit the page, he will be "locked" in the "Manage display" page.

For me, the biggest issue here is that this whould introduce some kind of new behaviour in Drupal. Right now, I can't find any field formatter that does not work with a 100% default settings. The resposive image formatter would be the only one that requiere a specific configuration before you'll be able to use it.

The proposed solution in the patch is to add a default responsive image style and associated breakpoints. However the defaults in the patch, while well-intentioned, aren't terribly useful. If we are going to add defaults, they should be sensible. That's what #2513604: Create default responsive image styles does. Hence, this issue is being postponed on that going in, which will go a long way towards solving this issue.

+++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
@@ -190,6 +191,28 @@ protected function doTestResponsiveImageFieldFormatters($scheme, $empty_styles =
+    // Test field not being configured. This should not cause a fetal error.

s/fetal/fatal

Manually tested this.

If I go enable responsive images, then configure the image field on the article to use the responsive image field formatter and then press save, when I add content with an image then the page displays but with no image at all. There is no warning as a site builder that I did anything wrong when I press save on manage display. That seems like a problem.

If when I select the responsive image field formatter, I go to configure the field formatter but don't select a responsive image style, then pressing update returns me to the same field formatter config with an error message. That seems like a correct behavior: I shouldn't be able to leave that field formatter empty.

#2547925: Possible to save malconfigured field display indicates this is a general problem for the manage display tab, not solely for the responsive image field formatter.

I'd rather see the root problem fixed rather than letting a site builder think everything is okay until they view their content and the image does not show up.

Tested this situation without this patch. That fatal error is definitely worse than what is happening with this patch applied. I'd still rather see the root problem fixed, but if the typo is fixed, I'll mark this RTBC.

+++ b/core/modules/responsive_image/responsive_image.module
@@ -147,6 +147,12 @@ function template_preprocess_responsive_image(&$variables) {
+  // If we don't have a style, log error and stop execution.

Sorry I missed this the first time through, but we should not use contractions in comments as per the coding standards.

Maybe:

"If a responsive image style is not selected, log the error and stop execution."

Thanks for working on this, googletorp and Haza.

Hmm, why don't we do the same as template_preprocess_image_formatter() does ? If there's no image style, it defaults to #theme => 'image. That actually makes more sense to me as you will actually see an image (probably a 'big' one then, so you'll see the 'problem' way faster), instead of seeing nothing and a watchdog entry. Default to at least something looks more elegant to me. And then at least both formatters act consistently.

- edit (added code example) -

For reference, here's the top of template_preprocess_image_formatter()

function template_preprocess_image_formatter(&$variables) {
  if ($variables['image_style']) {
    $variables['image'] = array(
      '#theme' => 'image_style',
      '#style_name' => $variables['image_style'],
    );
  }
  else {
    $variables['image'] = array(
      '#theme' => 'image',
    );
  }

I'm tempted to set it to needs review/work, but I'll let core committers decide to push back or not.

Yes that makes more sense to me too.

I think we may still want the error detection in preprocess_responsive_image, because it is very possible to create a responsive image render element in code without using the formatter (and sometimes necessary to do so from my experience).

So, to be clear, by falling back to an image, are we falling back to the original image? Or does this fall back to the image style selected as the fallback? Or do we fall back to the fallback style, but if there isn't one, then we fallback to the original image.

I may be up too late.

Thank you very much @mdrummond for that patch!
I have a few comments, though.

• +++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
  @@ -190,6 +191,28 @@ protected function doTestResponsiveImageFieldFormatters($scheme, $empty_styles =
  +      ->load('node' . '.' . 'article' . '.' . 'default');
  

  Why do you use concatenation here? It seems more readable to only use a simple string.

  </li><li>
  +++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
  @@ -190,6 +191,28 @@ protected function doTestResponsiveImageFieldFormatters($scheme, $empty_styles =
  +    $display = \Drupal::entityManager()
  ...
  +      $display = \Drupal::entityManager()->getStorage('entity_view_display')->create($values);
  

  You should use $this->container->get('entity.manager') here as done a few lines before in the same test. That ensure that the container can be mocked if necessary for the tests.

To answer your questions in #34 I'd say that it would be nice to have that fallback to the fallback image style then to the original image but it can be postponed to 8.1 as soon as this fatal error is fixed.

And there is the functional review.

Before:

After:

\o/

I'm not sure I think that falling back to the fallback style would be the best behavior. I just wasn't sure what would happen.

But now I've remembered that the fallback style is defined within the responsive image style, not the field formatter. So if there's no valid responsive image style, then original image it is.

Just tested this, and it works fine. Looks like code changes requested have been made.

It does feel strange that you can select a responsive image formatter, not select a responsive image style and get no onscreen warning about that. There really should be some sort of user message about that.

That's a separate issue, however, and we can file a follow-up issue for that. Moving this back to RTBC.

You're totally right, I think that's the best we can do without a deep change in the way that formatter settings are validated.

In the best possible case, submitting the view mode form would validate each formatter settings to ensure that everything is good. It seems to be quite hard to achieve right now, though.

Having some kind of default here is a lot better for UX as well.

Committed and pushed to 8.0.x. Thanks!