Task

Use Twig instead of PHPTemplate

Remaining

  • Patch needs review
  • Manual testing

Steps to test

Testing the Image Style admin UI

  1. Add a new image style. Go to Configuration->Image Styles, click on the 'Add image style' button, and enter an image style name (let's say 'mytest'). Click on the 'Create new style' button.
  2. You will be shown a 'Edit style mytest' form, with the preview of the style result (template: image-style-preview.html.twig). 'Original' and 'mytest' images should be the same in appearance and dimensions (800x600). The effect list table at the bottom should be empty.
  3. Add a 'Crop' effect. The resulting form should have 'Width', 'Height' and 'Anchor' elements. The Anchor element is a 3x3 table of radio buttons (template: image-anchor.html.twig). Enter Width 750, Height 550 and leave the Anchor to default center/center. Click 'Add effect'
  4. The 'Edit style mytest' preview reflects the change in size of the image (template: image-style-preview.html.twig). The effect list table at the bottom lists the effect with summary "Crop 750x550" (template: image-crop-summary.html.twig).
  5. Add a 'Desaturate' effect.
  6. The 'Edit style mytest' preview reflects the change of the image to greyscale (template: image-style-preview.html.twig). The effect list table at the bottom lists the effect with summary "Desaturate" (no template here).
  7. Add a 'Resize' effect. The resulting form should have 'Width' and 'Height' elements. Enter Width 700, Height 500. Click 'Add effect'
  8. The 'Edit style mytest' preview reflects the change in size of the image (template: image-style-preview.html.twig). The effect list table at the bottom lists the effect with summary "Resize 700x500" (template: image-resize-summary.html.twig).
  9. Add a 'Rotate' effect. Enter Rotation Angle 45. Click 'Add effect'
  10. The 'Edit style mytest' preview reflects the change in size of the image (849x849) (template: image-style-preview.html.twig). The effect list table at the bottom lists the effect with summary "Rotate 45°" (template: image-rotate-summary.html.twig).
  11. Add a 'Scale' effect. Enter Width 1000, tick on 'Allow upscaling'. Click 'Add effect'
  12. The 'Edit style mytest' preview reflects the change in size of the image (1000x1000) (template: image-scale-summary.html.twig). The effect list table at the bottom lists the effect with summary "Scale width 1000 (upscaling allowed)".
  13. Add a 'Scale and crop' effect. Enter Width 600, Height 400. Click 'Add effect'
  14. The 'Edit style mytest' preview reflects the change in size of the image (600x400) (template: image-style-preview.html.twig). The effect list table at the bottom lists the effect with summary "Scale and crop 600x400" (template: image-resize-summary.html.twig).

Note: at each change to effects, the preview should always be refreshed with the latest setup of effects - i.e. you should not need to clear browser's cache to get the latest images.

Testing the Field widget

  1. Change the preview image style of the uploaded images for the Article content type. Go to Structure->Content Types and click on 'Manage form display' from Article's operations dropbutton.
  2. Click on the gearbox icon right to the 'Image' field.
  3. From the appearing form, select 'mytest' (set in the test above) from the 'Preview image style' dropdown.
  4. Click the 'Update' button, then on the 'Save' button.
  5. Add a new node of type 'Article', uploading an image of your choice.
  6. The image preview on the node is formatted according to 'mytest' image style (templates: image-widget.html.twig and image-style.html.twig).
  7. Save the new Article. The original image is displayed with the 'Large (480x480)' image style.

Testing the Field formatter

  1. Change the display image style of the uploaded images for the Article content type. Go to Structure->Content Types and click on 'Manage display' Article's operations dropbutton.
  2. Click on the gearbox icon right to the 'Image' field.
  3. From the appearing form, select 'mytest' (set in the test above) from the 'Image style' dropdown.
  4. Click the 'Update' button, then on the 'Save' button.
  5. View the article you created in the previous steps.
  6. The image on the node is formatted according to 'mytest' image style. It is not clickable. (templates: image-formatter.html.twig and image-style.html.twig)
  7. Link the displayed image to its file. Go to Structure->Content Types and click on 'Manage display' Article's operations dropbutton.
  8. Click on the gearbox icon right to the 'Image' field.
  9. From the appearing form, select 'File' from the 'Link image to' dropdown.
  10. Click the 'Update' button, then on the 'Save' button.
  11. View the article you created in the previous steps.
  12. The image on the node is formatted according to 'mytest' image style. (templates: image-formatter.html.twig and image-style.html.twig). Clicking on the image, the ORIGINAL uploaded image is displayed in the browser.

#1757550: [Meta] Convert core theme functions to Twig templates
#1938910: Convert image theme tables to table #type

Review Bonus

See #2094585: [policy, no patch] Core review bonus.

- https://drupal.org/node/828268#comment-7897745 +2
- https://drupal.org/node/854956#comment-7897855 +2
- tagged #113 -3

Points: 1

CommentFileSizeAuthor
#242 1898420-twig-image-242.patch44.37 KBmondrake
#226 1898420-twig-image-226.patch44.26 KBmondrake
#219 interdiff_217-219.txt811 bytesmondrake
#219 1898420-twig-image-219.patch44.71 KBmondrake
#217 interdiff_212-217.txt857 bytesmondrake
#217 1898420-twig-image-217.patch44.76 KBmondrake
#212 1898420-twig-image-212.patch44.76 KBmondrake
#212 interdiff_207-212.txt2.35 KBmondrake
#207 1898420-twig-image-207.patch44.76 KBmondrake
#207 interdiff_203-207.txt659 bytesmondrake
#203 1898420-twig-image-203.patch44.69 KBmondrake
#203 interdiff_200-203.txt479 bytesmondrake
#200 diff-diff_193-200.txt211 bytesmondrake
#200 interdiff_198-200.txt860 bytesmondrake
#200 1898420-twig-image-200.patch44.62 KBmondrake
#198 interdiff_197-198.txt710 bytesmondrake
#198 1898420-twig-image-198.patch44.63 KBmondrake
#197 1898420-twig-image-196.patch44.68 KBmondrake
#196 interdiff_193-196.txt461 bytesmondrake
#195 1898420-195-duplicate-widget.png255.23 KBstar-szr
#193 1898420-twig-image-193.patch44.6 KBmondrake
#189 1898420-twig-image-189.patch44.28 KBmondrake
#189 interdiff_178-189.txt2.8 KBmondrake
#178 interdiff_177-178.txt771 bytesmondrake
#178 1898420-twig-image-178.patch44.32 KBmondrake
#177 interdiff_173-177.txt4.25 KBmondrake
#177 1898420-twig-image-177.patch44.22 KBmondrake
#173 interdiff_170-173.txt4.09 KBmondrake
#173 1898420-twig-image-173.patch45.01 KBmondrake
#170 interdiff_166-170.txt15.99 KBmondrake
#170 1898420-twig-image-170.patch44.89 KBmondrake
#166 interdiff_165-166.txt1.06 KBmondrake
#166 1898420-twig-image-166.patch45.92 KBmondrake
#165 interdiff_159-165.txt532 bytesmondrake
#165 1898420-twig-image-165.patch45.89 KBmondrake
#159 1898420-twig-image-159.patch45.89 KBmondrake
#159 interdiff_157-159.txt9.34 KBmondrake
#157 1898420-twig-image-157.patch42.1 KBmondrake
#156 1898420-twig-image-156.patch41.98 KBmondrake
#143 1898420-twig-image-143.patch41.98 KBmondrake
#143 interdiff_138-143.txt10.02 KBmondrake
#138 1898420-twig-image-138.patch40.99 KBmondrake
#138 interdiff_131-138.txt2.19 KBmondrake
#131 1898420-twig-image-131-reroll.patch39.73 KBjoelpittet
#129 image-twig-1898420-129.patch47.71 KBInternetDevels
#110 twig-1898420-110.patch45.66 KBclaudiu.cristea
#105 twig-1898420-105.patch45.71 KBclaudiu.cristea
#105 interdiff.txt4.65 KBclaudiu.cristea
#100 twig-1898420-100.patch45.54 KBclaudiu.cristea
#100 interdiff.txt10.46 KBclaudiu.cristea
#98 twig-1898420-98.patch45.18 KBclaudiu.cristea
#98 interdiff.txt27.74 KBclaudiu.cristea
#93 test-plan.png470.12 KBclaudiu.cristea
#93 8.x.png277.74 KBclaudiu.cristea
#93 twig-conversion.png278.46 KBclaudiu.cristea
#89 twig-1898420-89.patch36.17 KBclaudiu.cristea
#89 interdiff.txt10.18 KBclaudiu.cristea
#88 twig-1898420-88.patch34.56 KBclaudiu.cristea
#88 interdiff_to_84.txt5.39 KBclaudiu.cristea
#84 drupal-1898420-84.patch31.16 KBc4rl
#84 drupal-1898420-84.patch-interdiff.txt1.63 KBc4rl
#83 drupal-1898420-83.patch31.03 KBc4rl
#69 twig-n1898420-69.patch31.27 KBDamienMcKenna
#68 twig-n1898420-68.patch23.65 KBDamienMcKenna
#63 2013-05-21 11.55.54 pm.png35.43 KBintergalactic overlords
#63 2013-05-21 11.56.08 pm.png38.11 KBintergalactic overlords
#63 2013-05-21 11.56.22 pm.png36.52 KBintergalactic overlords
#59 twig-image-1898420-59.patch32.89 KBshanethehat
#59 interdiff.txt1.15 KBshanethehat
#57 twig-image-1898420-57.patch32.79 KBAlbert Volkman
#52 interdiff.txt8.36 KBshanethehat
#52 twig-image-1898420-51.patch32.73 KBshanethehat
#49 interdiff.txt12.08 KBshanethehat
#49 twig-image-1898420-49.patch36.06 KBshanethehat
#44 interdiff.txt11.38 KBshanethehat
#44 twig-image-1898420-44.patch35.14 KBshanethehat
#40 interdiff.txt8.8 KBshanethehat
#40 twig-image-1898420-40.patch35.94 KBshanethehat
#33 interdiff.txt13.62 KBshanethehat
#33 twig-image-1898420-33.patch31.19 KBshanethehat
#31 interdiff.txt533 bytesshanethehat
#31 twig-image-1898420-31.patch24.52 KBshanethehat
#23 twig-image-1898420-23.patch24.35 KBshanethehat
#23 interdiff.txt5.23 KBshanethehat
#18 interdiff.txt18.21 KBshanethehat
#18 twig-image-1898420-18.patch23.51 KBshanethehat
#4 1898420-twig-for-images-4.patch8.11 KBthedavidmeister
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

c4rl’s picture

Issue tags: +Twig

Tagging

thedavidmeister’s picture

Assigned: Unassigned » thedavidmeister

I'd like to look into this myself unless there are any objections?

thedavidmeister’s picture

thedavidmeister’s picture

Status: Active » Needs work
FileSize
8.11 KB

work-in-progress patch. Tests are currently failing what I've done because they're looking for the active class on image links, see issue linked in #3.

star-szr’s picture

Status: Needs work » Needs review

Sending to testbot so we can all see.

Status: Needs review » Needs work

The last submitted patch, 1898420-twig-for-images-4.patch, failed testing.

thedavidmeister’s picture

Lol, now the whole world knows my patch failed :P

Although, applying the patch locally and doing some verbose output on what the test is expecting vs. what appears makes it way more obvious that it's the "active" class throwing the test off - There's no a lot of "why" in the d.o. test reports, just a couple of red lines.

star-szr’s picture

@thedavidmeister - To skip testbot altogether, you can end your patch filename with -do-not-test, see http://groups.drupal.org/node/213473.

thedavidmeister’s picture

thanks for the tip. It's fine though, probably best that others can see which tests are failing rather than just taking my word for it :)

star-szr’s picture

I was about to post here that image-scale-summary.html.twig in the sandbox has a misaligned docblock (like #1898432-22: node.module - Convert PHPTemplate templates to Twig), but I don't see that template in the patch, it looks like a lot of the Twig templates found in the sandbox at /core/themes/stark/templates/image are missing from the patch, that or I'm missing something :)

thedavidmeister’s picture

yeah yeah, they're missing. I'm still working on it. I got distracted/deep into the issues around l() - I don't want to start a new template til i get the current ones green, even if i have to do something janky as a stop-gap, but I didn't want to not roll a patch with my work so far in case my laptop dies in the meantime.

Been super busy at work this week, will be back into this on the weekend. Thanks for the heads up on the docblock :)

thedavidmeister’s picture

Status: Needs work » Postponed

This is currently blocked on #1812562: Create best practices about use of the l() function and the url() function in templates and #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig as some of the templates for the Image module require clear direction on how links should be handled in templates/preprocess functions.

star-szr’s picture

Assigned: thedavidmeister » Unassigned
Status: Postponed » Needs work

There's a fair bit left to be converted here, I think we need to convert what we can while we wait for #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig and #1812562: Create best practices about use of the l() function and the url() function in templates.

@thedavidmeister, thanks for your hard work here :) I'm unassigning but if you think you'll have time to work on this in the near future, please comment and reassign.

