Issue #1898442 by Cottser, duellj, kgoel, tlattimore, cyborg_572, rwohleb: responsive_image.module - Convert theme_ functions to Twig.

Task

Use Twig instead of PHPTemplate

Remaining


Theme function name/template path Conversion status
theme_responsive_image_formatter todo

API changes

TBD

To test this code:

  1. Enable responsive image module
  2. Add a new picture mapping (admin/config/media/responsive-image-mapping) with
    "Bartik" as the breakpoint group and different image styles for each of the
    three breakpoints (e.g. 1x mobile = Thumbnail)
  3. Alter the image field on Article nodes to use picture formatting (admin/structure/types/manage/article/display)
  4. Select the responsive image mapping that you just created in the formatting
    settings
  5. Create a new article node with an image
  6. Resulting node should output image using picture templates (with
    <picture> and <source> tags)
CommentFileSizeAuthor
#187 diff-185-187.txt1.67 KBjayeshanandani
#187 1898442.187.patch22.26 KBjayeshanandani
#185 interdiff-181-185.txt4.87 KBjayeshanandani
#185 1898442.185.patch22.29 KBjayeshanandani
#182 diff 171-181.txt8.38 KBjayeshanandani
#181 1898442.181.patch19.89 KBjayeshanandani
#171 interdiff.txt857 bytesjayeshanandani
#171 1898442.171.patch22.18 KBjayeshanandani
#169 interdiff.txt1.65 KBjayeshanandani
#169 1898442.167.patch22.17 KBjayeshanandani
#166 convert-picture-twig-1898442-166.patch22.14 KBRainbowArray
#166 intediff-1898442-164-166.txt2.35 KBRainbowArray
#165 interdiff.txt12.14 KBjayeshanandani
#165 1898442.164.patch22.18 KBjayeshanandani
#163 inter-diff.txt6.83 KBjayeshanandani
#163 1898442.163.patch22.19 KBjayeshanandani
#156 1898442.154.patch21.68 KBjayeshanandani
#156 interdiff.txt6.9 KBjayeshanandani
#152 interdiff.txt6.54 KBjayeshanandani
#152 1898442.152.patch21.75 KBjayeshanandani
#148 1898442.148.patch21.71 KBjayeshanandani
#148 interdiff.txt21.71 KBjayeshanandani
#144 1898442.144.patch24.32 KBjayeshanandani
#134 picture-1898442.134.patch21.7 KBsteinmb
#124 picture-1898442.124.patch21.68 KBpplantinga
#122 picture-1898442.122.patch21.68 KBpplantinga
#122 interdiff.txt2.61 KBpplantinga
#120 picture-1898442.120-tests-only.patch4.6 KBEric_A
#120 picture-1898442.120.patch21.5 KBEric_A
#116 picture-1898442.116.patch16.9 KBpplantinga
#112 picture-1898442.112.patch16.89 KBpplantinga
#108 picture-1898442.interdiff.txt6.55 KBlarowlan
#108 picture-1898442.108.patch16.94 KBlarowlan
#101 picture-twig-1898442-101.patch15.7 KBRainbowArray
#101 interdiff.txt2.76 KBRainbowArray
#99 core-convert_picture_twig-1898442-99.patch14.47 KBjenlampton
#99 interdiff.txt7.69 KBjenlampton
#98 core-convert_picture_twig-1898442-98.patch14.43 KBjenlampton
#98 interdiff.txt1.56 KBjenlampton
#96 core-convert_picture_twig-1898442-96.patch13.97 KBjenlampton
#96 interdiff.txt6.4 KBjenlampton
#95 core-convert_picture_twig-1898442-95.patch15.11 KBjenlampton
#95 interdiff.txt738 bytesjenlampton
#89 core-1898442-89-picture_module_to_twig.patch15.11 KBpplantinga
#89 interdiff.txt689 bytespplantinga
#87 core-1898442-87-picture_module_to_twig.patch14.44 KBpplantinga
#87 interdiff.txt574 bytespplantinga
#85 core-1898442-85-picture_module_to_twig.patch14.44 KBpplantinga
#79 core-1898442-79-picture_module_to_twig.patch13.34 KBpplantinga
#72 core-1898442-72-picture_module_to_twig.patch13.34 KBLes Lim
#72 interdiff-1878442-70-72.txt1.2 KBLes Lim
#70 core-1898442-70-picture_module_to_twig.patch13.34 KBLes Lim
#62 twig-convert_picture-1898442-62.patch13.57 KBjenlampton
#62 interdiff.txt1.66 KBjenlampton
#62 picture-before.png69.52 KBjenlampton
#62 picture-after.png69.47 KBjenlampton
#59 twig-1898442-58.patch17.45 KBazinoman
#51 1898442-51-twig-picture.patch13.45 KBduellj
#36 1898442-36.patch13.37 KBcyborg_572
#36 interdiff.txt2.51 KBcyborg_572
#33 1898442-33.patch13.68 KBstar-szr
#33 interdiff.txt931 bytesstar-szr
#32 1898442-32.patch13.39 KBstar-szr
#32 interdiff.txt2.37 KBstar-szr
#29 1898442-twig-picture-module-29.patch13.12 KBkgoel
#29 interdiff.txt13.28 KBkgoel
#27 1898442-twig-picture-module-27.patch888 byteskgoel
#27 interdiff.txt1.04 KBkgoel
#26 1898442-twig-picture-module-26.patch10.28 KBkgoel
#26 interdiff.txt10.44 KBkgoel
#21 1898442-twig-picture-module-21.patch13.11 KBtlattimore
#21 interdiff.txt626 bytestlattimore
#19 1898442-19.patch13.36 KBstar-szr
#19 interdiff.txt5.86 KBstar-szr
#15 1898442-15.patch12.97 KBstar-szr
#15 interdiff.txt822 bytesstar-szr
#14 1898442-14.patch12.96 KBstar-szr
#14 interdiff.txt817 bytesstar-szr
#13 1898442-13.patch12.96 KBstar-szr
#13 interdiff.txt1.91 KBstar-szr
#12 1898442-12-twig-picture.patch12.68 KBduellj
#12 interdiff.txt5.79 KBduellj
#8 1898442-8-twig-picture.patch11.59 KBduellj
#6 1898442-6-twig-picture.patch11.58 KBduellj
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

c4rl’s picture

Issue tags: +Twig

Tagging

GeminiAgaloos’s picture

Assigned: Unassigned » GeminiAgaloos
GeminiAgaloos’s picture

At SandCamp 2013 twig sprint: On my first Drupal sprint after Ezra Gildesgame's keynote speech earlier today encouraging us to contribute even (especially?) when we've never done a drupal sprint before :)

star-szr’s picture

Assigned: GeminiAgaloos » Unassigned

Unassigning since it's been a while. @007g3m1n1 - if you'd still like to work on this, please re-assign to yourself. Thanks!

duellj’s picture

Assigned: Unassigned » duellj

I'm going to give this conversion a go. Any objections to doing all work here, rather then in the three issues in the twig sandbox?

duellj’s picture

Status: Active » Needs review
FileSize
11.58 KB

Here's the first pass at converting all picture themes to twig. One thing to note is that it looks like there was some misnamed variables in picture_theme() that I had to change in order for tests to pass, since I'm creating render arrays in preprocess functions now.

Status: Needs review » Needs work

The last submitted patch, 1898442-6-twig-picture.patch, failed testing.

duellj’s picture

Status: Needs work » Needs review
FileSize
11.59 KB

Whoops, tests should pass now.

duellj’s picture

Issue summary: View changes

Updated issue summary. Added related issues.

star-szr’s picture

Issue tags: +Needs manual testing

Tagging.

star-szr’s picture

Issue summary: View changes

Add conversion summary table

star-szr’s picture

Status: Needs review » Needs work
Issue tags: -Needs manual testing

This is looking really good, mostly nitpicky things. I added the two variable changes to the issue summary under API changes.

