Remove template_preprocess_image()

Only thing I found is a small comment nitpick.

+++ b/core/modules/system/templates/image.html.twig
@@ -4,12 +4,63 @@
+ * - srcset: Array of multiple URIs and sizes/multipliers.
+ * - sizes: The sizes attribute for viewport-based selection of images.

+++ b/core/themes/classy/templates/field/image.html.twig
@@ -1,13 +1,32 @@
+ * - srcset: Array of multiple URIs and sizes/multipliers.
+ * - sizes: The sizes attribute for viewport-based selection of images.

+++ b/core/themes/stable/stable.theme
@@ -23,3 +23,66 @@ function stable_preprocess_links(&$variables) {
+ *   - srcset: Array of multiple URIs and sizes/multipliers.
+ *   - sizes: The sizes attribute for viewport-based selection of images.

+++ b/core/themes/stable/templates/field/image.html.twig
@@ -4,10 +4,30 @@
+ * - srcset: Array of multiple URIs and sizes/multipliers.
+ * - sizes: The sizes attribute for viewport-based selection of images.

add (optional) to be consistent, see stylename explanation.

--- a/core/themes/classy/templates/field/image.html.twig
+++ b/core/themes/classy/templates/field/image.html.twig
@@ -1,13 +1,33 @@

@@ -1,13 +1,33 @@
+{% extends "@system/image.html.twig" %}

We can't do this. Even if we can guarantee the same result now, it would have Classy extending off of an unstable template.

Moving the preprocess to Stable should be fine, as well as updating the System template.

Agreed with #19. We have to move the preprocess and template to Stable to ensure that any custom template overrides doesn't break. Then if we want to, we can also implement the reverting change in Bartik and Seven if we want to try it out in an actual theme unless we fix #2659890: [Policy] [Plan] Drupal 9 and 10 markup and CSS backwards compatibility

Nope, Classy and Stable are stable. That's why Stable is the default base theme that themes are extending unless they explicitly specify not to extend it.

Some pictures to be explicit:

In the picture dark blue themes are stable and light blue ones are unstable.

Flowchart how people are evaluating which theme they want to extend.

Yes, but Classy extends Stable so there should be no need to repeat anything in Classy and most likely we can just leave it untouched

What I would love to see here is an issue summary explaining the motivation, otherwise I assume "because we can" :)

Looks good! I hope we'd have unstable classy but this is good for now!

Making sure Cottser gets a chance to take a look at this.

The template change is a bit o__O for me, but also getting rid of preprocess has been on my list for years now, so +1 from me in principle.

I'm still not totally sure about this - what are we gaining in a practical way other than one less template_preprocess_* function? Do we have a use case for users modifying this logic directly in the template, specifically something that couldn't be done by manipulating the attributes in the template?

I'm not opposed to this in principle (I think it actually does match with the principles that were set out, specifically "Don't dumb it down") but I think we should discuss this one a bit more because to my knowledge we don't have anything quite this complex in core:

+++ b/core/modules/system/templates/image.html.twig
@@ -4,12 +4,63 @@
+  {% if _context[attribute_name] is defined and attributes[attribute_name] is empty %}
+    {% set attributes = attributes.setAttribute(attribute_name, _context[attribute_name]) %}
+  {% endif %}

Specifically the use of _context and is defined/is empty, but it's also inside a loop :)

Finally, just a general comment:

+++ b/core/modules/system/templates/image.html.twig
@@ -4,12 +4,63 @@
+    {% if source.width is defined %}
...
+    {% elseif source.multiplier is defined %}

Do we need is defined for these?

In general we should think a bit more about BC. Preprocess functions aren't just a thing for themes, but they are something also for modules.
For example there is the Lazyloader module which used to implement its logic (replace the used image attributes) inside another hook_preprocess_image() method. If we move the logic from the preprocess function into a template, this could be sort of a blocker/problem for those modules. Whether modules should be able to provide that kind of functionality is a different question, but I want to rise the point, that its not risk free.

https://www.drupal.org/core/d8-bc-policy doesn't talk yet about preprocess functions.

I'd put preprocess in the same category as alter. i.e. no guarantees about what you get, or about whether what you add to it eventually gets used - might need adding to the doc though.

I think there's a question about how much logic we should be moving to templates, vs. how much logic we could potentially do earlier than preprocess - at least with entity rendering, a lot of things that used to be done in preprocess can be done in hook_entity_view()/alter() and friends which aren't going anywhere at the moment. i.e. do we really need to check if defined in the template or could we initialize earlier than the template instead?

Appears there is still some discussion to be had. Once a decision is made maybe an updated IS is needed.