thedavidmeister’s picture

I can probably work on this next weekend. When I postponed this issue it was *much* less clear how the l() thing was going to pan out - there were like 3 or 4 different solutions that might have gone through. It's probably safe to work on this in parallel to the other issue now.

If anyone wants to look at this between now and when I'm next playing with it, go nuts :)

star-szr’s picture

Issue summary: View changes

Add conversion summary table

star-szr’s picture

Issue tags: +Needs manual testing

Tagging.

star-szr’s picture

Issue tags: +Novice

Tagging as a Novice task to bring over the conversions listed in the summary from the sandbox.

I recommend taking a look at @c4rl's screencast: http://www.youtube.com/watch?v=HS4yKJjrb2E

If you'd like to work on this, please assign the issue to yourself. If you get stuck, drop by #drupal-twig on IRC or post your questions here. Thanks!

shanethehat’s picture

Assigned: Unassigned » shanethehat
shanethehat’s picture

Assigned: shanethehat » Unassigned
Status: Needs work » Needs review
FileSize
23.51 KB
18.21 KB

Brought over the conversions listed in the summary

Status: Needs review » Needs work
Issue tags: -Novice, -Needs manual testing, -Twig

The last submitted patch, twig-image-1898420-18.patch, failed testing.

shanethehat’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Needs manual testing, +Twig

#4: 1898420-twig-for-images-4.patch queued for re-testing.

thedavidmeister’s picture

Status: Needs review » Needs work

