Issue #1898420 by shanethehat, claudiu.cristea, mondrake, DamienMcKenna, c4rl, thedavidmeister, Albert Volkman: Image.module - Convert theme_ functions to Twig.

Task

Use Twig instead of PHPTemplate

Remaining

  • Patch needs review
  • Manual testing

Theme function name/template path Used in Conversion status
theme_image_anchor Admin UI converted
theme_image_crop_summary Admin UI converted
theme_image_formatter Field formatter converted
theme_image_resize_summary Admin UI converted
theme_image_rotate_summary Admin UI converted
theme_image_scale_summary Admin UI converted
theme_image_style Field widget/formatter converted
theme_image_style_effects Admin UI Will be converted to #type table in #1938910: Convert image theme tables to table #type
theme_image_style_list Admin UI Removed in #1788542: Use EntityFormController and EntityListController for image styles
theme_image_style_preview Admin UI converted
theme_image_widget Field widget converted

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-63] Convert core theme functions to Twig templates
#1938910: Convert image theme tables to table #type

Review Bonus

See #2094585: [meta] [experimental] Core review bonus.

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

Points: 1

Files: 
CommentFileSizeAuthor
#189 1898420-twig-image-189.patch44.28 KBmondrake
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,867 pass(es).
[ View ]
#189 interdiff_178-189.txt2.8 KBmondrake
#178 interdiff_177-178.txt771 bytesmondrake
#178 1898420-twig-image-178.patch44.32 KBmondrake
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,577 pass(es).
[ View ]

Comments

Issue tags:+Twig

Tagging

Assigned:Unassigned» thedavidmeister

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

Status:Active» Needs work
StatusFileSize
new8.11 KB
FAILED: [[SimpleTest]]: [MySQL] 52,567 pass(es), 5 fail(s), and 2 exception(s).
[ View ]

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.

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.

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.

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

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 :)

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 :)

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 :)

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.

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.

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 :)

Issue summary:View changes

Add conversion summary table

Issue tags:+Needs manual testing

Tagging.

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!

Assigned:Unassigned» shanethehat

Assigned:shanethehat» Unassigned
Status:Needs work» Needs review
StatusFileSize
new23.51 KB
FAILED: [[SimpleTest]]: [MySQL] 53,691 pass(es), 206 fail(s), and 67 exception(s).
[ View ]
new18.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.

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

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

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].

Assigned:Unassigned» shanethehat

Status:Needs work» Needs review
StatusFileSize
new5.23 KB
new24.35 KB
FAILED: [[SimpleTest]]: [MySQL] 53,923 pass(es), 266 fail(s), and 95 exception(s).
[ View ]

Hopefully this will appease the testbots

Assigned:shanethehat» Unassigned

Status:Needs review» Needs work

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

Assigned:Unassigned» shanethehat

@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?

@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.

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

Issue tags:-Novice

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

Status:Needs work» Needs review
StatusFileSize
new24.52 KB
FAILED: [[SimpleTest]]: [MySQL] 54,209 pass(es), 258 fail(s), and 162 exception(s).
[ View ]
new533 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.

Status:Needs work» Needs review
StatusFileSize
new31.19 KB
FAILED: [[SimpleTest]]: [MySQL] 54,715 pass(es), 15 fail(s), and 0 exception(s).
[ View ]
new13.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.

Wow. Keep on rocking!

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

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.

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

#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.

Status:Needs work» Needs review
StatusFileSize
new35.94 KB
FAILED: [[SimpleTest]]: [MySQL] 54,456 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
new8.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.

@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.

@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.

Status:Needs work» Needs review
StatusFileSize
new35.14 KB
FAILED: [[SimpleTest]]: [MySQL] 54,508 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new11.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.

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!

@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 :)

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

+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.

Status:Needs work» Needs review
StatusFileSize
new36.06 KB
FAILED: [[SimpleTest]]: [MySQL] 54,441 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new12.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.

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

Status:Needs work» Needs review
StatusFileSize
new32.73 KB
FAILED: [[SimpleTest]]: [MySQL] 54,481 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new8.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.

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.

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

Status:Needs work» Needs review
StatusFileSize
new32.79 KB
FAILED: [[SimpleTest]]: [MySQL] 54,740 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Re-roll.

Status:Needs review» Needs work

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

Assigned:shanethehat» Unassigned
Status:Needs work» Needs review
StatusFileSize
new1.15 KB
new32.89 KB
PASSED: [[SimpleTest]]: [MySQL] 57,007 pass(es).
[ View ]

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

Issue summary:View changes

Update for theme_image_style_list

Assigned:Unassigned» steveoliver

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

Issue summary:View changes

Updating conversion table and remaining

Title:Convert image module to Twigimage.module - Convert theme_ functions to Twig

Per #1757550-44: [META-63] 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.

Assigned:steveoliver» Unassigned

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

Status:Needs review» Needs work
StatusFileSize
new36.52 KB
new38.11 KB
new35.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.

Status:Needs work» Needs review

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

Status:Needs review» Needs work

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

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 :)

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

