Move fallback image style into the responsive image style entity

Fallback image style, right?

P.S.: wow, that's a huge amount of tags :D

Let's get this done?

;-)

Could only find one nitpick :) Leaving at NR so others can review too.

Would be great to update the IS in two ways:

1. Explain why this change is necessary, because it's actually a BC break
2. A beta evaluation

+++ b/core/modules/responsive_image/src/ResponsiveImageStyleForm.php
@@ -100,6 +100,15 @@ public function form(array $form, FormStateInterface $form_state) {
+      '#title' => t('Fallback image style'),

$this->t()

New patch. Fixes the nitpick from #8 and fixes the failing tests.

Forgot the interdiff.

I did my best at updating the issue summary. I'll have a look at the beta evaluation. I've never written one before so bear with me :-).

Added beta evaluation.

Oooh, +1 !

Took a quick look at the code, and it looks good. Wim gave the thumbs-up, so I think we're good on that front. I'll give it a test to make sure it works.

Tested, and this works fine.

This is going to be a big time saver for people who reuse a mapping in multiple places.

This looks RTBC to me. Wim gave the thumbs up on the code, and I'm verifying that this works as tested. I can select a fallback image style as I create a responsive image style. It is editable when I go back to the image style. The fallback selector does not appear on the field formatter on a content type field display tab. And the fallback image shows up properly in a picture element.

Great quick work on this!

Assuming the beta eval is correct, we need a change record draft for this. However, apart from a bunch of stuff in responsive_image module, I'm not seeing a huge impact on any other core modules. Did you manage to do this without breaking BC, or is it only breaking particular types of contrib modules?

This would affect modules, themes or sites that are using responsive image styles, since they would define the fallback image style in the responsive image style rather than in the field formatter or through the use of the theme function.

A change record would be good in case anybody is doing that. I'm not aware of contrib modules that are implementing this right now, and it's probably doubtful that there are many themes or sites that are doing so either. This change actually makes it a lot easier for people to make use of responsive image styles, but yes, this is a change.

In reviewing https://www.drupal.org/core/beta-changes, I think it's a stretch to call this a disruptive change, and I think the impact is definitely bigger than any disruption.

I agree with #21. At first I misread https://www.drupal.org/core/beta-changes#disruption

Introduces a BC break that will affect many contributed modules, or require some contributed modules to make non-trivial changes

as

Introduces a BC break that will affect any contributed modules, or require some contributed modules to make non-trivial changes

So if nobody objects I'd change this to non-disruptive in the IS.

Are there guidelines somewhere on how to write a change record? I haven't done it before.

Nope, no guidelines. The guideline I use: the information that I'd personally want to be given for a change :)

Here's a good write-up on how to write a change record: https://www.drupal.org/contributor-tasks/draft-change-record

Updating the issue summary to make clear this is not disruptive.

Patch no longer applies. We need a reroll.

Rerolled patch, I'll start writing a change record as well.

Change record created at https://www.drupal.org/node/2472153

Moving this back to RTBC. Beta evaluation updated, change notice posted and patch rerolled. Should be good to go!

Tagging because if we were doing beta to beta upgrades this would need a hook_update_N

Edited the beta evaluation to provide a more accurate assessment of the potential for disruption. Also edited the change notice at https://www.drupal.org/node/2472153 to provide guidance for existing sites that they will need to define a fallback style in their responsive image mappings.

RE #32: Not sure an update hook is possible here:

Field instance A can use responsive image style 1 with fallback image style "large".
Field instance B can use responsive image style 1 with fallback image style "medium".

Now that the fallback image style is moved from field instance to responsive image style, how would we update in that case? (See "Disruption" in the Beta phase evaluation as it is explained very well there)

This is a followup of the critical #2260061: Responsive image module does not support sizes/picture polyfill 2.2 and it improves the UX of configuring responsive images. Considering that responsive_image is not enabled in standard I think the disruption will be minimal. Because the UX improvement outweighs the disruption - you only have to configure fallback once per responsive image mapping rather than on every field formatter - I think this is acceptable for beta. Committed 7388924 and pushed to 8.0.x. Thanks!

Thank you for adding the detailed beta evaluation to the issue summary.

Woohoo! Thank alex! And thanks everyone for the hard work and feedback!

I'm writing beta9 to beta10 upgrade path functions in the HEAD to HEAD module and I was thinking if at least a minimal implementation would be enough for this issue.

Considering the scenario from #37, how about updating the responsive image style config entity to use the first fallback image style encountered (i.e. of the first field instance), and, in case the following field instances are configured with a different fallback image style just have the update function output a warning message to let the user know that manual intervention is required?

Or is that still too much trouble for no real gain? :)

@amateescu Sounds like a good plan. Thanks for working on that!

Ok, here it is: #2479801: Upgrade path for #2349461 (move fallback image style configuration). I haven't tested it yet so I'd appreciate a quick look at the patch if anyone has some time for that :)