@shanethehat - thanks for this. Aside from making the testbots happy, check your Template documentation for references to PHP data types like "associative array" - these are not supposed to be there [#1823416].

shanethehat’s picture

Assigned: Unassigned » shanethehat
shanethehat’s picture

Status: Needs work » Needs review
FileSize
5.23 KB
24.35 KB

Hopefully this will appease the testbots

shanethehat’s picture

Assigned: shanethehat » Unassigned

Status: Needs review » Needs work

The last submitted patch, twig-image-1898420-23.patch, failed testing.

shanethehat’s picture

Assigned: Unassigned » shanethehat
thedavidmeister’s picture

@shanthehat - It looks like most of the fails are being caused by those exceptions. Do you know how to run these tests locally while you're working on a patch?

shanethehat’s picture

@thedavidmeister - yeah. I had it working locally, so I reckon I screwed up the patch somehow. At work at the moment but I'll sort it later today.

thedavidmeister’s picture

#28 - Maybe your error reporting is not sensitive enough for the issues to appear on your local or something...

star-szr’s picture

Issue tags: -Novice

Removing novice tag now that the conversions are over. @shanethehat - anything we can do to help?

shanethehat’s picture

Status: Needs work » Needs review
FileSize
24.52 KB
533 bytes

@Cottser: sorry this is taking so long, I think I'm getting the hang of it but not all the tests are passing yet. Here's what I have so far, I'd missed an obvious use statement but had trouble getting the exception out of it.

I'd like to carry on with it, but if you're in a hurry I don't mind someone taking it over as I'm about to go to bed and won't be looking at it again until the morning.

Status: Needs review » Needs work

The last submitted patch, twig-image-1898420-31.patch, failed testing.

shanethehat’s picture

Status: Needs work » Needs review
FileSize
31.19 KB
13.62 KB

Down to 5 fails locally, let's see what the testbots say.

Status: Needs review » Needs work

The last submitted patch, twig-image-1898420-33.patch, failed testing.

star-szr’s picture

Wow. Keep on rocking!

Edit: 15 is still much better than 250+ :D and no exceptions!

shanethehat’s picture

The tests that have failed on an HTML comparision will be rewritten to do an xpath comparison of the HTML element instead. This will prevent failures for the order of the attributes, and (hopefully) for comments added by twig debug mode.

shanethehat’s picture

The conversion of theme_image_style seems to have created an additional class of 'image-style' on all the image elements it generates. Nine of the tests are failing because they do not expect the extra class. The classes that the tests do expect are all present. Should I:

  1. Figure out where the extra class is coming from and remove it?
  2. Change the tests to expect the extra class?
  3. Change the tests to ignore the extra class?
  4. Something else
thedavidmeister’s picture

#37 - I vote 2 since it could be useful to have the extra class, unless it's 100% redundant with another class on the image, in which case I vote 1.

That said, I'm not sure that we're allowed to change any markup other than whitespace in these conversion patches so we might just have to do 1 regardless of what I think.

star-szr’s picture

shanethehat’s picture

Status: Needs work » Needs review
FileSize
35.94 KB
8.8 KB

I've used the example from #1939068: Convert theme_image() to Twig to remove the undesirable class, but the same tests are still failing because the order of the attributes is different. I don't think that's something we should be concerned with, as long as all the expected attributes are present and nothing unexpected has been added. I mentioned this on IRC yesterday and joelpittet suggested doing something with xpath instead of just comparing strings.

This patch changes ImageDimensionsTest to use xpath instead of a string comparison. It will still fail if the element is not present, does not contain the expected attributes, or contains unexpected attributes. If you guys think this is okay, I'll move the functionality into a superclass so that it can be used elsewhere too.

star-szr’s picture

@shanethehat, that looks pretty nice but I fear it might be out of scope for this issue - can we just use normal XPaths for now? See #1959660: Replace xpath() with WebTestBase::cssSelect() by leveraging Symfony CssSelector.

Edit: We could probably keep the methods on this test class for now because I see it makes sure there are no unexpected attributes. Maybe it would be best to create a new issue to propose the 'super class' idea; it's probably okay to add these methods to this test class for now.

shanethehat’s picture

@Cottser - I thought that WebTestBase::xpath() was for running path expressions against the contents of the internal browser? The test failures that I have left are comparing one string against another without using the internal browser.

If you're happy with this technique then I can duplicate the code across the existing tests, and we can open a new issue to consolidate the duplication into a superclass later?

Status: Needs review » Needs work

The last submitted patch, twig-image-1898420-40.patch, failed testing.

shanethehat’s picture

Status: Needs work » Needs review
FileSize
35.14 KB
11.38 KB

After dicussion with @Cottser in IRC I've removed the xpath stuff from the tests and just amended the test cases to expect the attributes in a slightly different order.

star-szr’s picture

Status: Needs review » Needs work

This is looking great, thank you for sticking with this @shanethehat! Here is my review of the latest patch, a few implementation questions and some very nitpicky documentation tweaks ahead :)

  1. +++ b/core/modules/image/image.admin.incundefined
    @@ -764,73 +767,94 @@ function theme_image_anchor($variables) {
    +  $output = '';
       $data = $variables['data'];
     
       if ($data['width'] && $data['height']) {
    -    return check_plain($data['width']) . 'x' . check_plain($data['height']);
    +    $output = check_plain($data['width']) . 'x' . check_plain($data['height']);
       }
       else {
    -    return ($data['width']) ? t('width @width', array('@width' => $data['width'])) : t('height @height', array('@height' => $data['height']));
    +    $output = ($data['width']) ? t('width @width', array('@width' => $data['width'])) : t('height @height', array('@height' => $data['height']));
       }
    +
    +  if(isset($data['upscale'])) {
    +    $output .= ' (' . t('upscaling allowed') . ')';
    +  }
    +
    +  $variables['summary'] = $output;
    

    Would it make any sense to move the display logic for the summary theme output to the template? Just thinking out loud, maybe not.

    Edit: Let's discuss this one first :)

  2. +++ b/core/modules/image/image.admin.incundefined
    @@ -764,73 +767,94 @@ function theme_image_anchor($variables) {
    + * Default template: image-scale-summary.html.twig
    ...
    + * Default template: image-crop-summary.html.twig
    
    +++ b/core/modules/image/image.field.incundefined
    @@ -417,72 +418,82 @@ function _image_field_required_fields_validate($element, &$form_state) {
    + *   - item: the formatter item
    
    +++ b/core/modules/image/image.moduleundefined
    @@ -967,26 +982,21 @@ function image_effect_apply($image, $effect) {
    + *   - width: the width of the image
    + *   - height: the height of the image
    + *   - style_name: the name of the image style to be applied
    + *   - attributes: additional attributes to apply to the image
    + *   - uri: URI of the source image before styling
    + *   - alt: alternative text to be displayed instead of the image
    + *   - title: title of the image
    

    These comments need to end with a period. :)

    The last bunch should also start with a capital letter.

    Ref: http://drupal.org/node/1354#drupal.

  3. +++ b/core/modules/image/image.admin.incundefined
    @@ -764,73 +767,94 @@ function theme_image_anchor($variables) {
       $data = $variables['data'];
    -  return theme('image_resize_summary', array('data' => $data)) . ' ' . ($data['upscale'] ? '(' . t('upscaling allowed') . ')' : '');
    +  $variables['summary'] = array(
    +    '#theme' => 'image_resize_summary',
    +    '#data' => $data,
    +  );
    

    I think since we're only using $data once, let's just access $variables['data'] directly, no need to create a variable. Just do what template_preprocess_image_crop_summary() does.

  4. +++ b/core/modules/image/image.field.incundefined
    @@ -417,72 +418,82 @@ function _image_field_required_fields_validate($element, &$form_state) {
    - *   - element: A render element representing the image field widget.
    - *
    - * @ingroup themeable
    + *   - element: An associative array containing the image field widget.
    

    Let's leave this as 'A render element…' please, because that has more connotation than 'associative array' in this case.

  5. +++ b/core/modules/image/image.field.incundefined
    @@ -417,72 +418,82 @@ function _image_field_required_fields_validate($element, &$form_state) {
    +    $variables['preview'] = drupal_render($element['preview']);
    

    Did you try removing this drupal_render() or did it break things?

  6. +++ b/core/modules/image/templates/image-formatter.html.twigundefined
    @@ -0,0 +1,24 @@
    + * - url: An optional url the image can be linked to.
    

    url in the description should be URL; the variable should be kept lowercase.

  7. +++ b/core/modules/image/templates/image-style-preview.html.twigundefined
    @@ -0,0 +1,54 @@
    + * - original_height: The lesser of original_image_height or sample image
    + *     height.
    

    The second line on this list item shouldn't be indented by two spaces, the indent should match the beginning of the list text per http://drupal.org/node/1354#lists.

  8. +++ b/core/modules/image/templates/image-style-preview.html.twigundefined
    @@ -0,0 +1,54 @@
    + * - preview_image: HThe rendered preview image..
    

    Remove extra 'H' and remove extra period at the end.

  9. +++ b/core/modules/image/templates/image-style.html.twigundefined
    @@ -0,0 +1,31 @@
    + * - attributes: A collection of image attributes, including the following:
    + *   - attributes.src: Full URL or relative path to the image file.
    + *   - attributes.class: One or more classes to be applied to the image.
    + *   - attributes.width: The width of the image (if known).
    + *   - attributes.height: The height of the image (if known).
    + *   - attributes.title: The title of the image.
    + *   - attributes.alt: The alternate text for the image. HTML 4 and Xhtml 1.0
    

    For the sub-list elements, remove the 'attributes.' - we'd like indent to mean 'add a dot'. Also, I think we can just say 'HTML attributes for the image, including the following:', rather than referring to it as a collection/array.

    And please capitalize XHTML.

  10. +++ b/core/modules/image/templates/image-widget.html.twigundefined
    @@ -0,0 +1,26 @@
    + * - attributes: A list of element attributes.
    

    Similarly here, can we just say something along the lines of 'HTML attributes for the containing element.'?

Keep rocking!

jenlampton’s picture

@Cottser, I'm against moving all the logic for width & height to the template. Most of the time people won't want to override that in a template file, it's just the string of text being dumped into it. If people want to override the data, they'll likely go to preprocess to assemble something new anyway. Let's leave it there and call it good :)

star-szr’s picture

@jenlampton: sounds good - just wanted to be sure we had considered it. Thanks!

thedavidmeister’s picture

+1 to keeping all logic for image attributes in the preprocess. I actually have a use-case right now for keeping at least height, width and src completely in the preprocess.

shanethehat’s picture

Status: Needs work » Needs review
FileSize
36.06 KB
12.08 KB

Hopefully fixes all but the rdf fail. Also addresses feedback 2-10 from #45.

Status: Needs review » Needs work

The last submitted patch, twig-image-1898420-49.patch, failed testing.

star-szr’s picture

So close! I haven't looked at the interdiff yet but keep up the great work :)

shanethehat’s picture

Status: Needs work » Needs review
FileSize
32.73 KB
8.36 KB

Reverting changes to tests to improve compatibility with #1939068: Convert theme_image() to Twig.

RDF test still expected to fail.

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

The last submitted patch, twig-image-1898420-51.patch, failed testing.

shanethehat’s picture

Status: Needs work » Needs review

#52: twig-image-1898420-51.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs manual testing, +Twig

The last submitted patch, twig-image-1898420-51.patch, failed testing.

tlattimore’s picture

This patch is no longer applying cleanly and requires a re-roll. Here are the failures I am getting when I attempt to apply it locally:

error: while searching for:
}

/**
 * Returns HTML for an image field widget.
 *
 * @param array $variables
 *   An associative array containing:
 *   - element: A render element representing the image field widget.
 *
 * @ingroup themeable
 */
function theme_image_widget($variables) {
  $element = $variables['element'];
  $output = '';
  $output .= '<div class="image-widget form-managed-file clearfix">';

  if (isset($element['preview'])) {
    $output .= '<div class="image-preview">';
    $output .= drupal_render($element['preview']);
    $output .= '</div>';
  }

  $output .= '<div class="image-widget-data">';
  if ($element['fid']['#value'] != 0) {
    $element['filename']['#markup'] .= ' <span class="file-size">(' . format_size($element['#file']->filesize) . ')</span> ';
  }
  $output .= drupal_render_children($element);
  $output .= '</div>';
  $output .= '</div>';

  return $output;
}

/**
 * Returns HTML for an image field formatter.
 *
 * @param array $variables
 *   An associative array containing:

error: patch failed: core/modules/image/image.field.inc:417
error: core/modules/image/image.field.inc: patch does not apply
Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
32.79 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, twig-image-1898420-57.patch, failed testing.

shanethehat’s picture

Assigned: shanethehat » Unassigned
Status: Needs work » Needs review
FileSize
1.15 KB
32.89 KB

Finally got to grips with the RDF fail. image_style needs to use theme_image so that the hook in the rdf module catches.

shanethehat’s picture

Issue summary: View changes

Update for theme_image_style_list

star-szr’s picture

Assigned: Unassigned » steveoliver

Assigning to @steveoliver for review. Happy to see this green!

star-szr’s picture

Issue summary: View changes

Updating conversion table and remaining

c4rl’s picture

Title: Convert image module to Twig » image.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.

star-szr’s picture

Assigned: steveoliver » Unassigned

This is lower priority since it's just dealing with theme_ functions so taking off @steveoliver's review list for now.

intergalactic overlords’s picture

Status: Needs review » Needs work
FileSize
36.52 KB
38.11 KB
35.43 KB

Most of the conversions are ok. There are some small issues though:

- The images inside the preview of the image style have their src set to the base path instead of to the image.
- Although the image widget (a.k.a. the image input field) has the right code, the alt-text field and the image have switched places.

For both issues, see screenshots below.

artis’s picture

Status: Needs work » Needs review

#59: twig-image-1898420-59.patch queued for re-testing.

DamienMcKenna’s picture

Status: Needs review » Needs work

I've confirmed the problem with template_preprocess_image_style_preview() so am bumping this back to Needs Work.

DamienMcKenna’s picture

I can't work out which preprocess function or twig file is used for '#theme' => 'image', that appears to be the crux of the problem, any suggestions? I'm on IRC if anyone could give me some direction? Thanks :)

DamienMcKenna’s picture

ICBW but I believe this needs updating now that #1938430: Don't add a default theme hook class in template_preprocess() has been committed?

DamienMcKenna’s picture

FileSize
23.65 KB

Trying this out for size. I removed the piece in in template_preprocess_image_style() related to #1938430 as that has been committed, and added the '#uri' values to the $variables['original_image'] and $variables['preview_image'] variables in template_preprocess_image_style_preview() so that the images actually show.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
31.27 KB

Oops, the last patch forgot to include all of the new twig files.

DamienMcKenna’s picture

Just as a reminder, this dependent upon #1939068: Convert theme_image() to Twig.

ernie-g’s picture

profiling...

ernie-g’s picture

bbranches profile output:


=== 8.x..8.x compared (519fe99884ff9..519ff28c9b5d8):

ct  : 71,226|71,226|0|0.0%
wt  : 466,475|482,355|15,880|3.4%
cpu : 419,918|421,478|1,560|0.4%
mu  : 16,002,240|16,002,240|0|0.0%
pmu : 16,130,312|16,130,312|0|0.0%

<a href="http://drupal8.local/xhprof-kit/xhprof/xhprof_html/index.php?source=drupal-perf&url=%2F&run1=519fe99884ff9&run2=519ff28c9b5d8&extra=8.x..8.x">Profiler output</a>

=== 8.x..1898420 compared (519fe99884ff9..519ff309d02e6):

ct  : 71,226|71,226|0|0.0%
wt  : 466,475|517,336|50,861|10.9%
cpu : 419,918|424,094|4,176|1.0%
mu  : 16,002,240|16,001,960|-280|-0.0%
pmu : 16,130,312|16,130,528|216|0.0%

<a href="http://drupal8.local/xhprof-kit/xhprof/xhprof_html/index.php?source=drupal-perf&url=%2F&run1=519fe99884ff9&run2=519ff309d02e6&extra=8.x..1898420">Profiler output</a>

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage
The drush command 'rr' could not be found.  Run `drush cache-clear drush` to clear the commandfile cache if you have installed new extensions.    [error]
'all' cache was cleared in self                                                                                                                   [success]
ernie-g’s picture

MANY APOLOGIES
It turns out, the profiling instructions I received at the DrupalCon Code Sprint was incomplete.
I am going to be re-running all my profiling :-/

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

The last submitted patch, twig-n1898420-69.patch, failed testing.

ezeedub’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing, +Twig

#69: twig-n1898420-69.patch queued for re-testing.

ezeedub’s picture

Scenario #1: point site home to admin/config/media/image-styles/manage/medium/add/image_crop
hits core/modules/image/templates/image-anchor.html.twig

=== 8.x..8.x compared (51a462112dd99..51a4629fe3743):

ct  : 40,497|40,497|0|0.0%
wt  : 346,921|347,609|688|0.2%
cpu : 344,020|344,022|2|0.0%
mu  : 6,711,212|6,711,212|0|0.0%
pmu : 6,815,468|6,815,468|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a462112dd99&...

=== 8.x..image-1898420-69 compared (51a462112dd99..51a4630d23a4c):

ct  : 40,497|40,616|119|0.3%
wt  : 346,921|345,681|-1,240|-0.4%
cpu : 344,020|344,022|2|0.0%
mu  : 6,711,212|6,743,868|32,656|0.5%
pmu : 6,815,468|6,848,280|32,812|0.5%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a462112dd99&...

I checked out a branch to apply this patch, and then rebased the branch I created for #1939068: Convert theme_image() to Twig. I hope that's right. Before I test the rest of the twigplates in this patch, maybe someone could let me know if that's in fact the correct way to go, or does it make no difference?

Other twigplates in this patch...

core/modules/image/templates/image-crop-summary.html.twig
core/modules/image/templates/image-formatter.html.twig
core/modules/image/templates/image-resize-summary.html.twig
core/modules/image/templates/image-rotate-summary.html.twig
core/modules/image/templates/image-scale-summary.html.twig
core/modules/image/templates/image-style-preview.html.twig
core/modules/image/templates/image-style.html.twig
core/modules/image/templates/image-widget.html.twig
ezeedub’s picture

On second thought, rebasing the branch mentioned in #70 was unnecessary. Here's another run of the scenario in #76 without it.

=== 8.x..8.x compared (51a4f50036185..51a4ffaaeb078):

ct  : 40,497|40,497|0|0.0%
wt  : 353,504|349,672|-3,832|-1.1%
cpu : 348,022|340,022|-8,000|-2.3%
mu  : 6,711,212|6,711,212|0|0.0%
pmu : 6,815,468|6,815,468|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a4f50036185&...

=== 8.x..image-1898420-69 compared (51a4f50036185..51a5000df000f):

ct  : 40,497|40,616|119|0.3%
wt  : 353,504|348,911|-4,593|-1.3%
cpu : 348,022|344,022|-4,000|-1.1%
mu  : 6,711,212|6,743,868|32,656|0.5%
pmu : 6,815,468|6,848,280|32,812|0.5%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a4f50036185&...

ezeedub’s picture

Edit a style (i used medium) and add crop, resize and rotate effects, and then point home page to admin/config/media/image-styles/manage/medium

This hits the following twigplates:

  • image-style-preview.html.twig (empty?)
  • image-scale-summary.html.twig (wraps resize, no other output)
  • image-resize-summary.html.twig
  • image-crop-summary.html.twig (wraps resize, no other output)
  • image-rotate-summary.html.twig
=== 8.x..8.x compared (51a518f009231..51a524e76a9fb):

ct  : 51,355|51,355|0|0.0%
wt  : 433,387|432,749|-638|-0.1%
cpu : 432,027|428,026|-4,001|-0.9%
mu  : 7,274,048|7,274,048|0|0.0%
pmu : 7,391,328|7,391,328|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a518f009231&...

=== 8.x..image-1898420-69 compared (51a518f009231..51a5256dc4557):

ct  : 51,355|51,594|239|0.5%
wt  : 433,387|429,877|-3,510|-0.8%
cpu : 432,027|428,027|-4,000|-0.9%
mu  : 7,274,048|7,344,252|70,204|1.0%
pmu : 7,391,328|7,460,076|68,748|0.9%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a518f009231&...

I'm wondering if this patch needs work. image-style-preview, image-scale-summary and image-crop-summary seem to not have any output (although the last two do include the output of a nested twigplate).

ezeedub’s picture

scenarios remaining:

core/modules/image/templates/image-formatter.html.twig
core/modules/image/templates/image-style.html.twig
core/modules/image/templates/image-widget.html.twig
ezeedub’s picture

Generate 30 articles, edit frontpage view to show full node, no limit.
covers image-formatter and image-style

=== 8.x..8.x compared (51a5545d76a23..51a5621969453):

ct  : 119,579|119,579|0|0.0%
wt  : 973,749|972,147|-1,602|-0.2%
cpu : 964,060|964,060|0|0.0%
mu  : 9,616,596|9,616,596|0|0.0%
pmu : 9,732,336|9,732,336|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a5545d76a23&...

=== 8.x..image-1898420-69 compared (51a5545d76a23..51a562cdc3626):

ct  : 119,579|122,226|2,647|2.2%
wt  : 973,749|992,259|18,510|1.9%
cpu : 964,060|984,061|20,001|2.1%
mu  : 9,616,596|9,684,404|67,808|0.7%
pmu : 9,732,336|9,800,712|68,376|0.7%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a5545d76a23&...

ezeedub’s picture

Issue summary: View changes

Remove sandbox link

ezeedub’s picture

Not sure how to hit image-widget

jenlampton’s picture

image-widget is on a form, it's the upload widget that shows a image preview.
To test, create an article, upload a file, and save. Then grant permission for anonymous users to edit articles, and set the home page no node/1/edit (or update to whatever the nid of your node is).

c4rl’s picture

FileSize
31.03 KB

Rerolling

c4rl’s picture

Sorry, had some sloppy ternary operators.

Status: Needs review » Needs work

The last submitted patch, drupal-1898420-84.patch, failed testing.

claudiu.cristea’s picture

Assigned: Unassigned » claudiu.cristea

I'm taking this.

claudiu.cristea’s picture

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
5.39 KB
34.56 KB

Let's try again.

claudiu.cristea’s picture

FileSize
10.18 KB
36.17 KB

image_style_preview theme cleanup.

jenlampton’s picture

Issue tags: +needs profiling

tagging

claudiu.cristea’s picture

Profiling Twig patch docs: https://drupal.org/node/1999108

claudiu.cristea’s picture

My MAMP is on PHP 5.4.10. Last xhprof is 5.4.4 an doesn't work on my install. Is anybody willing to run profiling?

claudiu.cristea’s picture

FileSize
278.46 KB
277.74 KB
470.12 KB

Don't know if this helps. Just ran a JMeter test plan. See attached snapshots.

claudiu.cristea’s picture

Here's a complete writing on how I did profiling on this patch with Apache JMeter: https://www.evernote.com/shard/s298/sh/5cbfea3b-458e-4f7a-89b9-8dfc5ef7e...

star-szr’s picture

Status: Needs review » Needs work

Thanks for your work on this @claudiu.cristea, and it's cool to see new profiling techniques coming up.

Besides this high-level review, I'm wondering out loud whether the summary templates would make more sense as render arrays? I think we can do better than templates printing {{ summary }} strings and I'd like to question why that output is going through the theme system.

  1. +++ b/core/modules/image/image.admin.inc
    @@ -5,6 +5,8 @@
    +use Drupal\Core\Template\Attribute;
    
    +++ b/core/modules/image/image.module
    @@ -15,6 +15,7 @@
    +use Drupal\Core\Template\Attribute;
    

    I don't think we need to use Attribute in either of these files any more.

  2. +++ b/core/modules/image/image.admin.inc
    @@ -604,92 +606,79 @@ function theme_image_style_effects($variables) {
    + * Prepare variables for image style preview templates.
    
    +++ b/core/modules/image/image.field.inc
    @@ -382,53 +383,47 @@ function _image_field_required_fields_validate($element, &$form_state) {
    + * Prepare variables for image widget templates.
    
    +++ b/core/modules/image/image.module
    @@ -579,28 +596,21 @@ function image_effect_apply($image, $effect) {
    + * Prepare variables for image style templates.
    

    s/Prepare/Prepares/ per https://drupal.org/node/1354#themepreprocess and the general function documentation standards: http://drupal.org/node/1354#functions

  3. +++ b/core/modules/image/image.admin.inc
    @@ -697,77 +686,93 @@ function theme_image_anchor($variables) {
    + * Default template: image-crop-summary.html.twig
    

    This comment needs to end in a period to be a complete sentence per http://drupal.org/node/1354#drupal.

  4. +++ b/core/modules/image/image.admin.inc
    @@ -697,77 +686,93 @@ function theme_image_anchor($variables) {
    + * Prepares varaibles for image rotate summary templates.
    

    Typo: s/varaibles/variables/

  5. +++ b/core/modules/image/image.field.inc
    @@ -7,6 +7,7 @@
    +use Drupal\Core\Template\Attribute;
    
    @@ -382,53 +383,47 @@ function _image_field_required_fields_validate($element, &$form_state) {
    +  $variables['attributes'] = new Attribute($element['#attributes']);
    

    We don't need to instantiate Attribute here, just set $variables['attributes'] = $element['#attributes']. See #1982024: Lazy-load Attribute objects later in the rendering process only if needed and point #1 here.

  6. +++ b/core/modules/image/image.field.inc
    @@ -382,53 +383,47 @@ function _image_field_required_fields_validate($element, &$form_state) {
    - *   - element: A render element representing the image field widget.
    ...
    + *   - element: An render element representing the image field widget.
    

    Hmmm, I think "A render element" was better :)

  7. +++ b/core/modules/image/image.field.inc
    @@ -382,53 +383,47 @@ function _image_field_required_fields_validate($element, &$form_state) {
    +  $variables['data'] = drupal_render_children($element);
    

    We shouldn't need to use drupal_render_children() anymore - see #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead and #2004402: Discuss deprecating or removing drupal_render_children().

  8. +++ b/core/modules/image/image.field.inc
    @@ -382,53 +383,47 @@ function _image_field_required_fields_validate($element, &$form_state) {
    -  if (isset($item['title']) && drupal_strlen($item['title']) == 0) {
    +  if (empty($item['title'])) {
    

    Let's please remove this change unless we have a really good reason. This is probably drupal_strlen() for a good reason and this change feels very out of scope.

  9. +++ b/core/modules/image/lib/Drupal/image/Tests/ImageDimensionsTest.php
    @@ -69,7 +69,7 @@ function testImageDimensions() {
    -    $img_tag = theme_image_style($variables);
    +    $img_tag = theme('image_style', $variables);
         $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" width="120" height="60" alt="" />');
         $this->assertFalse(file_exists($generated_uri), 'Generated file does not exist.');
         $this->drupalGet($url);
    @@ -90,7 +90,7 @@ function testImageDimensions() {
    
    @@ -90,7 +90,7 @@ function testImageDimensions() {
         );
     
         image_effect_save($style, $effect);
    -    $img_tag = theme_image_style($variables);
    +    $img_tag = theme('image_style', $variables);
         $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" width="60" height="120" alt="" />');
         $this->assertFalse(file_exists($generated_uri), 'Generated file does not exist.');
         $this->drupalGet($url);
    @@ -112,7 +112,7 @@ function testImageDimensions() {
    
    @@ -112,7 +112,7 @@ function testImageDimensions() {
         );
     
         image_effect_save($style, $effect);
    -    $img_tag = theme_image_style($variables);
    +    $img_tag = theme('image_style', $variables);
         $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" width="45" height="90" alt="" />');
         $this->assertFalse(file_exists($generated_uri), 'Generated file does not exist.');
         $this->drupalGet($url);
    @@ -134,7 +134,7 @@ function testImageDimensions() {
    
    @@ -134,7 +134,7 @@ function testImageDimensions() {
         );
     
         image_effect_save($style, $effect);
    -    $img_tag = theme_image_style($variables);
    +    $img_tag = theme('image_style', $variables);
         $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" width="45" height="90" alt="" />');
         $this->assertFalse(file_exists($generated_uri), 'Generated file does not exist.');
         $this->drupalGet($url);
    @@ -152,7 +152,7 @@ function testImageDimensions() {
    
    @@ -152,7 +152,7 @@ function testImageDimensions() {
         );
     
         image_effect_save($style, $effect);
    -    $img_tag = theme_image_style($variables);
    +    $img_tag = theme('image_style', $variables);
         $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" width="45" height="90" alt="" />');
         $this->assertFalse(file_exists($generated_uri), 'Generated file does not exist.');
         $this->drupalGet($url);
    @@ -173,7 +173,7 @@ function testImageDimensions() {
    
    @@ -173,7 +173,7 @@ function testImageDimensions() {
         );
     
         image_effect_save($style, $effect);
    -    $img_tag = theme_image_style($variables);
    +    $img_tag = theme('image_style', $variables);
         $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" alt="" />');
         $this->assertFalse(file_exists($generated_uri), 'Generated file does not exist.');
         $this->drupalGet($url);
    @@ -193,7 +193,7 @@ function testImageDimensions() {
    
    @@ -193,7 +193,7 @@ function testImageDimensions() {
         );
     
         image_effect_save($style, $effect);
    -    $img_tag = theme_image_style($variables);
    +    $img_tag = theme('image_style', $variables);
         $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" width="30" height="30" alt="" />');
         $this->assertFalse(file_exists($generated_uri), 'Generated file does not exist.');
         $this->drupalGet($url);
    @@ -214,7 +214,7 @@ function testImageDimensions() {
    
    @@ -214,7 +214,7 @@ function testImageDimensions() {
         );
     
         image_effect_save($style, $effect);
    -    $img_tag = theme_image_style($variables);
    +    $img_tag = theme('image_style', $variables);
         $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" alt="" />');
         $this->assertFalse(file_exists($generated_uri), 'Generated file does not exist.');
         $this->drupalGet($url);
    @@ -232,7 +232,7 @@ function testImageDimensions() {
    
    @@ -232,7 +232,7 @@ function testImageDimensions() {
         );
     
         image_effect_save($style, $effect);
    -    $img_tag = theme_image_style($variables);
    +    $img_tag = theme('image_style', $variables);
    

    Given #2006152: [meta] Don't call theme() directly anywhere outside drupal_render() we should probably convert these to drupal_render() and they don't seem to be covered by #2009580: Replace theme() with drupal_render() in image module.

  10. +++ b/core/modules/image/lib/Drupal/image/Tests/ImageFieldDisplayTest.php
    @@ -100,9 +100,17 @@ function _testImageFieldFormatters($scheme) {
    +      array(
    +        ':path' => url('node/' . $nid),
    +        ':url' => file_create_url($image_info['uri']),
    +        ':width' => $image_info['width'],
    +        ':height' => $image_info['height']
    +      )
    

    The height line needs a trailing comma per http://drupal.org/coding-standards#array.

  11. +++ b/core/modules/image/templates/image-anchor.html.twig
    @@ -0,0 +1,15 @@
    + * @see template_preprocess()
    
    +++ b/core/modules/image/templates/image-crop-summary.html.twig
    @@ -0,0 +1,15 @@
    + * @see template_preprocess()
    
    +++ b/core/modules/image/templates/image-formatter.html.twig
    @@ -0,0 +1,24 @@
    + * @see template_preprocess()
    
    +++ b/core/modules/image/templates/image-resize-summary.html.twig
    @@ -0,0 +1,15 @@
    + * @see template_preprocess()
    
    +++ b/core/modules/image/templates/image-rotate-summary.html.twig
    @@ -0,0 +1,15 @@
    + * @see template_preprocess()
    
    +++ b/core/modules/image/templates/image-scale-summary.html.twig
    @@ -0,0 +1,15 @@
    + * @see template_preprocess()
    
    +++ b/core/modules/image/templates/image-style-preview.html.twig
    @@ -0,0 +1,52 @@
    + * @see template_preprocess()
    
    +++ b/core/modules/image/templates/image-style.html.twig
    @@ -0,0 +1,31 @@
    + * @see template_preprocess()
    
    +++ b/core/modules/image/templates/image-widget.html.twig
    @@ -0,0 +1,26 @@
    + * @see template_preprocess()
    

    These lines need to be removed, see #2013094: [policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file.

  12. +++ b/core/modules/image/templates/image-style.html.twig
    @@ -0,0 +1,31 @@
    +{# @todo Remove and consolidate with image: http://drupal.org/node/1804614 #}
    

    We really shouldn't be adding @todos at this stage, please remove this line. Hopefully it will get consolidated before release. See #1757550-38: [Meta] Convert core theme functions to Twig templates for some background.

markhalliwell’s picture

Besides this high-level review, I'm wondering out loud whether the summary templates would make more sense as render arrays? I think we can do better than templates printing {{ summary }} strings and I'd like to question why that output is going through the theme system.

An alternative would be to provide the template all the necessary variables and then move all the HTML and text that is added in the preprocess functions and use the new {% trans %} tag to construct & translate the variables at the same time.

See: Drupal 8 Twig Templates and Translations

star-szr’s picture

Now there's an idea, that would make #45.1 a little bit more reasonable.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
27.74 KB
45.18 KB

Thank you, @Cottser, for this detailed review! :)

Fixed all of #95 and dropped {{ summary }} vars (those were stupid). Now we have templates for all summaries but it seems that preprocess functions for summaries are useless now, so I dropped most of them.

Not so sure that I'm dealing very well with {% spaceless %} -- please review carefully template files.

star-szr’s picture

Status: Needs review » Needs work

Sorry for the delay @claudiu.cristea and thanks for your continued work on this issue :) The work here looks good, I like what you did with the image effect summaries, I would like you to review that part to make sure we are not changing any markup.

Here is my review of the interdiff… the patch got quite a bit bigger so I would like to address some scope issues before we proceed any further.

  1. +++ b/core/modules/image/lib/Drupal/image/Plugin/ImageEffect/CropImageEffect.php
    @@ -44,10 +44,14 @@ public function applyEffect($image) {
       public function getSummary() {
    -    return array(
    +    $summary = array(
           '#theme' => 'image_crop_summary',
           '#data' => $this->configuration,
         );
    +    $summary += parent::getSummary();
    +    $summary['#data']['anchor_label'] = self::getAnchorOptions($this->configuration['anchor']);
    +
    +    return $summary;
       }
     
       /**
    @@ -63,17 +67,7 @@ public function getForm() {
    
    @@ -63,17 +67,7 @@ public function getForm() {
         $form['anchor'] = array(
           '#type' => 'radios',
           '#title' => t('Anchor'),
    -      '#options' => array(
    -        'left-top' => t('Top') . ' ' . t('Left'),
    -        'center-top' => t('Top') . ' ' . t('Center'),
    -        'right-top' => t('Top') . ' ' . t('Right'),
    -        'left-center' => t('Center') . ' ' . t('Left'),
    -        'center-center' => t('Center'),
    -        'right-center' => t('Center') . ' ' . t('Right'),
    -        'left-bottom' => t('Bottom') . ' ' . t('Left'),
    -        'center-bottom' => t('Bottom') . ' ' . t('Center'),
    -        'right-bottom' => t('Bottom') . ' ' . t('Right'),
    -      ),
    +      '#options' => self::getAnchorOptions(),
           '#theme' => 'image_anchor',
           '#default_value' => $this->configuration['anchor'],
           '#description' => t('The part of the image that will be retained during the crop.'),
    @@ -81,4 +75,31 @@ public function getForm() {
    
    @@ -81,4 +75,31 @@ public function getForm() {
         return $form;
       }
     
    +  /**
    +   * Builds a list of anchor options.
    +   *
    +   * @var string $anchor
    +   *   (optional) If provided, only the label of this anchor will be returned.
    +   *
    +   * @return array|string
    +   *   A list of anchor options or an anchor label if $anchor has been provided.
    +   */
    +  static protected function getAnchorOptions($anchor = NULL) {
    +    $anchors = array(
    +      'left-top' => t('Top') . ' ' . t('Left'),
    +      'center-top' => t('Top') . ' ' . t('Center'),
    +      'right-top' => t('Top') . ' ' . t('Right'),
    +      'left-center' => t('Center') . ' ' . t('Left'),
    +      'center-center' => t('Center'),
    +      'right-center' => t('Center') . ' ' . t('Right'),
    +      'left-bottom' => t('Bottom') . ' ' . t('Left'),
    +      'center-bottom' => t('Bottom') . ' ' . t('Center'),
    +      'right-bottom' => t('Bottom') . ' ' . t('Right'),
    +    );
    +    if (empty($anchor)) {
    +      return $anchors;
    +    }
    +    return isset($anchors[$anchor]) ? $anchors[$anchor] : NULL;
    +  }
    +
    

    I'm wondering if this change is necessary at all. It looks like we are changing the markup for image_crop_summary. I don't see "(anchor" in the before code anywhere. We should not be changing markup as part of any Twig conversions unless specifically noted.

  2. +++ b/core/modules/image/lib/Drupal/image/Tests/ImageDimensionsTest.php
    @@ -233,7 +226,20 @@ function testImageDimensions() {
    -    $img_tag = theme('image_style', $variables);
    -    $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" alt="" />');
    +    $this->assertEqual($this->getImageTag($variables), '<img class="image-style-test" src="' . $url . '" alt="" />');
       }
    +
    +  /**
    +   * Render an image style element.
    +   *
    +   * drupal_render() alters the passed $variables array by adding a new key
    +   * '#printed' => TRUE. This prevents next call to re-render the element. We
    +   * wrapping drupal_render() in a helper protected method and pass each time a
    +   * fresh so that $variables won't get altered and the element is re-rendered
    +   * each time.
    +   */
    +  protected function getImageTag($variables) {
    +    return drupal_render($variables);
    +  }
    +
    

    I agree with this change but I'm wondering whether this part should be moved to another issue, keeping the tests as theme('image_style').

    If we are keeping getImageTag the docblock needs some tweaking for grammar, I can give some suggestions.

  3. +++ b/core/modules/image/lib/Drupal/image/Tests/ImageFieldDisplayTest.php
    @@ -107,7 +107,7 @@ function _testImageFieldFormatters($scheme) {
    -        ':height' => $image_info['height']
    +        ':height' => $image_info['height'],
    

    This is way out of scope, please remove this change. I know it's tempting to fix all the things but we really need to focus on the Twig conversion here, not minor coding standards tweaks that happen to be in the image module :)

  4. +++ b/core/modules/image/templates/image-crop-summary.html.twig
    @@ -4,12 +4,24 @@
    + *   - width: The with of the resized image.
    
    +++ b/core/modules/image/templates/image-resize-summary.html.twig
    @@ -4,12 +4,17 @@
    + *   - width: The with of the resized image.
    
    +++ b/core/modules/image/templates/image-scale-summary.html.twig
    @@ -4,12 +4,38 @@
    + *   - width: The with of the resized image.
    

    Typo, with = width

  5. +++ b/core/modules/image/templates/image-crop-summary.html.twig
    @@ -4,12 +4,24 @@
    + *   - anchor: The part of the image that retained during the crop.
    

    Wording could be a bit clearer, maybe something like "The part of the image that will be retained after cropping"?

  6. +++ b/core/modules/image/templates/image-rotate-summary.html.twig
    @@ -4,12 +4,26 @@
    + *   - degrees: Image rotation. Positive clockwise, negative counter-clockwise.
    

    This description is a bit terse. Maybe "Degrees to rotate the image, positive values will rotate the image clockwise, negative values counter-clockwise."

  7. +++ b/core/modules/image/templates/image-rotate-summary.html.twig
    @@ -4,12 +4,26 @@
    + *   - bgcolor: The background color of the exposed areas of the image, as hex.
    

    I think "the exposed areas of the image" could be clearer, but I don't have any immediate suggestions on improvement.

  8. +++ b/core/modules/image/templates/image-rotate-summary.html.twig
    @@ -4,12 +4,26 @@
      * @see template_preprocess_image_rotate_summary()
    
    +++ b/core/modules/image/templates/image-scale-summary.html.twig
    @@ -4,12 +4,38 @@
      * @see template_preprocess_image_scale_summary()
    

    If these preprocess functions are being removed, remove the @see as well… this applies for any preprocess functions being removed.

claudiu.cristea’s picture

FileSize
10.46 KB
45.54 KB

OK. I will comment each review point.

#99.1: I'm OK, so let's not change the outputted markup. But let's keep the anchor_label as variable passed to the theme system as long as we are already passing the anchor (machine name) too. Themers will be able to benefit of having this value for specific theming.

#99.2: You asked this conversions in #95.9. Anyway, there's a work already done, why should we drop it? Yes, please provide some grammar fixes to doc block. Sorry, I'm not a native English speaker :-)

#99.3: True, but in #95.10 you asked a similar change, also on an out-of-scope issue. What I did was based on the principle Leave everything cleaner than you found it. This doesn't deserve a followup or another issue ticket. It's just a normal collateral cleanup since I touched that line. I see no problem leaving this as we did with #95.10.

#99.4: Fixed.

#99.5: Ok, ok. I think that I inherited from code previous conversion. Changed.

#99.6: Changed as you suggested.

#99.7: I tried something more clear (not sure on grammar).

#99.8: Right!

Note: The interdiff.txt contains also changes due to #2033669: Image file objects should be classed and conflict fixes.

claudiu.cristea’s picture

Status: Needs work » Needs review

Needs review, of course :)

star-szr’s picture

Regarding #99.3, my apologies. I didn't see that hunk was already being added in the patch. Carry on :)

Will go over this again and provide some more suggestions. Thanks @claudiu.cristea and apologies if I'm treading over the same ground again. Too many patches to keep them all straight in my head!

claudiu.cristea’s picture

@Cottser, no problem!

It turned green again. Add your comments and let't kick this in :)

markhalliwell’s picture

Status: Needs review » Needs work

As promised @claudiu.cristea, here is my review (sorry it's taken so long!):

  1. +++ b/core/modules/image/image.admin.inc
    @@ -64,113 +66,81 @@ function theme_image_style_effects($variables) {
    +    'original' => array('label' => t('Sample original image'), 'path' => $sample['path']),
    +    'derivative' => array('label' => t('Sample modified image'), 'path' => $style->buildUri($sample['path'])),
    ...
    +      'style' => String::format('width: @width; height: @height;', array('@width' => $width . 'px', '@height' => $height . 'px')),
    

    These array definitions are longer than the 80 character limit, separate each of these on their own line (see: https://drupal.org/coding-standards#array).

  2. +++ b/core/modules/image/image.admin.inc
    @@ -64,113 +66,81 @@ function theme_image_style_effects($variables) {
    + * Prepares variables for image anchor templates.
    ...
    + * Default template: image-anchor.html.twig.
    
    +++ b/core/modules/image/image.field.inc
    @@ -384,76 +384,70 @@ function _image_field_required_fields_validate($element, &$form_state) {
    + * Prepares variables for image widget templates.
    ...
    + * Default template: image-widget.html.twig.
    ...
    + * Prepares variables for image formatter templates.
    ...
    + * Default template: image-formatter.html.twig.
    
    +++ b/core/modules/image/image.module
    @@ -343,18 +348,17 @@ function image_style_options($include_empty = TRUE) {
    + * Prepares variables for image style templates.
    ...
    + * Default template: image-style.html.twig.
    

    I may be wrong on this (ie: there may have been a conversation/issue I didn't read), but shouldn't these just be:
    Prepares variables for theme-id.html.twig.?

  3. +++ b/core/modules/image/image.admin.inc
    @@ -64,113 +66,81 @@ function theme_image_style_effects($variables) {
    + * @param array $variables
    + *   An associative array containing:
    + *   - element: An associative array containing the image.
    

    Shouldn't we include the properties of element here? This is a little ambiguous to know exactly what actually is in $variables['element']. If we are just leaving this let the themer do a {% dump(element) %} to figure this out, then this comment can be ignored.

  4. +++ b/core/modules/image/image.field.inc
    @@ -463,11 +457,6 @@ function theme_image_formatter($variables) {
         // When displaying an image inside a link, the html option must be TRUE.
         $options['html'] = TRUE;
    -    $output = l($image, $path, $options);
    +    $variables['url'] = url($path, $options);
    

    If we're no longer passing this through l(), then $options['html'] = TRUE; should be removed (along with the comment above it).

  5. +++ b/core/modules/image/image.module
    @@ -170,6 +170,7 @@ function image_theme() {
    +      'template' => 'image-style',
    
    @@ -180,36 +181,40 @@ function image_theme() {
    +      'template' => 'image-style-preview',
    ...
    +      'template' => 'image-anchor',
    ...
    +      'template' => 'image-resize-summary',
    ...
    +      'template' => 'image-scale-summary',
    ...
    +      'template' => 'image-crop-summary',
    ...
    +      'template' => 'image-rotate-summary',
    ...
    +      'template' => 'image-widget',
    ...
    +      'template' => 'image-formatter',
    

    These aren't necessary. These are automatically converted based on the key name used for registering the theme hook. This is only needed if you're defining a template that differs from the hook.

  6. +++ b/core/modules/image/lib/Drupal/image/Plugin/ImageEffect/CropImageEffect.php
    @@ -45,10 +45,14 @@ public function applyEffect(ImageInterface $image) {
    +    $summary['#data']['anchor_label'] = self::getAnchorOptions($this->configuration['anchor']);
    
    @@ -64,17 +68,7 @@ public function getForm() {
    +      '#options' => self::getAnchorOptions(),
    

    Use $this->getAnchorOptions() instead of self to keep things consistent.

  7. +++ b/core/modules/image/lib/Drupal/image/Tests/ImageDimensionsTest.php
    @@ -70,8 +71,7 @@ function testImageDimensions() {
    +    $this->assertEqual($this->getImageTag($variables), '<img class="image-style-test" src="' . $url . '" width="120" height="60" alt="" />');
    
    @@ -91,8 +91,7 @@ function testImageDimensions() {
    +    $this->assertEqual($this->getImageTag($variables), '<img class="image-style-test" src="' . $url . '" width="60" height="120" alt="" />');
    
    @@ -113,8 +112,7 @@ function testImageDimensions() {
    +    $this->assertEqual($this->getImageTag($variables), '<img class="image-style-test" src="' . $url . '" width="45" height="90" alt="" />');
    
    @@ -135,8 +133,7 @@ function testImageDimensions() {
    +    $this->assertEqual($this->getImageTag($variables), '<img class="image-style-test" src="' . $url . '" width="45" height="90" alt="" />');
    
    @@ -153,8 +150,7 @@ function testImageDimensions() {
    +    $this->assertEqual($this->getImageTag($variables), '<img class="image-style-test" src="' . $url . '" width="45" height="90" alt="" />');
    
    @@ -174,8 +170,7 @@ function testImageDimensions() {
    +    $this->assertEqual($this->getImageTag($variables), '<img class="image-style-test" src="' . $url . '" alt="" />');
    
    @@ -194,8 +189,7 @@ function testImageDimensions() {
    +    $this->assertEqual($this->getImageTag($variables), '<img class="image-style-test" src="' . $url . '" width="30" height="30" alt="" />');
    
    @@ -215,8 +209,7 @@ function testImageDimensions() {
    +    $this->assertEqual($this->getImageTag($variables), '<img class="image-style-test" src="' . $url . '" alt="" />');
    
    @@ -234,7 +227,20 @@ function testImageDimensions() {
    +    $this->assertEqual($this->getImageTag($variables), '<img class="image-style-test" src="' . $url . '" alt="" />');
    ...
    +
    +  /**
    +   * Render an image style element.
    +   *
    +   * drupal_render() alters the passed $variables array by adding a new key
    +   * '#printed' => TRUE. This prevents next call to re-render the element. We
    +   * wrap drupal_render() in a helper protected method and pass each time a
    +   * fresh array so that $variables won't get altered and the element is
    +   * re-rendered each time.
    +   */
    +  protected function getImageTag($variables) {
    +    return drupal_render($variables);
    +  }
    +
    

    I get why and how this got here, but it is unnecessary to create a protected method that simply wraps drupal_render(). It would be different (and needed) if we adding additional "default" variables before sending it to get rendered, but since we're not just replace all the $this->getImageTag($variables) with drupal_render($variables) and remove this added method.

  8. +++ b/core/modules/image/templates/image-style.html.twig
    @@ -0,0 +1,29 @@
    + *       http://www.w3.org/TR/REC-html40/struct/objects.html#h-13.8
    + *       http://www.w3.org/TR/xhtml1/dtds.html
    + *       http://dev.w3.org/html5/spec/Overview.html#alt
    + *
    + * @see template_preprocess_image_style()
    

    Move these links below as additional @see links.

  9. +++ b/core/modules/image/templates/image-widget.html.twig
    @@ -0,0 +1,25 @@
    + * - attributes: HTML attributes for the containing element.
    ...
    +<div class="{{ attributes.class }}"{{ attributes }}>
    

    If we're not going to expand the attributes in the doc, then we should just do <div{{attributes}} for the wrapper below.

claudiu.cristea’s picture

FileSize
4.65 KB
45.71 KB

Thank you for review, @Mark Carver. I'm replying point by point:

  • #104.1: OK. Fixed.
  • #104.2: That pattern was taken from https://drupal.org/node/1354#themepreprocess and that one is an official Coding Standards section.
  • #104.3: Let's leave just 'element' for now.
  • #104.4: Good catch. Fixed. Removed also $path = $variables['path']['path']; -- it makes no sense that line when using that variable only once.
  • #104.5: If I remove 'template' everything is broken. Tests are failing with 500. I tried but no luck and had to revert them back.
  • #104.6: Well, I disagree here. Static methods should not be called with an instance prefix. Anyway I wrongly used self, static is better because is allowing the method extension.
  • #104.7: Why would we redefine $variables for each assertion? Why would we duplicate the code? ::getImageTag it's not a simple wrapper, it assures that we always send the initial $variables array to drupal_render(). IMO this is more elegant way to code.
  • #104.8: Agree. Looks better.
  • #104.9: I have no knowledge here so going on your suggestion.
claudiu.cristea’s picture

Status: Needs work » Needs review

Ah!

markhalliwell’s picture

  • #104.2: Hmm, ok. Must have missed this transition somewhere.
  • #104.5: I was thinking ahead when/if theme functions no longer exist and the "template" would be automatically filled (like theme functions are now). They are needed, never mind me :)
  • #104.6: I honestly thought this was in a test (which wouldn't make sense to have self in a test), but static is fine :) Glad I helped fix something, even if it was for a different reason lol
  • #104.7: I really should have read the comment for that method (was just looking at the code inside it). Yes, this is fine and definitely needed.
star-szr’s picture

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

This now needs a reroll. Can you still work on this @claudiu.cristea? If not we have some sprinters who can :)

claudiu.cristea’s picture

@Cottser,

Can you still work on this @claudiu.cristea?

Why you asking that? The latest patch waits since August 26 for a RTBC or other reviews (if case).

Course, I will reroll the patch :)

claudiu.cristea’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
45.66 KB

Here we go.

claudiu.cristea’s picture

RTBC or more reviews, please.

star-szr’s picture

Just asking because some people forget or lose interest. Thanks for being awesome :)

star-szr’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

Issue tags: +Core Review Bonus

Tagging with "Core Review Bonus" #2094585: [policy, no patch] Core review bonus, to see if that helps get a review :)

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

@thedavidmeister, In fact this issue has been reviewed, not only once, but for some reasons it still not get RTBC :)

thedavidmeister’s picture

Well, I'm considering marking it as RTBC a "review" because you have to review it to be sure it's RTBC.

markhalliwell’s picture

In fairness, all patches must be reviewed (regardless if it's just a simple re-roll). Also, this issue still needs manual testing and profiling done before an RTBC status can be obtained.

I've just left a tell for @joelpittet to see if he can start on profiling. As for manual testing... if no one picks that up by this weekend, I'll do what I can.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

@Mark Carver, @thedavidmeister,

I've done profiling in #94. See this writeup on how it was done http://webikon.com/cases/drupal-twig-conversion-performance-test-with-jm....

I'm not against reviewing, manually testing, etc. This was not an easy conversion that's why maybe I want some attention to get it in. Otherwise we will still dance around with rerolling & stuff.

markhalliwell’s picture

@claudiu.cristea, yes I remember. But there has been a couple reviews/patches since then. I think @joelpittet could do another one real quick. If not, then perhaps the tag should be removed if everyone (those who do profiling, I don't) agree that #94 is ok.

claudiu.cristea’s picture

@Mark Carver, I'm fine. Let's see profiling and reviews

joelpittet’s picture

@claudiu.cristea that profiling looks cool. I'll do the same with the xhprof kit we've been using but I'm super glad to see another approach. And we'll see how they compare for the times. @see https://drupal.org/contributor-tasks/profiling

joelpittet’s picture

Scenario 1

As indicated in https://www.evernote.com/shard/s298/sh/5cbfea3b-458e-4f7a-89b9-8dfc5ef7e...

/admin/config/media/image-styles/manage/large

Added:

Scale 480x480 (upscaling allowed)
Crop 200x200
Resize 300x300
Rotate 7°

Tests the following twig files:
core/modules/image/templates/image-style-preview.html.twig
core/modules/image/templates/image-scale-summary.html.twig
core/modules/image/templates/image-crop-summary.html.twig
core/modules/image/templates/image-resize-summary.html.twig
core/modules/image/templates/image-rotate-summary.html.twig

=== 8.x..8.x compared (526a239923027..526a23ef211f9):

ct  : 53,817|53,817|0|0.0%
wt  : 264,356|263,411|-945|-0.4%
cpu : 239,656|238,186|-1,470|-0.6%
mu  : 12,530,432|12,530,432|0|0.0%
pmu : 12,701,832|12,701,832|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=526a239923027&...

=== 8.x..1898420-twig-theme-image compared (526a239923027..526a240c73a50):

ct  : 53,817|54,377|560|1.0%
wt  : 264,356|264,390|34|0.0%
cpu : 239,656|239,821|165|0.1%
mu  : 12,530,432|12,731,464|201,032|1.6%
pmu : 12,701,832|12,902,880|201,048|1.6%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=526a239923027&...

joelpittet’s picture

@claudiu.cristea would it be cool if I added your evernote write-up to show an alternative on the task profiling document?

https://drupal.org/contributor-tasks/profiling

claudiu.cristea’s picture

@joelpittet, OK. Use this link http://webikon.com/cases/drupal-twig-conversion-performance-test-with-jm.... That for Evernote was a temporary work.

joelpittet’s picture

Thanks, the page with instructions needs a few re-writes but I tacked that link on there as well because it's a really nice approach. Thanks @claudiu.cristea

https://drupal.org/contributor-tasks/profiling#jmeter

joelpittet’s picture

Issue summary: View changes

Updated issue summary.

joelpittet’s picture

110: twig-1898420-110.patch queued for re-testing.

chanderbhushan’s picture

110: twig-1898420-110.patch queued for re-testing.

markhalliwell’s picture

Assigned: joelpittet » Unassigned
Issue summary: View changes
Issue tags: -needs profiling

Still needs manual testing and issue summary update. Patch still passes, willing to RTBC once both needs are met.

star-szr’s picture

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

Tagging for reroll.

InternetDevels’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
47.71 KB

Here is the re-roll.

Status: Needs review » Needs work

The last submitted patch, 129: image-twig-1898420-129.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
39.73 KB

Removed " Builds a list of anchor options." helper function as it's out of scope of the conversion, unless someone can speak to it as asked by @Cottser in #99.

Re-rolled against #110 because #129 failed and doesn't apply anymore.

Status: Needs review » Needs work

The last submitted patch, 131: 1898420-twig-image-131-reroll.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 131: 1898420-twig-image-131-reroll.patch, failed testing.

star-szr’s picture

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

star-szr’s picture

I think we can keep the helper function, it's just in service of the tests.

mondrake’s picture

Status: Needs work » Needs review
FileSize
2.19 KB
40.99 KB

I hope I am not stepping on any toes. Here's a reroll + fixes to template_preprocess_image_widget which moved to image.field.inc, and the patch in 131 did not follow.

joelpittet’s picture

Status: Needs review » Needs work

@mondrake not at all, thank you very much for the reroll and fixes!

+++ b/core/modules/image/lib/Drupal/image/Plugin/ImageEffect/CropImageEffect.php
@@ -43,7 +43,6 @@ public function getSummary() {
-    $summary['#data']['anchor_label'] = static::getAnchorOptions($this->configuration['anchor']);

Is this change needed?

mondrake’s picture

Status: Needs work » Needs review

@joelpittet - yes, if we want to keep that getAnchorOptions method out of this patch. It was present in #110, then removed in #131. So that removal is needed to avoid a fatal for calling a non existing method (I ran into that just after applying the previous patch). Or we may want to put that helper back...

joelpittet’s picture

@mondrake oh sorry, I reviewed the interdiff:S

mondrake’s picture

Assigned: Unassigned » mondrake
Status: Needs review » Needs work

Actually testing manually the image style preview, noticed that the cache bypass token is no longer displayed. Will work on a fix.

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review
FileSize
10.02 KB
41.98 KB

So, this patch reworks the theme_image_style_preview conversion. The cache bypass token is back in the HTML. Also checked manually HTML before and after and it seems to me it is matching now.

mondrake’s picture

143: 1898420-twig-image-143.patch queued for re-testing.

star-szr’s picture

Issue summary: View changes
Issue tags: +Needs steps to reproduce

Adding a placeholder for testing steps to the issue summary. Let's document how to test all these templates!

mondrake’s picture

@Cottser can you point to a good example of such directions? Thanks

star-szr’s picture

mondrake’s picture

Assigned: Unassigned » mondrake
Issue summary: View changes

Updating issue summary to indicate where templates are used, preparing to add manual testing steps.

mondrake’s picture

Issue summary: View changes
mondrake’s picture

Assigned: mondrake » Unassigned
Issue summary: View changes

Added steps to test manually the Admin UI related theme templates. Still todo field widget and formatter ones, will come back when I can if noone beats me on that :)

mondrake’s picture

Added steps to test image widget and formatter templates. Removed tags for issue summary update and needs steps to reproduce, please set back if not clear enough.

mondrake’s picture

143: 1898420-twig-image-143.patch queued for re-testing.

mondrake’s picture

143: 1898420-twig-image-143.patch queued for re-testing.

star-szr’s picture

Thank you @mondrake, I totally missed your testing steps, they look very thorough! Since this has a lot of templates it might be helpful to note when each template is used/displayed but with twig_debug on it might not be so bad.

mondrake’s picture

Issue summary: View changes

Per #154, added reference to templates used in the testing steps. Hopefully rightly ;)

mondrake’s picture

mondrake’s picture

Rerolled after #1939068: Convert theme_image() to Twig, patch no longer applied.

Merge conflict only in ImageDimensionsTest, had to add a "\n" in a number of asserts

-    $this->assertEqual($this->getImageTag($variables), '<img class="image-style-test" src="' . $url . '" width="120" height="60" alt="" />');
+    $this->assertEqual($this->getImageTag($variables), '<img class="image-style-test" src="' . $url . '" width="120" height="60" alt="" />' . "\n");

Status: Needs review » Needs work

The last submitted patch, 157: 1898420-twig-image-157.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review
FileSize
9.34 KB
45.89 KB

Uhm. Not sure how to deal with the newline characters that drupal_render appends at the end of the formatted HTML. Any help welcome.

star-szr’s picture

That's not really drupal_render() but is coming from the new image.html.twig which has a newline at the EOF to meet https://drupal.org/coding-standards#indenting. Thanks for keeping on top of this @mondrake!

claudiu.cristea’s picture

@mondrake, I cannot set this patch to RTBC because I worked on it but from my point of view is super-RTBC. @Cottser, i think it's time to push :)

claudiu.cristea’s picture

Issue summary: View changes
filijonka’s picture

Hi

I was asked if I could do the manual testing last week and first now I got the time. I notice that there has been some new posts since then with patches but I used the latest revision (with git) and all steps worked as planned.
The only "problem" was that the gearimage was missing on the fieldforms.

star-szr’s picture

Assigned: Unassigned » star-szr

Thank you all, I'm going to sprinting at MidCamp starting tomorrow so putting this on my TODO list :)

mondrake’s picture

FileSize
45.89 KB
532 bytes

Needed a reroll because of #2136197: Move field/instance/widget/formatter settings out of annotation / plugin definition. Just change in the patch context.

mondrake’s picture

FileSize
45.92 KB
1.06 KB
star-szr’s picture

Thank you so much @mondrake for keeping this patch up to date. I haven't forgotten about my promise but I didn't get as much interrupted time to review this properly. It's still on my list and I will review this as soon as I can.

star-szr’s picture

Assigned: star-szr » Unassigned
Status: Needs review » Needs work

It's been a while so I'm getting familiar with the patch again, here is a code review for now, I apologize for the length in advance. This is looking very good overall, the main point that needs a closer look is the image resize summary. The rest is pretty much all nitpicks and commentary :)

  1. +++ b/core/modules/image/image.admin.inc
    @@ -4,8 +4,10 @@
    +
    ...
    +
    

    Nitpick alert: I would suggest moving these changes to #1938910: Convert image theme tables to table #type if anything. This seems out of scope to this issue when the patch is already a bit big.

  2. +++ b/core/modules/image/image.admin.inc
    @@ -71,113 +73,100 @@ function theme_image_style_effects($variables) {
    +function template_preprocess_image_style_preview(&$variables) {
    +
    +  // Style information.
    

    Extra blank line at the top of this function can be removed.

  3. +++ b/core/modules/image/image.admin.inc
    @@ -71,113 +73,100 @@ function theme_image_style_effects($variables) {
    +  $variables['original'] = array(
         'width' => $original_image->getWidth(),
         'height' => $original_image->getHeight(),
       );
    -  if ($original_image['width'] > $original_image['height']) {
    -    $original_width = min($original_image['width'], $sample_width);
    -    $original_height = round($original_width / $original_image['width'] * $original_image['height']);
    +  if ($original_image->getWidth() > $original_image->getHeight()) {
    +    $variables['preview']['original']['width'] = min($original_image->getWidth(), $sample_width);
    +    $variables['preview']['original']['height'] = round($variables['preview']['original']['width'] / $original_image->getWidth() * $original_image->getHeight());
       }
       else {
    -    $original_height = min($original_image['height'], $sample_height);
    -    $original_width = round($original_height / $original_image['height'] * $original_image['width']);
    +    $variables['preview']['original']['height'] = min($original_image->getHeight(), $sample_height);
    +    $variables['preview']['original']['width'] = round($variables['preview']['original']['height'] / $original_image->getHeight() * $original_image->getWidth());
    ...
    +  $variables['derivative'] = array(
         'width' => $preview_image->getWidth(),
         'height' => $preview_image->getHeight(),
       );
    -  if ($preview_image['width'] > $preview_image['height']) {
    -    $preview_width = min($preview_image['width'], $sample_width);
    -    $preview_height = round($preview_width / $preview_image['width'] * $preview_image['height']);
    +  if ($preview_image->getWidth() > $preview_image->getHeight()) {
    +    $variables['preview']['derivative']['width'] = min($preview_image->getWidth(), $sample_width);
    +    $variables['preview']['derivative']['height'] = round($variables['preview']['derivative']['width'] / $preview_image->getWidth() * $preview_image->getHeight());
       }
       else {
    -    $preview_height = min($preview_image['height'], $sample_height);
    -    $preview_width = round($preview_height / $preview_image['height'] * $preview_image['width']);
    +    $variables['preview']['derivative']['height'] = min($preview_image->getHeight(), $sample_height);
    +    $variables['preview']['derivative']['width'] = round($variables['preview']['derivative']['height'] / $preview_image->getHeight() * $preview_image->getWidth());
    

    We're adding a lot of calls to getWidth()/getHeight() here when we've already done so once. Maybe this is premature optimization but it seems unnecessary to me to add all these calls and it looks like each call makes it all the way to GD or whatever image toolkit is being used.

  4. +++ b/core/modules/image/image.admin.inc
    @@ -185,86 +174,21 @@ function theme_image_anchor($variables) {
    -  if ($data['width'] && $data['height']) {
    -    return String::checkPlain($data['width']) . 'x' . String::checkPlain($data['height']);
    -  }
    -  else {
    -    return ($data['width']) ? t('width @width', array('@width' => $data['width'])) : t('height @height', array('@height' => $data['height']));
    -  }
    
    +++ b/core/modules/image/templates/image-resize-summary.html.twig
    @@ -0,0 +1,18 @@
    +{{ data.width }}x{{ data.height }}
    

    I think we are losing display logic from the resize summary when there is no width or no height - this should probably look more similar to image_scale_summary (and be consolidated later). Also until we have autoescape on for Twig, calls to check_plain/String::checkPlain need to be added as {{ var_name|e }} in the template so that strings are escaped.

  5. +++ b/core/modules/image/image.field.inc
    @@ -52,51 +48,45 @@ function theme_image_widget($variables) {
      *
      * @ingroup themeable
      */
    -function theme_image_formatter($variables) {
    +function template_preprocess_image_formatter(&$variables) {
    

    Remove @ingroup themeable, it should be only used for themeable output in Twig templates (and theme functions but we're getting rid of 'em).

  6. +++ b/core/modules/image/image.module
    @@ -319,28 +319,21 @@ function theme_image_style($variables) {
    -
    

    I don't think this blank line is hurting anyone, let's leave it be :)

  7. +++ b/core/modules/image/image.module
    @@ -319,28 +319,21 @@ function theme_image_style($variables) {
    +    '#alt' => isset($variables['alt']) ? $variables['alt'] : NULL,
    +    '#title' => isset($variables['title']) ? $variables['title'] : NULL,
    ...
    -  if (isset($variables['alt']) || array_key_exists('alt', $variables)) {
    -    $image['#alt'] = $variables['alt'];
    -  }
    -  if (isset($variables['title']) || array_key_exists('title', $variables)) {
    -    $image['#title'] = $variables['title'];
    -  }
    

    Per discussion around #2009662-75: [REGRESSION] Replace theme() with drupal_render() in picture module we should probably keep the array_key_exists() stuff as-is, at least for 'alt'. I would prefer alt to be more explicit and use FALSE instead of NULL to prevent the default empty string but we need to make sure this works properly. There may be some missing test coverage.

  8. +++ b/core/modules/image/image.module
    @@ -319,28 +319,21 @@ function theme_image_style($variables) {
    +    '#style' => $style,
    

    This threw me off at first but I think it's just sending this extra variable to '#theme' => 'image' which is cool by me.

  9. +++ b/core/modules/image/lib/Drupal/image/Plugin/ImageEffect/CropImageEffect.php
    @@ -38,10 +38,13 @@ public function applyEffect(ImageInterface $image) {
    -    return array(
    +    $summary = array(
           '#theme' => 'image_crop_summary',
           '#data' => $this->configuration,
         );
    +    $summary += parent::getSummary();
    +
    +    return $summary;
    
    +++ b/core/modules/image/lib/Drupal/image/Plugin/ImageEffect/ResizeImageEffect.php
    @@ -46,10 +46,14 @@ public function transformDimensions(array &$dimensions) {
    -    return array(
    +    $summary = parent::getSummary();
    +    unset($summary['#markup']);
    +    $summary += array(
           '#theme' => 'image_resize_summary',
           '#data' => $this->configuration,
         );
    +
    +    return $summary;
    

    This makes a lot of sense but why does ResizeImageEffect use a different pattern? All the others are consistent with CropImageEffect.

  10. +++ b/core/modules/image/lib/Drupal/image/Tests/ImageDimensionsTest.php
    @@ -34,7 +34,7 @@ public static function getInfo() {
    -  function testImageDimensions() {
    +  public function testImageDimensions() {
    

    I would leave this kind of change to a minor cleanup issue instead.

  11. +++ b/core/modules/image/lib/Drupal/image/Tests/ImageDimensionsTest.php
    @@ -234,7 +227,20 @@ function testImageDimensions() {
    +
    +  /**
    +   * Render an image style element.
    +   *
    +   * drupal_render() alters the passed $variables array by adding a new key
    +   * '#printed' => TRUE. This prevents next call to re-render the element. We
    +   * wrap drupal_render() in a helper protected method and pass each time a
    +   * fresh array so that $variables won't get altered and the element is
    +   * re-rendered each time.
    +   */
    +  protected function getImageTag($variables) {
    +    return drupal_render($variables);
    +  }
    +
    

    You might be interested in #2227569: Add a helper method to WebTestBase for checking the output of render arrays and deprecate assertThemeOutput() :)

  12. +++ b/core/modules/image/templates/image-crop-summary.html.twig
    @@ -0,0 +1,22 @@
    +{% spaceless %}
    +  {{ data.width }}x{{ data.height }}
    +{% endspaceless %}
    

    Have we tried taking out all the spaceless tags from the templates and seeing what (if any) tests fail? It would be nice to do without them, I think in this particular case it's probably not doing anything anyway since it's meant to remove space in between HTML tags.

  13. +++ b/core/modules/image/templates/image-rotate-summary.html.twig
    @@ -0,0 +1,29 @@
    +      random between -{{ data.degrees|abs }}&deg; and {{ data.degrees|abs }}&deg;
    

    Just have to say, great use of built-in Twig filters for this (abs). Much cleaner than the theme function version IMO :)

  14. +++ b/core/modules/image/templates/image-style-preview.html.twig
    @@ -0,0 +1,57 @@
    +  </div>{# End preview-image-wrapper. #}
    ...
    +  </div>{# End preview-image-wrapper. #}
    

    I could do without these closing Twig comments, I don't think they are necessary. The template is pretty self-contained.

  15. +++ b/core/modules/image/templates/image-style.html.twig
    @@ -0,0 +1,29 @@
    + *     theme('image') to pass a meaningful value for this variable.
    

    theme() is now _theme() and is for internal use only so it would be nice to not add another instance of documentation for later cleanup but I don't have any immediate suggestions. At least we need to update this to _theme().

mondrake’s picture

Assigned: Unassigned » mondrake

ok i'll take it on

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review
FileSize
44.89 KB
15.99 KB

New patch re. #168:

1. OK
2. OK
3. OK
4. OK - crop, resize and scale all have the same display logic for width and height now. This may help consolidation later.
5. OK
6. OK
7. OK
8. Removed that line - it's not in the variables signature for theme 'image' so it's just not going to be passed to image anyway.
9. OK
10. OK
11. Nothing done here.
12. OK all spaceless tags removed, let's see.
13. -
14. OK
15. Just removed all the comment section, this is already repeated several times in hook_theme, template_preprocess and also in the image theme. I think making a @see reference to the preprocess is enough.

Status: Needs review » Needs work

The last submitted patch, 170: 1898420-twig-image-170.patch, failed testing.

star-szr’s picture

Thank you @mondrake! I think the failing tests should be fairly easy to update but this change should happen first because I suspect it will fix at least some:

+++ b/core/modules/image/image.admin.inc
@@ -99,18 +96,18 @@ function template_preprocess_image_style_preview(&$variables) {
-  $variables['original_url'] = file_create_url($original_path);
...
+    'url' => file_create_url($original_path),

@@ -121,18 +118,18 @@ function template_preprocess_image_style_preview(&$variables) {
-  $variables['derivative_url'] = file_create_url($preview_file);
...
+    'url' => file_create_url($preview_file),

Adding these is messing things up, we're getting additional "url" attributes, so I'd suggest reverting these changes.

mondrake’s picture

Status: Needs work » Needs review
FileSize
45.01 KB
4.09 KB

Thanks @Cottser, now I remember why I had the URLs out of the sub-array :)))

However, instead of reverting I'd prefer to change how image attributes are passed, and keep the 'original' and 'derivative' arrays as per new patch, I think it improves readability.

I am not expecting this one to pass yet, let's see where we are.

star-szr’s picture

That makes sense too :)

+++ b/core/modules/image/templates/image-crop-summary.html.twig
@@ -17,16 +17,11 @@
-{% if data.width and data.height %}
-  {{ data.width|e }}x{{ data.height|e }}
+{% if data.width and data.height %}{{ data.width|e }}x{{ data.height|e }}
...
-    {% trans %}
-      width {{ data.width|e }}
-    {% endtrans %}
+    {% trans %}width {{ data.width|e }}{% endtrans %}

+++ b/core/modules/image/templates/image-resize-summary.html.twig
@@ -15,16 +15,11 @@
-{% if data.width and data.height %}
-  {{ data.width|e }}x{{ data.height|e }}
+{% if data.width and data.height %}{{ data.width|e }}x{{ data.height|e }}

+++ b/core/modules/image/templates/image-scale-summary.html.twig
@@ -16,17 +16,12 @@
-{% if data.width and data.height %}
-  {{ data.width|e }}x{{ data.height|e }}
+{% if data.width and data.height %}{{ data.width|e }}x{{ data.height|e }}

I would suggest trying to use whitespace operators (-) instead of making these templates harder to read and possibly breaking coding standards. http://twig.sensiolabs.org/doc/templates.html#whitespace-control

Status: Needs review » Needs work

The last submitted patch, 173: 1898420-twig-image-173.patch, failed testing.

star-szr’s picture

After looking at the test fails I'm not sure the crop/resize/scale summary templates need to be changed at all.

mondrake’s picture

Status: Needs work » Needs review
Related issues: +#872206: Add possibility to not upscale for "Scale and crop" effect
FileSize
44.22 KB
4.25 KB

#176 well I still believe with input in #174 we can do - seeing what #872206: Add possibility to not upscale for "Scale and crop" effect will bring up there will be possibilities to reuse/consolidate later.

This patch passes locally on the tests that were failing in #170 and #173, let's see what bot thinks :)

mondrake’s picture

Found a glitch while testing manually - the images on the image style preview won't size correctly

The last submitted patch, 177: 1898420-twig-image-177.patch, failed testing.

markhalliwell’s picture

I canceled #177 since there's no point in having two run :)

Status: Needs review » Needs work

The last submitted patch, 178: 1898420-twig-image-178.patch, failed testing.

mondrake’s picture

Failure in #181 seems unrelated - another patch failed on Datetime field test recently, but that test passes locally. Retrying. Maybe #2160365: Date field required marker rendered as "Array" related?

mondrake’s picture

178: 1898420-twig-image-178.patch queued for re-testing.

The last submitted patch, 178: 1898420-twig-image-178.patch, failed testing.

mondrake’s picture

178: 1898420-twig-image-178.patch queued for re-testing.

mondrake’s picture

Status: Needs work » Needs review

green

star-szr’s picture

Cool, looking good and green! One more question before I start to poke at some manual testing :)

+++ b/core/modules/image/image.admin.inc
@@ -71,113 +71,107 @@ function theme_image_style_effects($variables) {
+    '#attributes' => array(
+      'width' => $variables['original']['width'],
+      'height' => $variables['original']['height'],
+      'style' => $variables['original']['style'],
+    ),
...
+    '#attributes' => array(
+      'width' => $variables['derivative']['width'],
+      'height' => $variables['derivative']['height'],
+      'style' => $variables['derivative']['style'],
+    ),

The 'style' lines here look suspect, are we really passing inline styles here or is this actually image style data that is being added?

mondrake’s picture

No, it's *really* inline styling here. This is the image style preview form. Here we have to show both the original image and the derivative resulting from the execution of the image style effects - each image will have its own 'real' width and height, but no matter what they are finally, we need to 'scale' them to fit in the preview form, and do so by setting the width and height style.

Try with and without the two lines added in #178 - you'll see what I mean :))

mondrake’s picture

Re #187 and #188, here I made the image inline styling more clear, taking it away from the 'original' and 'derivative' subarrays and making it specific to the rendered images.

mondrake’s picture

189: 1898420-twig-image-189.patch queued for re-testing.

mondrake’s picture

189: 1898420-twig-image-189.patch queued for re-testing.

star-szr’s picture

Assigned: Unassigned » star-szr

Putting this back on my list to continue testing and poking at :)

Thanks for the last clarification and keeping this up so fastidiously @mondrake!

mondrake’s picture

star-szr’s picture

Issue summary: View changes

Working on testing this tonight. I'm removing the suggested commit message, the Dreditor-generated one is fine.

star-szr’s picture

Status: Needs review » Needs work
FileSize
255.23 KB

The image style UI and field formatter components look great from a manual testing/markup perspective. There's even a markup fix in the rotate preview, changing &deg to &deg; :D

There seems to be a bug with the field widget changes where the image is being displayed twice. I don't get this on 8.x, only with the patch. Screenshot:

1898420-195-duplicate-widget.png

mondrake’s picture

Status: Needs work » Needs review
FileSize
461 bytes

Hm. That's because the 'data' variable contains the 'preview' object too, so it gets rendered both as 'preview' and then as 'data.preview'. Here's the quickest patch I could think of. Maybe there are more clever ways...

mondrake’s picture

FileSize
44.68 KB
mondrake’s picture

FileSize
44.63 KB
710 bytes

This seems simpler.

Jeff Burnz’s picture

Is this a viable case for without filter:

{{ data|without('preview') }}

As a general approach or pattern I am generally against unsetting stuff in preprocess unless it's critically necessary to do so.

mondrake’s picture

FileSize
44.62 KB
860 bytes
211 bytes

Cool :) That's the clever way I was looking for... but could not find this filter anywhere, is there any doc available? It's not on Twig website.

So this implements #199, and ends up being the only change vs. #193. Tested manually and looks ok to me, @Cottser the last word.

Thanks @Jeff Burns

star-szr’s picture

Nice!

'without' is an additional filter that we're adding as part of the core Twig extension. Full docs to come on all those, in the meantime here are two relevant change records regarding |without:

https://drupal.org/node/2212845
https://drupal.org/node/2240005

star-szr’s picture

Might be worth a small comment in the Twig template to explain why the preview is being filtered out via |without.

mondrake’s picture

FileSize
479 bytes
44.69 KB

Like this?

mondrake’s picture

203: 1898420-twig-image-203.patch queued for re-testing.

star-szr’s picture

Yes, it's about time I come back to this! I will see how far I can get today.

star-szr’s picture

Status: Needs review » Needs work

It seems we have a duplicate div/ID happening with the image widget template.

<div id="edit-field-image-0-upload-ajax-wrapper"> occurs twice with the patch. Looks to be inside image-widget.html.twig after image-style.html.twig, coming from the data variable again.

mondrake’s picture

Status: Needs work » Needs review
FileSize
659 bytes
44.76 KB

Render data without #prefix and #suffix too.

Status: Needs review » Needs work

The last submitted patch, 207: 1898420-twig-image-207.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review

207: 1898420-twig-image-207.patch queued for re-testing.

mondrake’s picture

Assigned: star-szr » mondrake
Status: Needs review » Needs work

This needs a better solution. Working on a patch.

star-szr’s picture

Thanks @mondrake!

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review
FileSize
2.35 KB
44.76 KB

The original theme_image_widget() is using drupal_render_children() to get the widget sub-elements rendered, i.e. avoiding element properties (array keys starting with a '#'). This is why #203 is rendering #prefix and #suffix - moving the entire $element to $variables['data'] takes the properties too. So here I changed to build $variables['data'] by looping through Element::children($element) so we only take the widget's subelements in. Since 'preview' is also one of the element's subelements, I also changed the preprocess and the template not to have a separate 'preview' variable, but to keep it in the 'data' variable and render from there.

Status: Needs review » Needs work

The last submitted patch, 212: 1898420-twig-image-212.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review

212: 1898420-twig-image-212.patch queued for re-testing.

star-szr’s picture

Assigned: Unassigned » star-szr

Oh cool that seems way more sensible! Back on my list for more reviewing and such. Thanks @mondrake :)

joelpittet’s picture

+++ b/core/modules/image/templates/image-widget.html.twig
@@ -0,0 +1,25 @@
+  {% if data.preview %}
+    <div class="image-preview">
+      {{ data.preview }}
...
+    {# Render widget data without the image preview that was output already. #}
+    {{ data|without('preview') }}

Awesome:)

mondrake’s picture

FileSize
44.76 KB
857 bytes

One more thing. When there is no image uploaded yet in the widget, a aria-describedby="edit-field-image-0--description" attribute leaks into the image_widget div container. That's again because we are loading $element['#attributes'] in the attributes variable. In theme_image_widget() there's no evidence of needing to do this - it straight renders the class attribute. This patch adjusts to the original behaviour.

star-szr’s picture

Cool, the only thing is currently Attribute is automatically instantiated for 'attributes', 'content_attributes', and 'title_attributes', see #2108771: Remove special cased title_attributes and content_attributes for Attribute creation

mondrake’s picture

FileSize
44.71 KB
811 bytes

Ah ok didn't know that, thanks @Cottser

Status: Needs review » Needs work

The last submitted patch, 219: 1898420-twig-image-219.patch, failed testing.

mondrake’s picture

219: 1898420-twig-image-219.patch queued for re-testing.

The last submitted patch, 219: 1898420-twig-image-219.patch, failed testing.

joelpittet’s picture

I think head is broken at the moment... wait to retest that for a bit.

star-szr’s picture

219: 1898420-twig-image-219.patch queued for re-testing.

mondrake’s picture

Status: Needs work » Needs review

Green.

mondrake’s picture

Status: Needs review » Needs work

The last submitted patch, 226: 1898420-twig-image-226.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review

Failure seems unrelated

mondrake’s picture

226: 1898420-twig-image-226.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 226: 1898420-twig-image-226.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review

226: 1898420-twig-image-226.patch queued for re-testing.

joelpittet’s picture

@mondrake Thanks for being so diligent with this patch. As you are aware it's tricky to find people to review larger patches and at the twig hangout a while ago we discussed splitting the larger patches to help them to be easier reviewed.

We did that with system.module and form.inc and it showed some promise. @see #1987410: [meta] system.module - Convert theme_ functions to Twig 1 left and 4 left in #1898480: [meta] form.inc - Convert theme_ functions to Twig

Some of these functions are easy to review together as a group, but maybe you'd know of a good line of division to split this patch up and help get more people's eyes on the review? My thought was by that used in column if that makes sense?

mondrake’s picture

@joelpittet - I was hoping @Cottser would finally RTBC this patch as a whole as he OKed image style UI and field formatter already in #195 and lately adjustments were only on the image field widget. I think this is overall pretty ready...

That said, yes, we could find ways to spin off subissues and make this a meta, but that would of course mean a lot more work and to some extend some steps back, and impact on time.

Maybe @Cottser can you share your opinion?

star-szr’s picture

I haven't had as much time as I'd like to review and thoroughly vet this issue for RTBC, but I'm still hoping to be able to do so for Austin (maybe on the way there :)). So I think if this issue is not RTBC by June 4 (second day of DrupalCon, two days ahead of the big sprint Friday) we should split it.

I want to do one more round of manual testing/markup comparison on everything and a bit of profiling for the forward-facing components. If anyone wants to help with those that would be cool too :)

joelpittet’s picture

Issue summary: View changes

Ok, I just added a column to the Issue Summary for now to keep track of which comment has the review for which function for now. Because this is a large patch just need some things to keep track.

I added the #195 to formatter but not sure which applies to the style UI. You can fill that in as you see fit. And anybody is welcome to help review all or any one function to help out @Cottser review. I may jump in but my attention is on the autoescape on by default issue ATM.

star-szr’s picture

I did profiling with 25 images on a node. I'm wondering if combining the image-style and image-formatter templates would help here (if that's even feasible).

The rest of the templates are backend only so don't need to be profiled.

=== 8.x..8.x compared (538e3ac0de109..538e3af7e977d):

ct  : 88,096|88,096|0|0.0%
wt  : 450,416|449,686|-730|-0.2%
cpu : 432,687|431,917|-770|-0.2%
mu  : 40,241,152|40,241,152|0|0.0%
pmu : 40,328,952|40,328,952|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=538e3ac0de109&...

=== 8.x..twig-image-1898420-226 compared (538e3ac0de109..538e3b37c1a66):

ct  : 88,096|89,441|1,345|1.5%
wt  : 450,416|454,703|4,287|1.0%
cpu : 432,687|436,389|3,702|0.9%
mu  : 40,241,152|40,348,320|107,168|0.3%
pmu : 40,328,952|40,447,344|118,392|0.3%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=538e3ac0de109&...

katiebot’s picture

Assigned: star-szr » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

This has been tested manually and is RTBC.

katiebot’s picture

Issue summary: View changes

Rather than link everything to my comment, I've removed the table as everything has been tested.

katiebot’s picture

Issue summary: View changes

Adding strikethrough to remaining tasks to indicate that they've been completed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 226: 1898420-twig-image-226.patch, failed testing.

joelpittet’s picture

Issue tags: +Novice, +Needs reroll
mondrake’s picture

Status: Needs work » Needs review
Issue tags: -Novice, -Needs reroll
FileSize
44.37 KB
mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • Commit 9f47a87 on 8.x by webchick:
    Issue #1898420 by mondrake, joelpittet, InternetDevels, claudiu.cristea...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.