StatusFileSize
new23.65 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new31.27 KB
PASSED: [[SimpleTest]]: [MySQL] 56,049 pass(es).
[ View ]

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

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

profiling...

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]

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.

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

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

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

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&...

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).

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

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&...

Issue summary:View changes

Remove sandbox link

Not sure how to hit image-widget

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).

StatusFileSize
new31.03 KB
FAILED: [[SimpleTest]]: [MySQL] 56,094 pass(es), 41 fail(s), and 18 exception(s).
[ View ]

Rerolling

StatusFileSize
new1.63 KB
new31.16 KB
FAILED: [[SimpleTest]]: [MySQL] 56,046 pass(es), 42 fail(s), and 18 exception(s).
[ View ]

Sorry, had some sloppy ternary operators.

Status:Needs review» Needs work

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

Assigned:Unassigned» claudiu.cristea

I'm taking this.

Status:Needs work» Needs review
StatusFileSize
new5.39 KB
new34.56 KB
PASSED: [[SimpleTest]]: [MySQL] 57,057 pass(es).
[ View ]

Let's try again.

StatusFileSize
new10.18 KB
new36.17 KB
PASSED: [[SimpleTest]]: [MySQL] 56,924 pass(es).
[ View ]

image_style_preview theme cleanup.

Issue tags:+Needs profiling

tagging

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

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?

StatusFileSize
new278.46 KB
new277.74 KB
new470.12 KB

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

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...

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-63] Convert core theme functions to Twig templates for some background.

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

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

Status:Needs work» Needs review
StatusFileSize
new27.74 KB
new45.18 KB
PASSED: [[SimpleTest]]: [MySQL] 57,912 pass(es).
[ View ]

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.

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.

StatusFileSize
new10.46 KB
new45.54 KB
PASSED: [[SimpleTest]]: [MySQL] 58,360 pass(es).
[ View ]

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.

Status:Needs work» Needs review

Needs review, of course :)

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!

@Cottser, no problem!

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

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.

StatusFileSize
new4.65 KB
new45.71 KB
PASSED: [[SimpleTest]]: [MySQL] 58,430 pass(es).
[ View ]

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.

Status:Needs work» Needs review

Ah!

  • #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.

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 :)

@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 :)

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new45.66 KB
PASSED: [[SimpleTest]]: [MySQL] 57,908 pass(es).
[ View ]

Here we go.

RTBC or more reviews, please.

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

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue tags:+Core Review Bonus

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

Issue summary:View changes

Updated issue summary.

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

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

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.

Issue summary:View changes

Updated issue summary.

@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.

@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.

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

@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

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&...

@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

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

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

Issue summary:View changes

Updated issue summary.

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

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

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.

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

Tagging for reroll.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new47.71 KB
FAILED: [[SimpleTest]]: [MySQL] 63,540 pass(es), 60 fail(s), and 1 exception(s).
[ View ]

Here is the re-roll.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new39.73 KB
FAILED: [[SimpleTest]]: [MySQL] 62,934 pass(es), 39 fail(s), and 68 exception(s).
[ View ]

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.

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Issue tags:+Twig conversion

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

Status:Needs work» Needs review
StatusFileSize
new2.19 KB
new40.99 KB
PASSED: [[SimpleTest]]: [MySQL] 64,568 pass(es).
[ View ]

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.

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?

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...

@mondrake oh sorry, I reviewed the interdiff:S

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.

Assigned:mondrake» Unassigned
Status:Needs work» Needs review
StatusFileSize
new10.02 KB
new41.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,249 pass(es).
[ View ]

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.

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

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!

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

Assigned:Unassigned» mondrake
Issue summary:View changes

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

Issue summary:View changes

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 :)

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.

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

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

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.

Issue summary:View changes

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

StatusFileSize
new41.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,397 pass(es).
[ View ]

StatusFileSize
new42.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,740 pass(es), 21 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new9.34 KB
new45.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,769 pass(es).
[ View ]

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

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!

@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 :)

Issue summary:View changes

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.

Assigned:Unassigned» Cottser

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

StatusFileSize
new45.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,740 pass(es).
[ View ]
new532 bytes

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

StatusFileSize
new45.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,936 pass(es).
[ View ]
new1.06 KB

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.

Assigned:Cottser» 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 \Drupal\simpletest\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().

Assigned:Unassigned» mondrake

ok i'll take it on

Assigned:mondrake» Unassigned
Status:Needs work» Needs review
StatusFileSize
new44.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,524 pass(es), 18 fail(s), and 0 exception(s).
[ View ]
new15.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.

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.

Status:Needs work» Needs review
StatusFileSize
new45.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,600 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
new4.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.

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.

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

Status:Needs work» Needs review
Related issues:+#872206: Add possibility to not upscale for "Scale and crop" effect
StatusFileSize
new44.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new4.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 :)

StatusFileSize
new44.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,577 pass(es).
[ View ]
new771 bytes

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.

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.

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?

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

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

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

Status:Needs work» Needs review

green

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?

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 :))

StatusFileSize
new2.8 KB
new44.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,867 pass(es).
[ View ]

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.

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