Add srcset to template_preprocess_image

I think there's no reason to introduce API change here, so just add a new attribute and test for it

+++ b/core/includes/theme.inc
@@ -1276,9 +1276,31 @@ function template_preprocess_links(&$variables) {
+  if (isset($variables['uri']) && !empty($variables['uri'])) {
...
+  if (isset($variables['srcset']) && !empty($variables['srcset'])) {

!empty() is enough

+++ b/core/modules/image/lib/Drupal/image/Entity/ImageStyle.php
@@ -312,6 +312,15 @@ public function transformDimensions(array &$dimensions) {
+  public function transformMimeType(&$mime_type) {

+++ b/core/modules/image/lib/Drupal/image/ImageEffectBase.php
@@ -47,6 +47,13 @@ public function transformDimensions(array &$dimensions) {
+  public function transformMimeType(&$mime_type) {
+    $mime_type = NULL;

+++ b/core/modules/image/lib/Drupal/image/ImageEffectInterface.php
@@ -37,6 +37,14 @@ public function applyEffect(ImageInterface $image);
+  public function transformMimeType(&$mime_type);

+++ b/core/modules/image/lib/Drupal/image/Plugin/ImageEffect/DesaturateImageEffect.php
@@ -30,6 +30,12 @@ public function transformDimensions(array &$dimensions) {
+  public function transformMimeType(&$mime_type) {
+  }

It's not clear the reason to introduce API change here?

This is API addition, updated summary.
except tests this one needs at least one usage in core

@attiks please check summary

+++ b/core/modules/image/lib/Drupal/image/Entity/ImageStyle.php
@@ -312,6 +312,15 @@ public function transformDimensions(array &$dimensions) {
+  public function transformMimeType(&$mime_type) {

suppose this one should be added to interface as well

No, I mean ImageStyleInterface should be updated with the new public method too

Added tests for transformMimeType().

1. +++ b/core/includes/theme.inc
   @@ -1279,9 +1279,31 @@ function template_preprocess_links(&$variables) {
   +      if (isset($srcset['uri']) && !empty($srcset['uri'])) {
   ...
   +        if (isset($srcset['width']) && !empty($srcset['width'])) {
   ...
   +        elseif (isset($srcset['multiplier']) && !empty($srcset['multiplier'])) {
   

   Why both isset() and !empty() ? The latter should be enough ?

2. +++ b/core/modules/image/lib/Drupal/image/ImageEffectBase.php
   @@ -47,6 +47,13 @@ public function transformDimensions(array &$dimensions) {
   +  public function transformMimeType(&$mime_type) {
   +    $mime_type = NULL;
   +  }
   

   The base/default implementation coul be "just leave the $mime_type untouched", instead of forcing 99% effects to override that one ?

1. That is true for the first level of the $variables array, as you can provide default values for each key. But since the 'srcset' key contains an array with multiple srcsets, there are no default values for those keys, so both isset() and !empty() are necessary to prevent PHP notices.
2. new patch changes transformMimeType in ImageEffectBase to an empty function.

+++ b/core/modules/image/src/Entity/ImageStyle.php
@@ -333,7 +342,7 @@ public function getEffect($effect) {
-      $this->effectsBag = new ImageEffectBag(\Drupal::service('plugin.manager.image.effect'), $this->effects);
+      $this->effectsBag = new ImageEffectBag($this->getImageEffectPluginManager(), $this->effects);

@@ -377,4 +386,11 @@ public function setName($name) {
+  /**
+   * {@inheritdoc}
+   */
+  public function getImageEffectPluginManager() {
+    return \Drupal::service('plugin.manager.image.effect');
+  }
+

+++ b/core/modules/image/src/Tests/ImageStyleTest.php
@@ -0,0 +1,111 @@
+      ->setMethods(array('getImageEffectPluginManager'))
...
+      ->method('getImageEffectPluginManager')

This is probably a dumb question because I don't understand things, but why I are we creating a method just to wrap around the service call?

Is that for use in ImageStyleTest?

#2260061-89: Responsive image module does not support sizes/picture polyfill 2.2 point 3 and #2260061-93: Responsive image module does not support sizes/picture polyfill 2.2

So basically, yes it is for use in ImageStyleTest. This way we don't have to mock the container in a UnitTest (which apparently is a big no-no :-) )

Nitpicks below

1. +++ b/core/includes/theme.inc
   @@ -1077,9 +1077,30 @@ function template_preprocess_links(&$variables) {
   +    $srcsets = array(); 
   +    foreach ($variables['srcset'] as $srcset) {
   +      // URI is mandatory.
   +      $srcset['src'] = file_create_url($srcset['uri']);
   +      $new_source = $srcset['src'];
   +      if (isset($srcset['width']) && !empty($srcset['width'])) {
   +        $new_source .= ' ' . $srcset['width'];
   +      }
   +      elseif (isset($srcset['multiplier']) && !empty($srcset['multiplier'])) {
   +        $new_source .= ' ' . $srcset['multiplier'];
   +      }
   +      $srcsets[] = $new_source;
   +    }
   +    $variables['attributes']['srcset'] = implode(', ', $srcsets);
   

   Var names could be worked on a bit here.

   - This translates the info in $variable['srcset'] into an $srcsets array suitable for an 'srcset' HTML attribute. Why is one singular and the other plural ? Those two things contain the same info in different shapes.

   - "foreach (srcset as srcset)" is a bit weird. Strictly speaking, an element of an "srcset" is an "src" ? or a "source" ?

   - We don't need to assign back $srcset['src'], it is never used, so why not just $new_dource = file_create_uri(...) ?

   - "new" in $new_source is not really needed, it's the source string we're building in this iteration of the foreach. There is no "old" source here.

2. +++ b/core/modules/image/src/ImageEffectInterface.php
   @@ -44,6 +44,14 @@ public function applyEffect(ImageInterface $image);
   +   *   MIME type to be modified.
   
   +++ b/core/modules/image/src/ImageStyleInterface.php
   @@ -131,6 +131,14 @@ public function createDerivative($original_uri, $derivative_uri);
   +   *   MIME type to be modified.
   

   Misses an article to be an actual sentence

New patch with remarks from #30 addressed.

Thanks!

I don't see where the mimetype part of the patch gets used though ? AFAIK, the 'type' attribute is only for <source> tags within a picture, not for srcset on <img> tags, so this seems out of scope here ?

Well, or keep the mimetype part for a patch that does add support for the 'type' attribute on sources in theme_responsive_image() ?

New patch without the MIME type stuff, and with an added test.

Not sure "outputted" is actual english ?
Other than that, looks good when green :-)

Although "outputted" is actual english, preference seems to go to just "output" (http://english.stackexchange.com/questions/35418/past-tense-of-to-output...), so I changed it just to be on the safe side.

You could replace output with processed as another option.

Thanks. Language can be adjusted on commit if deemed necessary.

FYI: I also created #2333395: Add sizes to template_preprocess_image

1. +++ b/core/includes/theme.inc
   @@ -1120,9 +1120,29 @@ function template_preprocess_links(&$variables) {
   + *   - srcset: Array of multiple URI's and sizes/multipliers.
   

   Minor grammar thing: URIs, not URI's. ;) http://theoatmeal.com/comics/apostrophe

   I can fix this on commit, but...

2. +++ b/core/includes/theme.inc
   @@ -1120,9 +1120,29 @@ function template_preprocess_links(&$variables) {
   -  $variables['attributes']['src'] = file_create_url($variables['uri']);
   +  if (!empty($variables['uri'])) {
   +    $variables['attributes']['src'] = file_create_url($variables['uri']);
   +  }
   

   Reading through http://www.w3.org/html/wg/drafts/html/master/embedded-content.html#attr-... it says:

   The src attribute must be present.

   The srcset attribute may also be present.

   (emphasis mine)

   So why did we turn variables['attributes']['src'] into an optional variable here?

1. +++ b/core/modules/system/src/Tests/Theme/ImageTest.php
   @@ -0,0 +1,123 @@
   +class ImageTest extends DrupalUnitTestBase {
   

   Yay! Tests! That's great.

   However, AFAIK DUTB is being phased out in favor of KernelTestBase: https://www.drupal.org/node/1829160

2. +++ b/core/modules/system/src/Tests/Theme/ImageTest.php
   @@ -0,0 +1,123 @@
   +    $this->render($image);
   +    $this->removeWhiteSpace();
   ...
   +    $this->render($image);
   +    $this->removeWhiteSpace();
   

   Unfortunate that we need this method call, and render() doesn't just do what you would expect. :( Is that a separate bug somewhere?

Based on dawehner's comments on the sizes issue, this is actually needs work for the test.

Changed the test. I didn't want to change anything about the src attribute being optional or not until we can decide on #48 or #49.

I just checked with the responsive images community group that came up with the picture element, and yes, while spec says that the img inside picture should always have src, that results in a double download with browsers that don't understand picture. So to make everything groovy when using this in conjunction with Picturefill, no src on the img inside picture. We're going to close our eyes, stick our fingers in our ears, go la la la la, and save the old browsers from two downloads. :)

That's a fair explanation, but is that relevant for D8? What "old browsers" are we supporting in D8? Or does IE9 count as an "old browser" in this context?

In addition to #53:
It seems that in Firefox you'll have to enable a flag: http://caniuse.com/#feat=srcset
Also iOS Safari will only support srcset in iOS8 (To be fair, most people will probably have updated by the time Drupal 8 releases)

The biggest group will probably be the IE users and the Firefox users who haven't enabled the flag for srcset support.

The browsers in question are all browsers out today. D8 is supporting browsers like IE11, so using no src on the img is something we need to ge able to do.

Firefox won't always have this behind a flag. That's a temporary thing.

Cool, thanks for answering!

I'm guessing this will need a reroll because #2333395: Add sizes to template_preprocess_image has been committed.

Rerolled patch

Since it's just a reroll...

Ok great! Committed and pushed to 8.x. Thanks!

Almost there. :)