Documentation

  1. +++ b/core/modules/picture/picture.moduleundefined
    @@ -199,9 +204,15 @@ function picture_theme() {
      * @ingroup themeable
    
    @@ -242,7 +260,7 @@ function theme_picture_formatter($variables) {
      * @ingroup themeable
    
    @@ -334,23 +357,8 @@ function theme_picture($variables) {
      * @ingroup themeable
    

    The preprocess functions docblocks all look good, but we should be removing @ingroup themeable from them, we are adding @ingroup themeable to the Twig templates instead. See #1913208: [policy] Standardize template preprocess function documentation. We can also update the @param to @param array $variables on a couple of the preprocess functions.

  2. +++ b/core/modules/picture/templates/picture-formatter.html.twigundefined
    @@ -0,0 +1,22 @@
    + * - path: An optional array containing the link 'path' and attributes
    + *   'attributes'.
    ...
    +  <a href="{{ path.path }}" {{ attributes }}>{{ image }}</a>
    

    This is not docs only - the documentation confused me a bit, the variable description almost makes it sound like the attributes would be at path.attributes, and the preprocess function looks like it puts the attributes there. So I think the comment is probably correct but it should be {{ path.attributes }}, and butted up against the href="".

    We're also trying to avoid putting the word "array" in Twig templates, so maybe something like this?

    - path: (optional) Contains information about the link.
      - path.path: The URL the image links to.
      - path.attributes: HTML attributes for the link tag.
  3. +++ b/core/modules/picture/templates/picture-source.html.twigundefined
    @@ -0,0 +1,32 @@
    + * - srcset: The srcset containing the the path of the image file or a full
    + *   URL and optionally multipliers.
    

    It would be nice if this explained briefly what a srcset is, and what multipliers are :) Two "the" here on the first line as well.

  4. +++ b/core/modules/picture/templates/picture-source.html.twigundefined
    @@ -0,0 +1,32 @@
    + * - attributes: additional HTML attributes for source tag.
    

    Capitalize "Additional".

Code

  1. +++ b/core/modules/picture/picture.moduleundefined
    @@ -199,9 +204,15 @@ function picture_theme() {
    +function template_preprocess_picture_formatter(&$variables) {
    ...
    +    return;
    

    Since preprocess functions modify the variables by reference, there shouldn't be a return.

  2. +++ b/core/modules/picture/picture.moduleundefined
    @@ -211,25 +222,32 @@ function theme_picture_formatter($variables) {
    +    $variables['path']['attributes'] = array();
    +    if (isset($variables['path']['options']['attributes'])) {
    +      $variables['path']['attributes'] = new Attribute($variables['path']['options']['attributes']);
    +    }
    

    Not sure about this hunk, in one case attributes would be an array, in the other it would be an Attribute object. Do we need the empty array here? If so, I think it should be new Attribute() instead of array().

star-szr’s picture

Issue tags: +Needs manual testing

Didn't mean to remove the tag. @duellj said he could add manual testing steps to the summary.

star-szr’s picture

Issue summary: View changes

Add API changes

duellj’s picture

Status: Needs work » Needs review
FileSize
5.79 KB
12.68 KB

Updated patch with fixes from #10, thanks Cottser!

A couple notes:

Since preprocess functions modify the variables by reference, there shouldn't be a return.

The return was there to break out of the function so further variable preprocessing wouldn't happen. I wrapped it in an else{} instead.

Not sure about this hunk, in one case attributes would be an array, in the other it would be an Attribute object. Do we need the empty array here? If so, I think it should be new Attribute() instead of array().

Good call, I've changed it to create a new empty Attribute() if there's no attribtues for the path.

Also updated the issue with manual testing steps.

star-szr’s picture

Issue tags: -Needs manual testing
FileSize
1.91 KB
12.96 KB

I tested this manually and found one small discrepancy in picture-source. In the after-conversion markup, the second <source> element didn't have width and height attributes.

Before:

<!-- <source media="all and (min-width: 851px)" src="http://d8clean.dev/sites/default/files/styles/large/public/field/image/tumblr_melb7d75OW1qegssko1_1280.jpeg?itok=gdQdt_49"  width="358" height="480" /> -->
<source media="all and (min-width: 851px)" src="http://d8clean.dev/sites/default/files/styles/large/public/field/image/tumblr_melb7d75OW1qegssko1_1280.jpeg?itok=gdQdt_49"  width="358" height="480"/>

After:

    <!-- <source media="all and (min-width: 851px)" src="http://d8.dev/sites/default/files/styles/large/public/field/image/tumblr_melb7d75OW1qegssko1_1280.jpeg?itok=9OoTn6s-"  width="358" height="480" /> -->
    <source media="all and (min-width: 851px)" src="http://d8.dev/sites/default/files/styles/large/public/field/image/tumblr_melb7d75OW1qegssko1_1280.jpeg?itok=9OoTn6s-" />

The Attribute class keeps track of which attributes are printed so that we can do things like <tag class="{{ attributes.class }}"{{attributes}}> without having the classes print twice (http://drupal.org/node/1727592).

Attached fixes this issue by setting a new variable in the Twig template to allow for repeated printing of the attributes. Also made all {{ attributes }} butt up against preceding elements as per Twig coding standards and tweaked the closing slashes on the <source> tags to be consistent.

star-szr’s picture

FileSize
817 bytes
12.96 KB

Improved the comment explaining why we set a new variable.

star-szr’s picture

FileSize
822 bytes
12.97 KB

Ignore #14, interdiff here is from #13.

star-szr’s picture

Assigned: duellj » steveoliver

Assigning this one to @steveoliver to review, I think this is ready to go.

steveoliver’s picture

Assigned: steveoliver » Unassigned
Status: Needs review » Needs work
+++ b/core/modules/picture/picture.module
@@ -171,20 +172,24 @@ function picture_theme() {
         'srcset' => NULL,
-        'dimension' => NULL,
+        'dimensions' => NULL,
         'media' => NULL,

This var change seems reasonable to me--yes an API change, but more like a clean-up task as 'dimension' doesn't really make sense.

+++ b/core/modules/picture/picture.module
@@ -192,40 +197,53 @@ function picture_theme() {
-  if (isset($variables['path']['path'])) {
-    $path = $variables['path']['path'];
-    $options = isset($variables['path']['options']) ? $variables['path']['options'] : array();
-    $options['html'] = TRUE;
-    $output = l($output, $path, $options);
+    if (isset($variables['path']['path'])) {
+      if (isset($variables['path']['options']['attributes'])) {
+        $variables['path']['attributes'] = new Attribute($variables['path']['options']['attributes']);
+      }
+      else {
+        $variables['path']['attributes'] = new Attribute();
+      }
+      $options = isset($variables['path']['options']) ? $variables['path']['options'] : array();

Let's keep to the ternary form when setting $variables['path']['attributes'], yeah?

Like this:

$variables['path']['attributes'] = new Attribute(isset($variables['path']['options']['attributes']) ? $variables['path']['options']['attributes'] : array());
+++ b/core/modules/picture/picture.module
@@ -251,13 +267,14 @@ function theme_picture($variables) {
     unset($variables['height']);
   }
 
-  $sources = array();
+  $variables['sources'] = array();
   $output = array();
 

$output = array(); can be deleted.

+++ b/core/modules/picture/picture.module
@@ -274,50 +291,54 @@ function theme_picture($variables) {
 /**
- * Returns HTML for a source tag.
+ * Prepares variables for source tag templates.
+ *
+ * Default template: picture-source.html.twig.

"Prepares variables for /picture source/ templates."

--- /dev/null
+++ b/core/modules/picture/templates/picture-source.html.twig
@@ -0,0 +1,40 @@
+{#
+/**
+ * @file
+ * Default theme implementation to display a picture source tag.
+ *
+ * Available variables:
+ * - media: The media query to use.
+ * - dimensions: The width and height of the image (if known).
+ * - attributes: Additional HTML attributes for source tag.
+ *
+ * One of the following two variables will be available:
+ * - srcset: A group of image sources/resolutions used to display different
+ *   image resolutions at different breakpoints.
+ * - src: Either the path of the image file (relative to base_path()) or a
+ *   full URL.
+ *
+ * @see template_preprocess()
+ * @see template_preprocess_picture_source()
+ *
+ * @ingroup themeable
+ */
+#}
+{#
+  The Attribute object keeps track of which attributes have been printed and
+  will only print an attribute once. We create a new variable here to allow
+  printing the entire set of attributes as many times as we'd like.
+#}
+{% set src_attributes %}{{ attributes }}{% endset %}
+{% if media %}
+  {% if src %}
+  <!-- <source media="{{ media }}" src="{{ src }}"{{ src_attributes }} /> -->
+    <source media="{{ media }}" src="{{ src }}"{{ src_attributes }} />
+  {% elseif srcset %}
+    <!-- <source media="{{ media }}" srcset="{{ srcset }}"{{ src_attributes }} /> -->
+    <source media="{{ media }}" srcset="{{ srcset }}"{{ src_attributes }} />
+  {% endif %}
+{% else %}
+  <!-- <source src="{{ src }}"{{ attributes }} /> -->
+  <source src="{{ src }}"{{ attributes }} />
+{% endif %}

1. The form of {% set src_attributes %}{{ attributes }}{% endset %} should be {% set src_attributes = attributes %}. Also let's call that var 'attributes_copy' or something that doesn't blur in with all the other 'src' text throughout this template.

1.b. Also, the comment above, "...as many times as we'd like" I think should just be "...a second time for the commented source tags."

2. Since {{ attributes }} and {{ attributes_copy }} (or whatever we call it) are each only being printed once, let's use them like this:

{{ attributes_copy }} for the commented-out <source tags, and {{ attributes }} for the non-commented-out <source tags.

3. if src / elseif srcset should use empty tests, so we should have
{% if src is not empty %} ... {% if srcset is not empty %}. This will keep things working if we ever implement strict variables Twig, where an undefined src/srcset would throw errors without the empty (or is defined) check.

4. Actually, since all of the variables are actually attributes, why not put them into the attributes variable?. As is it, the only attributes are width and height dimensions, which with the current docblock, is actually not clear.

The docblock would need restructuring, and our template code would look more like this:

{% if attributes.media is not media %}
...
{% if attributes.src is not empty %}
...
<!-- <source media="{{ attributes_copy.media }}" src="{{ attributes_copy.src }}"{{ attributes_copy }} />
...
<source media="{{ attributes.media }}" src="{{ attributes.src }}"{{ attributes }} /> 
--- /dev/null
+++ b/core/modules/picture/templates/picture.html.twig
@@ -0,0 +1,24 @@
+{#
+/**
+ * @file
+ * Default theme implementation to display a picture tag.
+ *
+ * Available variables:
+ * - sources: Source elements.
+ * - attributes: HTML attributes for picture element.
+ * - fallback: Fallback image for non-JS browsers.
+ *
+ * @see template_preprocess()
+ * @see template_preprocess_picture()
+ *
+ * @ingroup themeable
+ */
+#}
+{% if sources %}
+  <picture{{ attributes }}>
+    {% for source in sources %}
+      {{ source }}
+    {% endfor %}
+    <noscript>{{ fallback }}</noscript>
+  </picture>

Edits:

1. attributes: HTML attributes for /the/ picture /tag/.

2. fallback: Fallback image /tag/ for non-JS browsers.

star-szr’s picture

Assigned: Unassigned » star-szr

I'm on it, thanks for the thorough review @steveoliver!

star-szr’s picture

Assigned: star-szr » steveoliver
Status: Needs work » Needs review
FileSize
5.86 KB
13.36 KB

This should address all of #17, other than these exceptions:

  1. "Prepares variables for /picture source/ templates."

    I think "source tag templates" is valid, picture-source.html.twig outputs the <source> tags, picture.html.twig outputs the <picture> tag. I could go either way on this though.

  2. We discussed {% if src is not empty %} on IRC, the 'is not empty' is not needed in this case.

Changes:
Now template_preprocess_picture_source() builds the Attribute object based on the passed variables and creates two variables to allow for printing both in the template. picture-source.html.twig is much simpler now that everything is in attributes.

Markup tested again and still matches up with HEAD.

steveoliver’s picture

Update: will be reviewing this Thursday, incase anyone else wants to review/RTBC in the meantime.

In the meantime, a few things that

+++ b/core/modules/picture/picture.module
@@ -155,7 +155,7 @@ function picture_theme() {
     'picture' => array(
       'variables' => array(
         'style_name' => NULL,
-        'path' => NULL,
+        'uri' => NULL,
         'width' => NULL,
         'height' => NULL,

API change.
Is it worth doing this? If so, why? and we'll need a change notice.

+++ b/core/modules/picture/picture.module
@@ -171,20 +172,24 @@ function picture_theme() {
     'picture_source' => array(
       'variables' => array(
         'src' => NULL,
         'srcset' => NULL,
-        'dimension' => NULL,
+        'dimensions' => array(),
         'media' => NULL,
       ),
+      'template' => 'picture-source',

API change/cleanup (typo fix). Change notice?

tlattimore’s picture

re #20: I am with steveoliver. I don't think we need to change path to uri. Path is pretty commonly used across core for things url's passed. Re-rolled patch and removed this change (see interdiff.txt).

I do however think that dimensions should expect an array as it's value and not NULL, as this will always be an array. So I am tagging for change notification to be done.

Status: Needs review » Needs work

The last submitted patch, 1898442-twig-picture-module-21.patch, failed testing.

star-szr’s picture

Assigned: steveoliver » Unassigned

We'll need to change 'uri' back to 'path' throughout the patch.

tlattimore’s picture

I believe what cottser is saying is that 'path' needs to be changed to 'url' across the whole patch and not just one place like I did in #21. Correct?

kgoel’s picture

Assigned: Unassigned » kgoel

Going to work on this issue.

kgoel’s picture

Issue summary: View changes

Adds testing steps

kgoel’s picture

Status: Needs work » Needs review
FileSize
10.44 KB
10.28 KB

Patch attached.

kgoel’s picture

Replaced uri with path in picture.module. Templates look fine.

Status: Needs review » Needs work

The last submitted patch, 1898442-twig-picture-module-27.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
13.28 KB
13.12 KB

Another try

Status: Needs review » Needs work

The last submitted patch, 1898442-twig-picture-module-29.patch, failed testing.

star-szr’s picture

Assigned: kgoel » star-szr

Thanks @kgoel, you did good in the end :) I wasn't careful enough when I was looking at this with you last night, I will straighten this out later today. I wasn't clear enough in my task description in #23 and this rollback requires a bit of familiarity with this issue's history.

star-szr’s picture

Assigned: star-szr » steveoliver
Status: Needs work » Needs review
FileSize
2.37 KB
13.39 KB

theme_picture() before this patch was not even using the correct variables. picture_theme() declares:

    'picture' => array(
      'variables' => array(
        'style_name' => NULL,
        'path' => NULL,
        'width' => NULL,
        'height' => NULL,
        'alt' => '',
        'title' => NULL,
        'attributes' => array(),
        'breakpoints' => array(),
      ),
  ),

But then theme_picture() looks like this:

/**
 * Returns HTML for a picture.
 *
 * @param $variables
 *   An associative array containing:
 *   - uri: Either the path of the image file (relative to base_path()) or a
 *     full URL.
 *   - width: The width of the image (if known).
 *   - height: The height of the image (if known).
 *   - alt: The alternative text for text-based browsers.
 *   - breakpoints: An array containing breakpoints.
 *
 * @ingroup themeable
 */
function theme_picture($variables) {

So things should be good now. Interdiff is from #29.

I did another manual test and compared markup via visual diff and DaisyDiff and markup is still looking good.

Back to you, @steveoliver :)

star-szr’s picture

FileSize
931 bytes
13.68 KB

And here are the missing docs based on the hook_theme() declaration. Grabbed 'title' and 'style_name' descriptions from theme_image_style().

steveoliver’s picture

Status: Needs review » Needs work

I got through maybe a 50% review of this. For now:

+++ b/core/modules/picture/templates/picture-source.html.twig
@@ -0,0 +1,38 @@
+/**
+ * @file
+ * Default theme implementation to display a picture source tag.
+ *
+ * Available variables:
+ * - attributes: HTML attributes for the source tag, including:
+ *   - media: The media query to use.
+ *   - width: The width of the image (if known).
+ *   - height: The width of the image (if known).
+ *   One of the following two variables will be available:
+ *   - src: Either the path of the image file (relative to base_path()) or a
+ *     full URL.
+ *   - srcset: A group of image sources/resolutions used to display different
+ *     image resolutions at different breakpoints.
+ * - attributes_copy: A copy of the HTML attributes in the 'attributes' variable
+ *   to allow for printing the attributes a second time for the commented source
+ *   tags. Drupal's Attribute object keeps track of which attributes have been
+ *   printed and will only print an attribute once.
+ *
+ * @see template_preprocess()
+ * @see template_preprocess_picture_source()
+ *
+ * @ingroup themeable
+ */
+#}
+{% if attributes.media %}
+  {% if attributes.src %}
+  <!-- <source{{ attributes_copy }} /> -->
+    <source{{ attributes }} />
+  {% elseif attributes.srcset %}
+    <!-- <source{{ attributes_copy }} /> -->
+    <source{{ attributes }} />
+  {% endif %}
+{% else %}
+  <!-- <source{{ attributes_copy }} /> -->
+  <source{{ attributes }} />
+{% endif %}

1. DocBlock change
1.a. Change "One of the following two variables will be available" to "One of the following two attributes will also be set."
1.b. attributes_copy: Change "A copy of the HTML attributes in the 'attributes' variable..." to just "A copy of the 'attributes' variable..."

2. All the conditions render the same tags. I think we literally need zero checks and only the two source tags.

+++ b/core/modules/picture/templates/picture-formatter.html.twig
@@ -0,0 +1,23 @@
+{#
+/**
+ * @file
+ * Default theme implementation to display a picture field formatter.
+ *
+ * Available variables:
+ * - image: Picture element, if picture has breakpoint sources, otherwise img
+ *   element.
+ * - path: (optional) Contains information about the link.
+ *   - path.path: The URL the image links to.
+ *   - path.attributes: HTML attributes for the link tag.
+ *
+ * @see template_preprocess()
+ * @see template_preprocess_picture_formatter()
+ *
+ * @ingroup themeable
+ */
+#}
+{% if path %}
+  <a href="{{ path.path }}"{{ path.attributes }}>{{ image }}</a>
+{% else %}
+  {{ image }}

3. It'd really make sense if the 'path' var were named 'link', as that's actually what it is and that's how we describe it. Could be non-Twig API follow-up, perhaps.

star-szr’s picture

Assigned: steveoliver » Unassigned
Issue tags: +Novice

Relating to #3, those shouldn't be documented as path.path and path.attributes either, just 'path' and 'attributes'. Indent means 'add a dot'.

Great catches @steveoliver, tagging as a good novice task.

cyborg_572’s picture

Issue tags: -Novice
FileSize
2.51 KB
13.37 KB
cyborg_572’s picture

Status: Needs work » Needs review

Forgot to update issue status.

c4rl’s picture

Title: Convert picture module to Twig » picture.module - Convert theme_ functions to Twig

Per #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions, which are lower in priority than PHPTemplate conversion issues.

attiks’s picture

FYI: keep an eye on #1883526: Decide on the picture polyfill to use, because the output will no longer use picture and source tags

webthingee’s picture

Unable to find the process described in Step 3 for testing.

jstoller’s picture

Issue tags: -Needs change record, -Twig

#36: 1898442-36.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs change record, +Twig

The last submitted patch, 1898442-36.patch, failed testing.

jrglasgow’s picture

Assigned: Unassigned » jrglasgow

I will attempt to get the patch working

jrglasgow’s picture

Status: Needs work » Needs review

#36: 1898442-36.patch queued for re-testing.

jrglasgow’s picture

Assigned: jrglasgow » Unassigned
rwohleb’s picture

Running profile against 1898442-36.patch

rwohleb’s picture

=== 8.x..8.x compared (519ff47950466..519ff57991ea2):

ct : 586,572|586,572|0|0.0%
wt : 5,528,379|5,523,830|-4,549|-0.1%
cpu : 5,438,279|5,428,694|-9,585|-0.2%
mu : 53,683,752|53,684,856|1,104|0.0%
pmu : 53,875,312|53,876,392|1,080|0.0%

Profiler output

=== 8.x..issue-1898442 compared (519ff47950466..519ff595715f0):

ct : 586,572|586,887|315|0.1%
wt : 5,528,379|5,556,077|27,698|0.5%
cpu : 5,438,279|5,460,500|22,221|0.4%
mu : 53,683,752|53,687,264|3,512|0.0%
pmu : 53,875,312|53,878,152|2,840|0.0%

Profiler output

benjy’s picture

@rwohleb, links to the profiler output seem to be your local machine.

rwohleb’s picture

@benjy, I forgot to remove those links from the xhprof-kit output. I don't have an API key yet to get it online.

I realized there was an issue with my previous profiling, so here is an updated version.

=== 8.x..8.x compared (519fff4bc13bf..51a0008669b58):

ct : 610,847|610,847|0|0.0%
wt : 5,733,916|5,743,103|9,187|0.2%
cpu : 5,638,489|5,641,707|3,218|0.1%
mu : 58,147,712|58,147,712|0|0.0%
pmu : 58,361,520|58,361,520|0|0.0%

=== 8.x..issue-1898442 compared (519fff4bc13bf..51a000a29e44d):

ct : 610,847|611,132|285|0.0%
wt : 5,733,916|5,700,138|-33,778|-0.6%
cpu : 5,638,489|5,599,642|-38,847|-0.7%
mu : 58,147,712|58,151,928|4,216|0.0%
pmu : 58,361,520|58,366,384|4,864|0.0%

rwohleb’s picture

Issue summary: View changes

Remove sandbox link

ezeedub’s picture

Issue summary: View changes

Updated issue summary.

joelpittet’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

This one needs a re-roll.

error: patch failed: core/modules/picture/picture.module:192
error: core/modules/picture/picture.module: patch does not apply
duellj’s picture

Status: Needs work » Needs review
FileSize
13.45 KB

Here's a reroll of the patch from #36

jenlampton’s picture

Issue tags: -Needs reroll

updating tags

jenlampton’s picture

Issue summary: View changes

add to related

azinoman’s picture

Assigned: Unassigned » azinoman
adamcowboy’s picture

Assigned: azinoman » Unassigned

The patch works! It resizes correctly.

adamcowboy’s picture

Status: Needs review » Reviewed & tested by the community

(forgot to change)

star-szr’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice, +Needs manual testing

Would like another round of manual testing please, since it's been a while.

+++ b/core/modules/picture/templates/picture-formatter.html.twigundefined
@@ -0,0 +1,23 @@
+ * @see template_preprocess()

+++ b/core/modules/picture/templates/picture-source.html.twigundefined
@@ -0,0 +1,28 @@
+ * @see template_preprocess()

+++ b/core/modules/picture/templates/picture.html.twigundefined
@@ -0,0 +1,24 @@
+ * @see template_preprocess()

We'll also need to remove these lines per #2013094: [policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file.

andypost’s picture

azinoman’s picture

Status: Needs work » Needs review

Applied Cottser's changes. And rerolled patch. Needs Review

azinoman’s picture

FileSize
17.45 KB

Whoops. Forgot to attach the file

joelpittet’s picture

You got some whitespace issues in there, you can just remove that entire line not just the @see

jenlampton’s picture

Status: Needs review » Needs work

Also looks like this patch includes changes for aggregator module. Make sure you stash your previous changes before starting on a new issue :)

jenlampton’s picture

Status: Needs work » Needs review
FileSize
69.47 KB
69.52 KB
1.66 KB
13.57 KB

Okay, here's a reroll of the patch in #52, with interdiff.

Also, manual testing results:
Before
picture-before.png
After
picture-after.png

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice, -Needs manual testing

RTBC as per #56 being covered.

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/picture/picture.moduleundefined
@@ -295,27 +317,24 @@ function theme_picture($variables) {
-  $output = array();
-  if (isset($variables['media']) && !empty($variables['media'])) {
-    if (!isset($variables['srcset'])) {
-      $output[] = '<!-- <source media="' . $variables['media'] . '" src="' . $variables['src'] . '" ' . new Attribute($variables['dimensions']) . ' /> -->';
-      $output[] = '<source media="' . $variables['media'] . '" src="' . $variables['src'] . '" ' . new Attribute($variables['dimensions']) . '/>';
-    }
-    elseif (!isset($variables['src'])) {
-      $output[] = '<!-- <source media="' . $variables['media'] . '" srcset="' . $variables['srcset'] . '" ' . new Attribute($variables['dimensions']) . ' /> -->';
-      $output[] = '<source media="' . $variables['media'] . '" srcset="' . $variables['srcset'] . '" ' . new Attribute($variables['dimensions']) . ' />';
+function template_preprocess_picture_source(&$variables) {
+  $attributes = array();
+  foreach (array('media', 'src', 'srcset') as $attribute) {
+    if (!empty($variables[$attribute])) {
+      $attributes[$attribute] = $variables[$attribute];
+      unset($variables[$attribute]);
     }
   }
-  else {
-    $output[] = '<!-- <source src="' . $variables['src'] . '" ' . new Attribute($variables['dimensions']) . ' /> -->';
-    $output[] = '<source src="' . $variables['src'] . '" ' . new Attribute($variables['dimensions']) . '/>';
-  }
-  return implode("\n", $output);

The logic here is not quite equivalent... the original function is extremely messy but it seems to have the following qualities that the new one does not have. I do not know if this is important this is just from reviewing the code. These qualities were:

  • if media was not set assume src is set and use it.
  • If media is set and srcset is not not set assume src is and use it.
  • If media is set and src is not not set assume srcset is and use it.

The point being is that it was impossible to have both src and srcset as properties on the same tag... it now is...

I'm more than happy to convince that this is not a problem. The original logic is full of code smells to me. And seems to be undocumented as to why :)

attiks’s picture

#65 It's important, we can use either src or srcset, not both at the same time, but the code only sets one, so I think we are lacking some documentation, feel free to add the comments or create a follow up and I'll add them.

According to the working draft, both can be defined at the same time (and even on the picture element as well. The current polyfill will first check srcset first and assumes a src attribute in the other case.

The specs says: "There are three ways to specify an image resource, the src attribute, the srcset attribute, or source elements. The srcset attribute overrides both the src attribute and the elements; the src attribute overrides the elements."

jenlampton’s picture

Status: Needs work » Reviewed & tested by the community

After reading #67 it sounds like the new, cleaner code is fine as-is. changing back to RTBC.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

#62 doesn't apply to HEAD.

Les Lim’s picture

Assigned: Unassigned » Les Lim

Re-rolling #62.

Les Lim’s picture

Status: Needs work » Needs review
FileSize
13.34 KB

Reroll changes image_style_url() calls to entity_load('image_style').

Status: Needs review » Needs work

The last submitted patch, core-1898442-70-picture_module_to_twig.patch, failed testing.

Les Lim’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
13.34 KB

Updated patch to use $variables['path'] instead of $variables['uri'].

Les Lim’s picture

Assigned: Les Lim » Unassigned
Issue tags: -Needs reroll

Unassigning.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC, thanks @Les Lim

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

@Les Lim - why did you change path to uri ... in the issue summary it states that this patch is changing path to uri.

Let's use uri where it contains a string that will be used to build the uri and path where it contains an array with options for url() or l()

Les Lim’s picture

Hm, I'm about to go to bed, but if I recall, $variables['uri'] isn't an index that gets passed to the preprocess function. That's where the 16 exceptions from the straight reroll at #70 come from. $variables['path'] is available, though. I can go back and verify this in the morning.

pplantinga’s picture

This issue has been fixed in #2026319: Mis-named variables in picture_theme() so we can go back to using $variables['uri']

pplantinga’s picture

Status: Needs work » Needs review
pplantinga’s picture

Reroll of #70

Status: Needs review » Needs work

The last submitted patch, core-1898442-79-picture_module_to_twig.patch, failed testing.

thedavidmeister’s picture

alexpott’s picture

steinmb’s picture

Assigned: Unassigned » steinmb

Will be working on this

xjm’s picture

We only add the "Needs change notification" tag after commit since things can change. :)

pplantinga’s picture

Status: Needs work » Needs review
FileSize
14.44 KB

reroll of #79

Status: Needs review » Needs work

The last submitted patch, core-1898442-85-picture_module_to_twig.patch, failed testing.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
574 bytes
14.44 KB

Missed one.

Status: Needs review » Needs work

The last submitted patch, core-1898442-87-picture_module_to_twig.patch, failed testing.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
689 bytes
15.11 KB

one more.

steinmb’s picture

Thanx for re-rolling. I started on it did not work 100% after I rebased it and then I got swamped :-/

Testing #89 on HEAD of 8.x

<picture alt="">
<!-- <source src="http://d8.dev/sites/default/files/styles/thumbnail/public?itok=ssaedzUG" width="100" height="67" /> -->
<source height="67" width="100" src="http://d8.dev/sites/default/files/styles/thumbnail/public?itok=ssaedzUG"></source>

<!-- <source media="(min-width: 0px)" src="http://d8.dev/sites/default/files/styles/thumbnail/public?itok=ssaedzUG" width="100" height="67" /> -->
<source height="67" width="100" src="http://d8.dev/sites/default/files/styles/thumbnail/public?itok=ssaedzUG" media="(min-width: 0px)"></source>

<!-- <source media="all and (min-width: 560px) and (max-width: 850px)" src="http://d8.dev/sites/default/files/styles/medium/public?itok=U21kPsmu" width="220" height="147" /> -->
<source height="147" width="220" src="http://d8.dev/sites/default/files/styles/medium/public?itok=U21kPsmu" media="all and (min-width: 560px) and (max-width: 850px)"></source>

<!-- <source media="all and (min-width: 851px)" src="http://d8.dev/sites/default/files/styles/large/public?itok=rGS4QIRH" width="480" height="320" /> -->
<source height="320" width="480" src="http://d8.dev/sites/default/files/styles/large/public?itok=rGS4QIRH" media="all and (min-width: 851px)"></source>

<noscript>&lt;img class="image-style-thumbnail" typeof="foaf:Image" src="http://d8.dev/sites/default/files/styles/thumbnail/public?itok=ssaedzUG" width="100" height="67" alt="" /&gt;</noscript>
  <img width="480" height="320" alt="" src="http://d8.dev/sites/default/files/styles/large/public?itok=rGS4QIRH"></picture>

So looking at this it seems to do it's thing but I have problem getting access to the generated versions of the attached image, it keep giving me a 403.

Resizing my browser window and firebug throws:

"NetworkError: 403 Forbidden - http://d8.dev/sites/default/files/styles/medium/public/?itok=U21kPsmu"
?itok=U21kPsmu
"NetworkError: 403 Forbidden - http://d8.dev/sites/default/files/styles/medium/public/?itok=U21kPsmu"
?itok=U21kPsmu
"NetworkError: 403 Forbidden - http://d8.dev/sites/default/files/styles/medium/public/?itok=U21kPsmu"
?itok=U21kPsmu
"NetworkError: 403 Forbidden - http://d8.dev/sites/default/files/styles/medium/public/?itok=U21kPsmu"
?itok=U21kPsmu
"NetworkError: 403 Forbidden - http://d8.dev/sites/default/files/styles/large/public/?itok=rGS4QIRH"
?itok=rGS4QIRH
"NetworkError: 403 Forbidden - http://d8.dev/sites/default/files/styles/large/public/?itok=rGS4QIRH"
?itok=rGS4QIRH
"NetworkError: 403 Forbidden - http://d8.dev/sites/default/files/styles/medium/public/?itok=U21kPsmu"
?itok=U21kPsmu
"NetworkError: 403 Forbidden - http://d8.dev/sites/default/files/styles/thumbnail/public/?itok=ssaedzUG"
?itok=ssaedzUG
"NetworkError: 403 Forbidden - http://d8.dev/sites/default/files/styles/thumbnail/public/?itok=ssaedzUG"
?itok=ssaedzUG
"NetworkError: 403 Forbidden - http://d8.dev/sites/default/files/styles/large/public/?itok=rGS4QIRH"

And yes, loading the generated version. example http://d8.dev/sites/default/files/styles/thumbnail/public/field/image/gr... works just fine.

steinmb’s picture

Backtrace from drupal log:

Notice: Undefined offset: 2 in Drupal\image\PathProcessor\PathProcessorImageStyles->processInbound() (line 49 of core/modules/image/lib/Drupal/image/PathProcessor/PathProcessorImageStyles.php).

Drupal\image\PathProcessor\PathProcessorImageStyles->processInbound('sites/default/files/styles/large/public', Object)
Drupal\Core\PathProcessor\PathProcessorManager->processInbound('sites/default/files/styles/large/public', Object)
Drupal\Core\EventSubscriber\PathSubscriber->onKernelRequestConvertPath(Object)
call_user_func(Array, Object)
Symfony\Component\EventDispatcher\EventDispatcher->doDispatch(Array, 'kernel.request', Object)
Symfony\Component\EventDispatcher\EventDispatcher->dispatch('kernel.request', Object)
Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.request', Object)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\HttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)
drupal_handle_request()


Notice: Undefined offset: 2 in Drupal\image\PathProcessor\PathProcessorImageStyles->processInbound() (line 49 of core/modules/image/lib/Drupal/image/PathProcessor/PathProcessorImageStyles.php).

Drupal\image\PathProcessor\PathProcessorImageStyles->processInbound('sites/default/files/styles/large/public', Object)
Drupal\Core\PathProcessor\PathProcessorManager->processInbound('sites/default/files/styles/large/public', Object)
Drupal\Core\EventSubscriber\PathSubscriber->onKernelRequestConvertPath(Object)
call_user_func(Array, Object)
Symfony\Component\EventDispatcher\EventDispatcher->doDispatch(Array, 'kernel.request', Object)
Symfony\Component\EventDispatcher\EventDispatcher->dispatch('kernel.request', Object)
Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.request', Object)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 2)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 2, 1)
Drupal\Core\HttpKernel->handle(Object, 2, 1)
Symfony\Component\HttpKernel\EventListener\ExceptionListener->onKernelException(Object)
call_user_func(Array, Object)
Symfony\Component\EventDispatcher\EventDispatcher->doDispatch(Array, 'kernel.exception', Object)
Symfony\Component\EventDispatcher\EventDispatcher->dispatch('kernel.exception', Object)
Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.exception', Object)
Symfony\Component\HttpKernel\HttpKernel->handleException(Object, Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\HttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)
drupal_handle_request()
pplantinga’s picture

Status: Needs review » Needs work

I guess this needs work then...

steinmb’s picture

Assigned: steinmb » Unassigned

Some debugging information dumped from template_preprocess_picture_formatter()

variables['image'] :

array(
  '#theme' => 'picture',
  '#style_name' => 'thumbnail',
  '#path' => 'public://field/image/brown.png',
  '#width' => '800',
  '#height' => '533',
  '#breakpoints' => array(
    'theme.bartik.mobile' => array(
      '1x' => 'thumbnail',
    ),
    'theme.bartik.narrow' => array(
      '1x' => 'medium',
    ),
    'theme.bartik.wide' => array(
      '1x' => 'large',
    ),
  ),
  '#alt' => '',
  '#title' => NULL,
  '#attributes' => NULL,
)

Looks right to me. With public://field/image/brown.png should it be able to build the correct URI.

Here goes the whole thingy:

array(
  'item' => array(
    'alt' => '',
    'title' => '',
    'width' => '800',
    'height' => '533',
    'entity' => Drupal\file\Plugin\Core\Entity\File::__set_state(array(
       'values' => 
      array (
        'langcode' => 
        array (
          'und' => 'und',
        ),
        'fid' => 
        array (
          'und' => '3',
        ),
        'uuid' => 
        array (
          'und' => 'd0617450-1e47-4ef1-b309-9db4e260d8cf',
        ),
        'uid' => 
        array (
          'und' => '1',
        ),
        'filename' => 
        array (
          'und' => 'brown.png',
        ),
        'uri' => 
        array (
          'und' => 'public://field/image/brown.png',
        ),
        'filemime' => 
        array (
          'und' => 'image/png',
        ),
        'filesize' => 
        array (
          'und' => '1703',
        ),
        'status' => 
        array (
          'und' => '1',
        ),
        'timestamp' => 
        array (
          'und' => '1376428475',
        ),
      ),
       'bundle' => 'file',
       'fields' => 
      array (
        'fid' => 
        array (
          'und' => 
          Drupal\Core\Entity\Field\Field::__set_state(array(
             'list' => 
            array (
              0 => 
              Drupal\Core\Entity\Plugin\DataType\IntegerItem::__set_state(array(
                 'values' => 
                array (
                  'value' => '3',
                ),
                 'properties' => 
                array (
                ),
                 'definition' => 
                array (
                  'list' => false,
                  'label' => 'File ID',
                  'description' => 'The file ID.',
                  'type' => 'integer_field',
                  'read-only' => true,
                ),
                 'name' => 0,
                 'parent' => NULL,
              )),
            ),
             'definition' => 
            array (
              'label' => 'File ID',
              'description' => 'The file ID.',
              'type' => 'integer_field',
              'read-only' => true,
              'list' => true,
            ),
             'name' => 'fid',
             'parent' => NULL,
          )),
        ),
        'uri' => 
        array (
          'und' => 
          Drupal\Core\Entity\Field\Field::__set_state(array(
             'list' => 
            array (
              0 => 
              Drupal\Core\Entity\Plugin\DataType\StringItem::__set_state(array(
                 'values' => 
                array (
                  'value' => 'public://field/image/brown.png',
                ),
                 'properties' => 
                array (
                ),
                 'definition' => 
                array (
                  'list' => false,
                  'label' => 'URI',
                  'description' => 'The URI to access the file (either local or remote).',
                  'type' => 'string_field',
                ),
                 'name' => 0,
                 'parent' => NULL,
              )),
            ),
             'definition' => 
            array (
              'label' => 'URI',
              'description' => 'The URI to access the file (either local or remote).',
              'type' => 'string_field',
              'list' => true,
            ),
             'name' => 'uri',
             'parent' => NULL,
          )),
        ),
      ),
       'bcEntity' => NULL,
       'language' => NULL,
       'languages' => 
      array (
        'en' => 
        Drupal\Core\Language\Language::__set_state(array(
           'name' => 'English',
           'id' => 'en',
           'direction' => 0,
           'weight' => 0,
           'default' => true,
           'method_id' => NULL,
           'locked' => 0,
        )),
        'und' => 
        Drupal\Core\Language\Language::__set_state(array(
           'name' => 'Not specified',
           'id' => 'und',
           'direction' => 0,
           'weight' => 1,
           'default' => false,
           'method_id' => NULL,
           'locked' => true,
           'enabled' => true,
        )),
        'zxx' => 
        Drupal\Core\Language\Language::__set_state(array(
           'name' => 'Not applicable',
           'id' => 'zxx',
           'direction' => 0,
           'weight' => 2,
           'default' => false,
           'method_id' => NULL,
           'locked' => true,
           'enabled' => true,
        )),
      ),
       'fieldDefinitions' => 
      array (
        'fid' => 
        array (
          'label' => 'File ID',
          'description' => 'The file ID.',
          'type' => 'integer_field',
          'read-only' => true,
          'list' => true,
        ),
        'uuid' => 
        array (
          'label' => 'UUID',
          'description' => 'The file UUID.',
          'type' => 'uuid_field',
          'read-only' => true,
          'list' => true,
        ),
        'langcode' => 
        array (
          'label' => 'Language code',
          'description' => 'The file language code.',
          'type' => 'language_field',
          'list' => true,
        ),
        'uid' => 
        array (
          'label' => 'User ID',
          'description' => 'The user ID of the file.',
          'type' => 'entity_reference_field',
          'settings' => 
          array (
            'target_type' => 'user',
          ),
          'list' => true,
        ),
        'filename' => 
        array (
          'label' => 'Filename',
          'description' => 'Name of the file with no path components.',
          'type' => 'string_field',
          'list' => true,
        ),
        'uri' => 
        array (
          'label' => 'URI',
          'description' => 'The URI to access the file (either local or remote).',
          'type' => 'string_field',
          'list' => true,
        ),
        'filemime' => 
        array (
          'label' => 'File MIME type',
          'description' => 'The file\'s MIME type.',
          'type' => 'string_field',
          'list' => true,
        ),
        'filesize' => 
        array (
          'label' => 'File size',
          'description' => 'The size of the file in bytes.',
          'type' => 'boolean_field',
          'list' => true,
        ),
        'status' => 
        array (
          'label' => 'Status',
          'description' => 'The status of the file, temporary (0) and permanent (1)',
          'type' => 'integer_field',
          'list' => true,
        ),
        'timestamp' => 
        array (
          'label' => 'Created',
          'description' => 'The time that the node was created.',
          'type' => 'integer_field',
          'list' => true,
        ),
      ),
       'uriPlaceholderReplacements' => NULL,
       'activeLangcode' => 'und',
       'translations' => 
      array (
        'und' => 
        array (
          'status' => 1,
        ),
      ),
       'translationInitialize' => false,
       'entityType' => 'file',
       'enforceIsNew' => NULL,
       'newRevision' => false,
       'isDefaultRevision' => true,
    )),
    'label' => NULL,
    'access' => NULL,
    'target_id' => '3',
  ),
  'path' => '',
  'image_style' => 'thumbnail',
  'breakpoints' => array(
    'theme.bartik.mobile' => array(
      '1x' => 'thumbnail',
    ),
    'theme.bartik.narrow' => array(
      '1x' => 'medium',
    ),
    'theme.bartik.wide' => array(
      '1x' => 'large',
    ),
  ),
  'theme_hook_original' => 'picture_formatter',
  'theme_hook_suggestions' => array(),
  'directory' => 'core/modules/picture',
  'attributes' => array(),
  'title_attributes' => array(),
  'content_attributes' => array(),
  'title_prefix' => array(),
  'title_suffix' => array(),
  'db_is_active' => TRUE,
  'is_admin' => TRUE,
  'logged_in' => TRUE,
  'is_front' => FALSE,
  'user' => Drupal\Core\Session\UserSession::__set_state(array(
     'uid' => '1',
     'roles' => 
    array (
      0 => 'authenticated',
      1 => 'administrator',
    ),
     'session' => 'dblog_overview_filter|a:0:{}',
     'timestamp' => '1376436555',
     'name' => 'admin',
     'preferred_langcode' => '',
     'preferred_admin_langcode' => '',
     'mail' => 'a@a.b',
     'timezone' => 'Europe/Oslo',
     'uuid' => NULL,
     'langcode' => '',
     'theme' => '',
     'signature' => '',
     'signature_format' => NULL,
     'created' => '1376426732',
     'access' => '1376436206',
     'login' => '1376426806',
     'status' => '1',
     'init' => 'a@a.b',
     'hostname' => '127.0.0.1',
  )),
  'image' => array(
    '#theme' => 'picture',
    '#style_name' => 'thumbnail',
    '#path' => 'public://field/image/brown.png',
    '#width' => '800',
    '#height' => '533',
    '#breakpoints' => array(
      'theme.bartik.mobile' => array(
        '1x' => 'thumbnail',
      ),
      'theme.bartik.narrow' => array(
        '1x' => 'medium',
      ),
      'theme.bartik.wide' => array(
        '1x' => 'large',
      ),
    ),
    '#alt' => '',
    '#title' => NULL,
    '#attributes' => NULL,
  ),
)
jenlampton’s picture

Assigned: Unassigned » jenlampton
Issue tags: -Needs reroll

THREE TEMPLATES? this is just silly. no wonder its so hard to track down the problem. I'll take a stab at consolidating these and figuring out why the derivatives can't be generated.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
738 bytes
15.11 KB

Fixed the problem. Re-running tests. Next patch will consolidate temlates.

jenlampton’s picture

this patch removes the unnecessary picture-source.html.twig template. Will do one more tomorrow.

Status: Needs review » Needs work

The last submitted patch, core-convert_picture_twig-1898442-96.patch, failed testing.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
1.56 KB
14.43 KB

This fixes the failing test, and cleans up some incorrect text in the tests. Going to consolidate in the next patch.

jenlampton’s picture

And here's the last consolidation, getting us down to one template for picture elements. woot!

Status: Needs review » Needs work

The last submitted patch, core-convert_picture_twig-1898442-99.patch, failed testing.

RainbowArray’s picture

Fun little four-hour patch that still needs further review.

Trying to correct a couple bugs in the patch on #99. Haven't been able to find the origin of the second bug. Trying again tomorrow.

Here's the patch and the interdiff with #99.

RainbowArray’s picture

Fun little four-hour patch that still needs further review.

Trying to correct a couple bugs in the patch on #99. Haven't been able to find the origin of the second bug. Trying again tomorrow.

Here's the patch and the interdiff with #99.

RainbowArray’s picture

Status: Needs work » Needs review

Accidentally hit save twice. Sorry. Marking as needs review so it's clearer which error remains.

RainbowArray’s picture

Also, I should note that I manually set up mappings and followed the other instructions at the top of this issue, and I wasn't able to reproduce the error that testbot is finding. So there may be something wrong with the way the test is written.

Status: Needs review » Needs work

The last submitted patch, picture-twig-1898442-101.patch, failed testing.

joelpittet’s picture

@mdrummond nice work getting rid of half the errors! Sorry it took 4 hours:S

Keep in mind some tests failures will happen in CLI but not in the web interface, which is kind of annoying but good to keep in mind when you're going crazy.

RainbowArray’s picture

larowlan did all the hard work of figuring out what was going wrong. I just set up the patch and botched up my git patch workflow. Anyhow, hopefully we'll figure out the rest soon.

larowlan’s picture

Status: Needs work » Needs review
FileSize
16.94 KB
6.55 KB

This should fix the fails.
The issue is that the new logic (in earlier patches) doesn't handle what happens when there is no picture mapping and therefore no breakpoints. The old code in theme_picture_formatter exited early by outputting the original image. This adds a second class of fallback if no sources exist, which is just the original image.

larowlan’s picture

woot green, thanks mdrummond, joelpittet

star-szr’s picture

Assigned: jenlampton » Unassigned

Thanks everyone. Nice diffstat!

4 files changed, 157 insertions, 200 deletions.

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
16.89 KB

reroll

joelpittet’s picture

Issue tags: -Needs reroll

This one is looking good, nice work. Untagging the re-roll.

gavin.hughes’s picture

Status: Needs review » Reviewed & tested by the community

Looks good RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

pplantinga’s picture

Issue tags: -Needs reroll
FileSize
16.9 KB

Re-roll.

pplantinga’s picture

Status: Needs work » Needs review
pplantinga’s picture

Status: Needs review » Reviewed & tested by the community
Eric_A’s picture

travis-echidna, Cottser and jeanfei have been adding green test coverage for theme_picture() and theme_picture_formatter() (next to one red assertion) in the process of trying to target a specific bug. Since this issue is RTBC and the other is not I'll try to add this new test file here, and within the hour. (Just the green assertions of course.)

Eric_A’s picture

This is #116 with tests added by travis-echidna, Cottser and jeanfei that capture some existing functionality. We'd expect both patches to be green.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, picture-1898442.120.patch, failed testing.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
2.61 KB
21.68 KB

Maybe this will fare better?

Status: Needs review » Needs work

The last submitted patch, picture-1898442.122.patch, failed testing.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
21.68 KB

Worst patch ever. Here's a replacement.

Status: Needs review » Needs work

The last submitted patch, picture-1898442.124.patch, failed testing.

jenlampton’s picture

Status: Needs work » Postponed

It looks like picture module may be removed from core, so postponing this issue until a decision has been made.

RainbowArray’s picture

Where is the discussion taking place about picture's inclusion or removal from core?

attiks’s picture

webchick’s picture

Status: Postponed » Needs work

I don't feel like that issue is particularly close to resolution, and whether the module lives in core or contrib, it'll still need to be converted to Twig. :) Therefore, I think it's ok to continue working on this.

webchick’s picture

Issue summary: View changes

Update commit message

cosmicdreams’s picture

Issue summary: View changes
joelpittet’s picture

Status: Needs work » Needs review

124: picture-1898442.124.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 124: picture-1898442.124.patch, failed testing.

steinmb’s picture

Status: Needs work » Needs review
FileSize
21.7 KB

Chasing head.

Status: Needs review » Needs work

The last submitted patch, 134: picture-1898442.134.patch, failed testing.

yanniboi’s picture

I'm just having a look at the errors being thrown by the last test.

"picture element in theme_picture() correctly renders with a NULL value for the alt option."

This is trying to be asserted:

+++ b/core/modules/picture/lib/Drupal/picture/Tests/PictureThemeFunctionsTest.php
@@ -0,0 +1,115 @@
+    $this->assertTrue(strpos($rendered_element, '<picture>') !== FALSE, 'picture element in theme_picture() correctly renders with a NULL value for the alt option.');

$rendered_element at this point is '\<\i\m\g src="h\t\t\p://localhost/drupal/sites/default/files/simpletest/355953/image-test_0.png" />'
(ignore the '\'s in the above string, the text format on the comment was trying to evaluate the image tag and I could'y work out another way of making it take the html as a string...)

Not sure what the expected outcome here should be.

yanniboi’s picture

"img element in theme_picture() correctly renders the alt option."

and

"picture element in theme_picture() correctly renders the alt option."

+++ b/core/modules/picture/lib/Drupal/picture/Tests/PictureThemeFunctionsTest.php
@@ -0,0 +1,115 @@
+    $this->assertTrue(strpos($rendered_element, $expected_result) !== FALSE, 'img element in theme_picture() correctly renders the alt option.');
+    $this->assertTrue(strpos($rendered_element, '<picture alt="">') !== FALSE, 'picture element in theme_picture() correctly renders the alt option.');
$rendered_element = '<img class="image-style-test" src="http://localhost/drupal/sites/default/files/simpletest/355953/styles/test/public/image-test_0.png?itok=Q55Ks_Y1" alt="" />';
$expected_result = '<img src="http://localhost/drupal/sites/default/files/simpletest/355953/image-test_0.png" alt="" />';
yanniboi’s picture

"theme_picture_formatter() correctly renders without breakpoints and with a NULL value for the alt option."

and

"theme_picture_formatter() correctly renders without title, alt, or path options."

and

"theme_picture_formatter() correctly renders a link fragment."

+++ b/core/modules/picture/lib/Drupal/picture/Tests/PictureThemeFunctionsTest.php
@@ -0,0 +1,115 @@
+    $this->assertEqual($expected_result, $rendered_element, 'theme_picture_formatter() correctly renders without breakpoints and with a NULL value for the alt option.');
+++ b/core/modules/picture/lib/Drupal/picture/Tests/PictureThemeFunctionsTest.php
@@ -0,0 +1,115 @@
+    $this->assertEqual($expected_result, $rendered_element, 'theme_picture_formatter() correctly renders without title, alt, or path options.');
+++ b/core/modules/picture/lib/Drupal/picture/Tests/PictureThemeFunctionsTest.php
@@ -0,0 +1,115 @@
+    $this->assertEqual($expected_result, $rendered_element, 'theme_picture_formatter() correctly renders a link fragment.');

$rendered_element is an empty string, so maybe the theme_picture_formatter function isn't working correctly.

yanniboi’s picture

I think I have found why the tests are failing for the picture_formatter theme implementation.

  1. +++ b/core/modules/picture/picture.module
    @@ -128,94 +128,30 @@ function picture_theme() {
    -function theme_picture_formatter($variables) {
    

    theme_picture_formatter() is removed, but there is no preprocess function to replace it.

  2. +++ b/core/modules/picture/picture.module
    @@ -128,94 +128,30 @@ function picture_theme() {
    -function theme_picture($variables) {
    +function template_preprocess_picture(&$variables) {
    

    theme_picture() is replaced with template_preprocess_picture()

star-szr’s picture

@yanniboi - thanks for taking a look! I believe #2009662: [REGRESSION] Replace theme() with drupal_render() in picture module is pretty relevant for the test coverage/failures, check the last handful of comments there.

Eric_A’s picture

Because of #2042773: Change #items within a theme_field() render array from an *array* to the same $items *object* used throughout the rest of the formatter pipeline the assertions added in #120 by travis-echidna, Cottser and jeanfei now need updating. They'll probably get simpler. These tests *could* be moved into their own issue *but* it would be wise to not simply ignore test coverage.

star-szr’s picture

Issue tags: +Twig conversion
star-szr’s picture

Tagging for reroll before we look at the tests again. If you start to look at the test changes please post the reroll first then an updated patch an interdiff. Thanks!

jayeshanandani’s picture

FileSize
24.32 KB

Uploaded a rerolled patch.

jayeshanandani’s picture

Issue tags: -Needs reroll
jayeshanandani’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 144: 1898442.144.patch, failed testing.

jayeshanandani’s picture

Status: Needs work » Needs review
FileSize
21.71 KB
21.71 KB

Status: Needs review » Needs work

The last submitted patch, 148: 1898442.148.patch, failed testing.

YesCT’s picture

1.
the previous patch has some noise in it about newlines at the end of file. That should be fixed.

2.
$path is unused in testPictureTheme() and $expected_result is set, then not used, then reset.
so those should be taken out or asserts should use them.

3.
The test comment is:

   * Tests usage of the theme_picture() function.

and in the code:

    // Test using theme_picture() with a NULL value for the alt option.

but a call to theme_picture() is not in the function,
just

    $rendered_element = render($element);

I can't find theme_picture() anywhere in core with this patch applied.

this patch renames it (replaces it?)

-function theme_picture($variables) {
+function template_preprocess_picture(&$variables) {

4.
in there:

      '#alt' => isset($variables['alt']) || array_key_exists('alt', $variables) ? $variables['alt'] : NULL,

5.
Also, I tried using minimal profile to test this... and it was problematic.

6.
So with standard profile, I used the steps to reproduce, and got to an image, which I inspected to find this html:

<picture alt="">
          <source src="http://localhost/drupal2/sites/default/files/styles/large/public/field/image/comments-view.png?itok=blSHCvU-" height="258" width="480"></source>
          <source height="54" width="100" src="http://localhost/drupal2/sites/default/files/styles/thumbnail/public/field/image/comments-view.png?itok=ZLZ-NHlg" media="(min-width: 0px)"></source>
          <source height="118" width="220" src="http://localhost/drupal2/sites/default/files/styles/medium/public/field/image/comments-view.png?itok=usN60KC6" media="all and (min-width: 560px) and (max-width: 850px)"></source>
          <source height="258" width="480" src="http://localhost/drupal2/sites/default/files/styles/large/public/field/image/comments-view.png?itok=blSHCvU-" media="all and (min-width: 851px)"></source>
        <noscript>&lt;img class="image-style-large" typeof="foaf:Image" src="http://localhost/drupal2/sites/default/files/styles/large/public/field/image/comments-view.png?itok=blSHCvU-" width="480" height="258" alt="" /&gt;</noscript>
  <img width="220" height="118" alt="" src="http://localhost/drupal2/sites/default/files/styles/medium/public/field/image/comments-view.png?itok=usN60KC6"></picture>

7.
I thought the tests might be failing because the alt tag was not part of <picture> anymore.
(See http://picture.responsiveimages.org/ picture)
and that we needed to update the tests to not expect alt in <picture>
but that html code shows that we are still outputting <picture alt="">

8.
the tests have a situation where alt is unset.
a.
But in the ui, I dont see how we can unset it. alt is a field, and so even if that is not filled out, it would be an empty string?
b.
unsetting, that would have to be programmatically, right?

9.
Maybe if we found where render() was controlling the output, we could make it match http://picture.responsiveimages.org/ and fit the tests to that?
but I guess that might be a separate issue, since this is supposed to just be a conversion.

10.
the tests assume that if alt is unset that alt="" should be in the source, but if alt is null, that alt="" should not be printed. this is confusing for me.
I suppose it's supposed to read as: if alt is not set the default is alt="", if alt is set, but null, then it is intended not to be output at all.

this get's into tricky versions of == vs === and empty string vs null vs not set.

11.
I want to see what $rendered_element is actually in the test. what is the best way to do that?
xdebug, and set a breakpoint?

[edit:]
also tried:

file_put_contents('/tmp/debug', print_r($rendered_element), FILE_APPEND);
//file_put_contents('/tmp/debug', "hello", FILE_APPEND);

but printing the redered_element gave an error:

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: /drupal2/batch?id=8&op=do_nojs&op=do
StatusText: OK
ResponseText: {"status":true,"percentage":"100","message":"","label":"Processed test 1 of 1 - \u003Cem class=\u0022placeholder\u0022\u003EPicture theme functions\u003C\/em\u003E.\u003Cdiv class=\u0022simpletest-fail\u0022\u003EOverall results: 2 passes, 6 fails, 0 exceptions\u003C\/div\u003E\u003Cdiv class=\u0022item-list\u0022\u003E\u003Cul\u003E\u003Cli\u003E\u003Cdiv class=\u0022simpletest-fail\u0022\u003EPicture theme functions: 2 passes, 6 fails, 0 exceptions\u003C\/div\u003E\u003C\/li\u003E\u003C\/ul\u003E\u003C\/div\u003E"}

Stopping on this for now.

RainbowArray’s picture

So I hate to rain on the parade of all the work that has been done on this issue, but looks as if part of this issue is about outputting an alt attribute on the picture element, but there shouldn't be an alt attribute on the picture element. The alt attribute goes on the img element inside of the picture element.

The specification for the picture element has changed in the last couple months, so it's possible alt was on picture in the past, but that's no longer the case.

Here's the current spec: http://picture.responsiveimages.org

jayeshanandani’s picture

FileSize
21.75 KB
6.54 KB

Remove unused variables and changed functions.

jayeshanandani’s picture

Status: Needs work » Needs review

Remove unused variables and changed functions.

Status: Needs review » Needs work

The last submitted patch, 152: 1898442.152.patch, failed testing.

The last submitted patch, 152: 1898442.152.patch, failed testing.

jayeshanandani’s picture

Status: Needs work » Needs review
FileSize
6.9 KB
21.68 KB

End line issue resolved.

Status: Needs review » Needs work

The last submitted patch, 156: 1898442.154.patch, failed testing.

YesCT’s picture

Issue summary: View changes
Status: Needs work » Postponed

I think that interdiff was a diff of two patches? and not a git diff (looks weird in dreditor)

also seems to be more than just fixing the newlines at the end of files.

Fixes #150.3 also?

and,

< +    );
---
> +    );$expected_result = '<img class="image-style-test" src="' . $url . '" alt=""/>';

huh? is that a paste mistake?

and there are some other chunks changed... more than just newline fixes.

I think we can leave this and focus on #2211831: Removal of alt attribute from [picture] tag first. Postponed on that.
When we come back here, we will probably need to redo this issue, but it will be simpler.

attiks’s picture

Issue tags: +Needs reroll

FYI: picture is renamed to responsive_image so this needs a reroll once #2124377-74: Rename "Picture" module to "Responsive Image" module is committed

RainbowArray’s picture

Status: Postponed » Needs work

#2124377: Rename "Picture" module to "Responsive Image" module is in, and so is #2211831: Removal of alt attribute from [picture] tag, so this can be rerolled and simplified, since the alt attribute tests aren't needed any more.

jayeshanandani’s picture

Assigned: Unassigned » jayeshanandani
jayeshanandani’s picture

Lot of things have been changed since the patch was rerolled. Looking into details for same.
#2009662: [REGRESSION] Replace theme() with drupal_render() in picture module has also got some relation to this. Looking at both issues and checking what all functions are needed to be changed.

jayeshanandani’s picture

Status: Needs work » Needs review
FileSize
22.19 KB
6.83 KB

Rerolled #152 since many functionalities got change after that (#162)

Status: Needs review » Needs work

The last submitted patch, 163: 1898442.163.patch, failed testing.

jayeshanandani’s picture

Status: Needs work » Needs review
FileSize
22.18 KB
12.14 KB

Rerolled #148 cleanly. Straight reroll without any modifications.

RainbowArray’s picture

Issue tags: -Needs reroll
FileSize
2.35 KB
22.14 KB

I've been working with jayeshanandani, who's doing a great job on this issue. Walked through the reroll and found a couple other tweaks that were necessary. Then we'll start in on fixing the issues since #148.

The last submitted patch, 165: 1898442.164.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 166: convert-picture-twig-1898442-166.patch, failed testing.

jayeshanandani’s picture

Status: Needs work » Needs review
FileSize
22.17 KB
1.65 KB

I've been working with mdrummond on this issue. We made a clean reroll and then looked out for issues in the same. This patch modifies some function fixes that were changed. There are some issues still left due to which image is not being rendered with picture and source tags. Working on the issue to get that fixed, in meantime uploading a minor fix.

Status: Needs review » Needs work

The last submitted patch, 169: 1898442.167.patch, failed testing.

jayeshanandani’s picture

Status: Needs work » Needs review
FileSize
22.18 KB
857 bytes

Made changes to #169 patch. Now image renders correclty with picture and source tag.
Remaining Tasks:

1. Get alt alttribute out from picture tag.
2. Rewrite tests for new functions.

star-szr’s picture

Title: picture.module - Convert theme_ functions to Twig » responsive_image.module - Convert theme_ functions to Twig

I mentioned in IRC that it might make sense to rename picture.html.twig to responsive-image.html.twig, that was the main confusion here I think.

Thanks @jayeshanandani and @mdrummond for your hard work here!

webchick’s picture

Yeah, is there a reason we don't do that here? Because:

a) the patch as-is leaves us in the confusing world where there's both #theme => image and #theme => picture. The idea of renaming the module was to avoid that confusion.
b) "responsive-image.twig.html" would de-couple the concept "responsive images" from the specific implementation <picture> which may or may not change over the course of HTML5+. (It is admittedly a bit of a mouhtful, though)

RainbowArray’s picture

+1 to renaming picture.html.twig to responsive-image.html.twig.

We name templates after their purpose, not a specific HTML element. Makes sense.

RainbowArray’s picture

Status: Needs review » Active

We need to get this fixed before we can continue with the conversation: #2220865: Add empty img element inside picture element.

jayeshanandani’s picture

Issue summary: View changes
star-szr’s picture

Status: Active » Postponed
star-szr’s picture

Status: Postponed » Needs work

Actually we can still move forward a fair bit on this instead of twiddling our thumbs until #2220865: Add empty img element inside picture element gets in. @jayeshanandani is working on this during core mentoring.

Eli-T’s picture

Component: picture.module » responsive_image.module
webchick’s picture

Then another naïve question, which probably has nothing to do with this issue but just in case...

Why do you sometimes but not always want responsive images? Wouldn't we want all images to be responsive..?

jayeshanandani’s picture

Status: Needs work » Needs review
FileSize
19.89 KB

Removed alt from picture tag and wrote new tests for twig conversion. Renamed the picture.html.twig to responsive-image.html.twig and other minor changes.

jayeshanandani’s picture

FileSize
8.38 KB

Diff between #171 and #181.

Status: Needs review » Needs work

The last submitted patch, 181: 1898442.181.patch, failed testing.

jayeshanandani’s picture

Discussing this issue with @mdrummond and @YesCT in irc:
While going through past comments and reviewing patches we found that theme_picture, theme_picture_formatter and theme_picture_source got merged into template_preprocess_responsive_image() on #99. Later on #120 we found the tests coverage for theme_picture, theme_picture_formatter but not for theme_picture_source. Now since all 3 functions theme_picture, theme_picture_formatter and theme_picture_source were merged into one function ie: template_preprocess_responsive_image, we got confused on why was #120 adding tests for the functions that were being removed. We tried to understand and then there was a suggestion to update the tests made in #120 make work for template_preprocess_responsive_image. Modification of tests shall include tests coverage from #120 along with some modifications.Some of the things that needs to be changed in tests:

1. #120 Tests checks only for <picture>but not </picture>. We need to make sure that we get both picture tags. Also there should be an <img> tag inside <noscript> as well as outside of that tag. The tests written in #120 doesnt covers that.

2. We need to check the above tests for value of alt when set and unset at both times.

3. We need to include tests without breakpoints,alt and title attribute and check for same.

jayeshanandani’s picture

FileSize
22.29 KB
4.87 KB

New patch adds missing tests for picture element. It covers all tests as discusses in #184.
The patch will fail the tests as there is no <a> tag being generated by template_preprocess_responsive_image. The old code used to generate <a> on image elements.
Patch also removes title attribute from <picture> tag.

jayeshanandani’s picture

Status: Needs work » Needs review
jayeshanandani’s picture

FileSize
22.26 KB
1.67 KB

Minor changes to ResponsiveImageThemetest file.

The last submitted patch, 185: 1898442.185.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 187: 1898442.187.patch, failed testing.

jayeshanandani’s picture

Status: Needs work » Postponed

Postponing the issue as #2123251: Improve DX of responsive images; convert theme functions to new #type element is a major issue. Once that gets in , this will need a reroll of patch along with some function change.

jayeshanandani’s picture

Issue summary: View changes
Wim Leers’s picture

Status: Postponed » Closed (duplicate)

#2260061: Responsive image module does not support sizes/picture polyfill 2.2 got committed (YAY!), hence this is now unblocked :) But actually, that patch already performed the conversion. So marking this as a duplicate.

star-szr’s picture

Assigned: jayeshanandani » Unassigned
Issue summary: View changes
Status: Closed (duplicate) » Postponed

Thanks Wim! :)

However it looks like theme_responsive_image_formatter() still exists, that should likely be converted still. Leaving postponed on #2123251: Improve DX of responsive images; convert theme functions to new #type element for now but I don't necessarily agree that this is blocked on that issue, at least not at this time.

Gave the issue summary a bit of an update.

attiks’s picture

I had a quick look at the code, it is similar on how image does it, so why remove it in one place and not the other?

joelpittet’s picture

@attiks what "other" are you referring to?

attiks’s picture

#195 theme_image_formatter

star-szr’s picture

That doesn't exist, unless you are proposing a consolidation with image-formatter.html.twig?

attiks’s picture

#197 You're right, I checked using ag theme_image_formatter, but the only results are inside comments.

So we need to convert theme_responsive_image_formatter to template_preprocess_responsive_image_formatter

star-szr’s picture

Status: Postponed » Active
Issue tags: -RTBC July 1

#2123251: Improve DX of responsive images; convert theme functions to new #type element hasn't really moved so re-activating this to convert theme_responsive_image_formatter().

attiks’s picture

Status: Active » Closed (duplicate)

Closing this so it can fixed in the same commit of #2123251: Improve DX of responsive images; convert theme functions to new #type element, thanks all

star-szr’s picture

Status: Closed (duplicate) » Postponed

Thanks @attiks, it's great to see momentum there! Don't want to be a pessimist but postponing until that is actually committed because otherwise things can get lost.

attiks’s picture

@Cottser good thinking ;-)

RainbowArray’s picture

Status: Postponed » Closed (duplicate)

Since #2123251: Improve DX of responsive images; convert theme functions to new #type element is in, this can officially be closed and joelpittet can finally check this conversion off his list!

star-szr’s picture

Woohoo! Thank you @mdrummond :)