Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Task
Use Twig instead of PHPTemplate
Remaining
Patch needs reviewManual testing
Steps to test
Testing the Image Style admin UI
- 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.
- 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.
- 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'
- 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).
- Add a 'Desaturate' effect.
- 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).
- Add a 'Resize' effect. The resulting form should have 'Width' and 'Height' elements. Enter Width 700, Height 500. Click 'Add effect'
- 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).
- Add a 'Rotate' effect. Enter Rotation Angle 45. Click 'Add effect'
- 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).
- Add a 'Scale' effect. Enter Width 1000, tick on 'Allow upscaling'. Click 'Add effect'
- 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)".
- Add a 'Scale and crop' effect. Enter Width 600, Height 400. Click 'Add effect'
- 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
- 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.
- Click on the gearbox icon right to the 'Image' field.
- From the appearing form, select 'mytest' (set in the test above) from the 'Preview image style' dropdown.
- Click the 'Update' button, then on the 'Save' button.
- Add a new node of type 'Article', uploading an image of your choice.
- The image preview on the node is formatted according to 'mytest' image style (templates: image-widget.html.twig and image-style.html.twig).
- Save the new Article. The original image is displayed with the 'Large (480x480)' image style.
Testing the Field formatter
- 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.
- Click on the gearbox icon right to the 'Image' field.
- From the appearing form, select 'mytest' (set in the test above) from the 'Image style' dropdown.
- Click the 'Update' button, then on the 'Save' button.
- View the article you created in the previous steps.
- 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)
- Link the displayed image to its file. Go to Structure->Content Types and click on 'Manage display' Article's operations dropbutton.
- Click on the gearbox icon right to the 'Image' field.
- From the appearing form, select 'File' from the 'Link image to' dropdown.
- Click the 'Update' button, then on the 'Save' button.
- View the article you created in the previous steps.
- 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.
Related
#1757550: [Meta] Convert core theme functions to Twig templates
#1938910: Convert image theme tables to table #type
Review Bonus
See #2094585: [policy, no patch] Core review bonus.
- https://drupal.org/node/828268#comment-7897745 +2
- https://drupal.org/node/854956#comment-7897855 +2
- tagged #113 -3
Points: 1
Comment | File | Size | Author |
---|---|---|---|
#242 | 1898420-twig-image-242.patch | 44.37 KB | mondrake |
Comments
Comment #1
c4rl CreditAttribution: c4rl commentedTagging
Comment #2
thedavidmeister CreditAttribution: thedavidmeister commentedI'd like to look into this myself unless there are any objections?
Comment #3
thedavidmeister CreditAttribution: thedavidmeister commentedtemplate_preprocess_image_formatter() is affected by #1812562: Create best practices about use of the l() function and the url() function in templates.
Comment #4
thedavidmeister CreditAttribution: thedavidmeister commentedwork-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.
Comment #5
star-szrSending to testbot so we can all see.
Comment #7
thedavidmeister CreditAttribution: thedavidmeister commentedLol, 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.
Comment #8
star-szr@thedavidmeister - To skip testbot altogether, you can end your patch filename with -do-not-test, see http://groups.drupal.org/node/213473.
Comment #9
thedavidmeister CreditAttribution: thedavidmeister commentedthanks 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 :)
Comment #10
star-szrI 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 :)
Comment #11
thedavidmeister CreditAttribution: thedavidmeister commentedyeah 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 :)
Comment #12
thedavidmeister CreditAttribution: thedavidmeister commentedThis 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.
Comment #13
star-szrThere'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.
Comment #14
thedavidmeister CreditAttribution: thedavidmeister commentedI 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 :)
Comment #14.0
star-szrAdd conversion summary table
Comment #15
star-szrTagging.
Comment #16
star-szrTagging 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!
Comment #17
shanethehat CreditAttribution: shanethehat commentedComment #18
shanethehat CreditAttribution: shanethehat commentedBrought over the conversions listed in the summary
Comment #20
shanethehat CreditAttribution: shanethehat commented#4: 1898420-twig-for-images-4.patch queued for re-testing.
Comment #21
thedavidmeister CreditAttribution: thedavidmeister commented@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].
Comment #22
shanethehat CreditAttribution: shanethehat commentedComment #23
shanethehat CreditAttribution: shanethehat commentedHopefully this will appease the testbots
Comment #24
shanethehat CreditAttribution: shanethehat commentedComment #26
shanethehat CreditAttribution: shanethehat commentedComment #27
thedavidmeister CreditAttribution: thedavidmeister commented@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?
Comment #28
shanethehat CreditAttribution: shanethehat commented@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.
Comment #29
thedavidmeister CreditAttribution: thedavidmeister commented#28 - Maybe your error reporting is not sensitive enough for the issues to appear on your local or something...
Comment #30
star-szrRemoving novice tag now that the conversions are over. @shanethehat - anything we can do to help?
Comment #31
shanethehat CreditAttribution: shanethehat commented@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.
Comment #33
shanethehat CreditAttribution: shanethehat commentedDown to 5 fails locally, let's see what the testbots say.
Comment #35
star-szrWow. Keep on rocking!
Edit: 15 is still much better than 250+ :D and no exceptions!
Comment #36
shanethehat CreditAttribution: shanethehat commentedThe 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.
Comment #37
shanethehat CreditAttribution: shanethehat commentedThe 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:
Comment #38
thedavidmeister CreditAttribution: thedavidmeister commented#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.
Comment #39
star-szr#1938430: Don't add a default theme hook class in template_preprocess() is worth a look. #1939068-42: Convert theme_image() to Twig has one example of removing the default class in preprocess, and #1898466-32: update.module - Convert theme_ functions to Twig has a different example.
Comment #40
shanethehat CreditAttribution: shanethehat commentedI'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.
Comment #41
star-szr@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.
Comment #42
shanethehat CreditAttribution: shanethehat commented@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?
Comment #44
shanethehat CreditAttribution: shanethehat commentedAfter 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.
Comment #45
star-szrThis 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 :)
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 :)
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.
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.
Let's leave this as 'A render element…' please, because that has more connotation than 'associative array' in this case.
Did you try removing this drupal_render() or did it break things?
url in the description should be URL; the variable should be kept lowercase.
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.
Remove extra 'H' and remove extra period at the end.
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.
Similarly here, can we just say something along the lines of 'HTML attributes for the containing element.'?
Keep rocking!
Comment #46
jenlampton@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 :)
Comment #47
star-szr@jenlampton: sounds good - just wanted to be sure we had considered it. Thanks!
Comment #48
thedavidmeister CreditAttribution: thedavidmeister commented+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.
Comment #49
shanethehat CreditAttribution: shanethehat commentedHopefully fixes all but the rdf fail. Also addresses feedback 2-10 from #45.
Comment #51
star-szrSo close! I haven't looked at the interdiff yet but keep up the great work :)
Comment #52
shanethehat CreditAttribution: shanethehat commentedReverting changes to tests to improve compatibility with #1939068: Convert theme_image() to Twig.
RDF test still expected to fail.
Comment #54
shanethehat CreditAttribution: shanethehat commented#52: twig-image-1898420-51.patch queued for re-testing.
Comment #56
tlattimore CreditAttribution: tlattimore commentedThis 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:
Comment #57
Albert Volkman CreditAttribution: Albert Volkman commentedRe-roll.
Comment #59
shanethehat CreditAttribution: shanethehat commentedFinally got to grips with the RDF fail. image_style needs to use theme_image so that the hook in the rdf module catches.
Comment #59.0
shanethehat CreditAttribution: shanethehat commentedUpdate for theme_image_style_list
Comment #60
star-szrAssigning to @steveoliver for review. Happy to see this green!
Comment #60.0
star-szrUpdating conversion table and remaining
Comment #61
c4rl CreditAttribution: c4rl commentedPer #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions, which are lower in priority than PHPTemplate conversion issues.
Comment #62
star-szrThis is lower priority since it's just dealing with theme_ functions so taking off @steveoliver's review list for now.
Comment #63
intergalactic overlords CreditAttribution: intergalactic overlords commentedMost 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.
Comment #64
artis CreditAttribution: artis commented#59: twig-image-1898420-59.patch queued for re-testing.
Comment #65
DamienMcKennaI've confirmed the problem with template_preprocess_image_style_preview() so am bumping this back to Needs Work.
Comment #66
DamienMcKennaI 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 :)
Comment #67
DamienMcKennaICBW but I believe this needs updating now that #1938430: Don't add a default theme hook class in template_preprocess() has been committed?
Comment #68
DamienMcKennaTrying 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.
Comment #69
DamienMcKennaOops, the last patch forgot to include all of the new twig files.
Comment #70
DamienMcKennaJust as a reminder, this dependent upon #1939068: Convert theme_image() to Twig.
Comment #71
ernie-g CreditAttribution: ernie-g commentedprofiling...
Comment #72
ernie-g CreditAttribution: ernie-g commentedComment #73
ernie-g CreditAttribution: ernie-g commentedMANY 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 :-/
Comment #75
ezeedub CreditAttribution: ezeedub commented#69: twig-n1898420-69.patch queued for re-testing.
Comment #76
ezeedub CreditAttribution: ezeedub commentedScenario #1: point site home to admin/config/media/image-styles/manage/medium/add/image_crop
hits core/modules/image/templates/image-anchor.html.twig
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a462112dd99&...
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...
Comment #77
ezeedub CreditAttribution: ezeedub commentedOn second thought, rebasing the branch mentioned in #70 was unnecessary. Here's another run of the scenario in #76 without it.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a4f50036185&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a4f50036185&...
Comment #78
ezeedub CreditAttribution: ezeedub commentedEdit 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:
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a518f009231&...
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).
Comment #79
ezeedub CreditAttribution: ezeedub commentedscenarios remaining:
Comment #80
ezeedub CreditAttribution: ezeedub commentedGenerate 30 articles, edit frontpage view to show full node, no limit.
covers image-formatter and image-style
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a5545d76a23&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a5545d76a23&...
Comment #80.0
ezeedub CreditAttribution: ezeedub commentedRemove sandbox link
Comment #81
ezeedub CreditAttribution: ezeedub commentedNot sure how to hit image-widget
Comment #82
jenlamptonimage-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).
Comment #83
c4rl CreditAttribution: c4rl commentedRerolling
Comment #84
c4rl CreditAttribution: c4rl commentedSorry, had some sloppy ternary operators.
Comment #86
claudiu.cristeaI'm taking this.
Comment #87
claudiu.cristea#2029649: Move entity_load('image_style', ...) in preprocess duplicates this.
Comment #88
claudiu.cristeaLet's try again.
Comment #89
claudiu.cristeaimage_style_preview
theme cleanup.Comment #90
jenlamptontagging
Comment #91
claudiu.cristeaProfiling Twig patch docs: https://drupal.org/node/1999108
Comment #92
claudiu.cristeaMy 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?
Comment #93
claudiu.cristeaDon't know if this helps. Just ran a JMeter test plan. See attached snapshots.
Comment #94
claudiu.cristeaHere'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...
Comment #95
star-szrThanks 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.
I don't think we need to use Attribute in either of these files any more.
s/Prepare/Prepares/ per https://drupal.org/node/1354#themepreprocess and the general function documentation standards: http://drupal.org/node/1354#functions
This comment needs to end in a period to be a complete sentence per http://drupal.org/node/1354#drupal.
Typo: s/varaibles/variables/
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.
Hmmm, I think "A render element" was better :)
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().
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.
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.
The height line needs a trailing comma per http://drupal.org/coding-standards#array.
These lines need to be removed, see #2013094: [policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file.
We really shouldn't be adding @todos at this stage, please remove this line. Hopefully it will get consolidated before release. See #1757550-38: [Meta] Convert core theme functions to Twig templates for some background.
Comment #96
markhalliwellAn 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
Comment #97
star-szrNow there's an idea, that would make #45.1 a little bit more reasonable.
Comment #98
claudiu.cristeaThank 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.Comment #99
star-szrSorry 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.
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.
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.
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 :)
Typo, with = width
Wording could be a bit clearer, maybe something like "The part of the image that will be retained after cropping"?
This description is a bit terse. Maybe "Degrees to rotate the image, positive values will rotate the image clockwise, negative values counter-clockwise."
I think "the exposed areas of the image" could be clearer, but I don't have any immediate suggestions on improvement.
If these preprocess functions are being removed, remove the @see as well… this applies for any preprocess functions being removed.
Comment #100
claudiu.cristeaOK. 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.
Comment #101
claudiu.cristeaNeeds review, of course :)
Comment #102
star-szrRegarding #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!
Comment #103
claudiu.cristea@Cottser, no problem!
It turned green again. Add your comments and let't kick this in :)
Comment #104
markhalliwellAs promised @claudiu.cristea, here is my review (sorry it's taken so long!):
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).
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.
?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.If we're no longer passing this through
l()
, then$options['html'] = TRUE;
should be removed (along with the comment above it).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.
Use
$this->getAnchorOptions()
instead of self to keep things consistent.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)
withdrupal_render($variables)
and remove this added method.Move these links below as additional
@see
links.If we're not going to expand the attributes in the doc, then we should just do
<div{{attributes}}
for the wrapper below.Comment #105
claudiu.cristeaThank you for review, @Mark Carver. I'm replying point by point:
$path = $variables['path']['path'];
-- it makes no sense that line when using that variable only once.self
,static
is better because is allowing the method extension.$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 todrupal_render()
. IMO this is more elegant way to code.Comment #106
claudiu.cristeaAh!
Comment #107
markhalliwellComment #108
star-szrThis now needs a reroll. Can you still work on this @claudiu.cristea? If not we have some sprinters who can :)
Comment #109
claudiu.cristea@Cottser,
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 :)
Comment #110
claudiu.cristeaHere we go.
Comment #111
claudiu.cristeaRTBC or more reviews, please.
Comment #112
star-szrJust asking because some people forget or lose interest. Thanks for being awesome :)
Comment #112.0
star-szrUpdated issue summary.
Comment #112.1
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #112.2
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #113
thedavidmeister CreditAttribution: thedavidmeister commentedTagging with "Core Review Bonus" #2094585: [policy, no patch] Core review bonus, to see if that helps get a review :)
Comment #113.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #114
claudiu.cristea@thedavidmeister, In fact this issue has been reviewed, not only once, but for some reasons it still not get RTBC :)
Comment #115
thedavidmeister CreditAttribution: thedavidmeister commentedWell, I'm considering marking it as RTBC a "review" because you have to review it to be sure it's RTBC.
Comment #116
markhalliwellIn 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.
Comment #116.0
markhalliwellUpdated issue summary.
Comment #117
claudiu.cristea@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.
Comment #118
markhalliwell@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.
Comment #119
claudiu.cristea@Mark Carver, I'm fine. Let's see profiling and reviews
Comment #120
joelpittet@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
Comment #121
joelpittetScenario 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
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=526a239923027&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=526a239923027&...
Comment #122
joelpittet@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
Comment #123
claudiu.cristea@joelpittet, OK. Use this link http://webikon.com/cases/drupal-twig-conversion-performance-test-with-jm.... That for Evernote was a temporary work.
Comment #124
joelpittetThanks, 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
Comment #124.0
joelpittetUpdated issue summary.
Comment #125
joelpittet110: twig-1898420-110.patch queued for re-testing.
Comment #126
chanderbhushan CreditAttribution: chanderbhushan commented110: twig-1898420-110.patch queued for re-testing.
Comment #127
markhalliwellStill needs manual testing and issue summary update. Patch still passes, willing to RTBC once both needs are met.
Comment #128
star-szrTagging for reroll.
Comment #129
InternetDevels CreditAttribution: InternetDevels commentedHere is the re-roll.
Comment #131
joelpittetRemoved " 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.
Comment #133
joelpittet131: 1898420-twig-image-131-reroll.patch queued for re-testing.
Comment #135
star-szrComment #136
star-szrComment #137
star-szrI think we can keep the helper function, it's just in service of the tests.
Comment #138
mondrakeI 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.
Comment #139
joelpittet@mondrake not at all, thank you very much for the reroll and fixes!
Is this change needed?
Comment #140
mondrake@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...
Comment #141
joelpittet@mondrake oh sorry, I reviewed the interdiff:S
Comment #142
mondrakeActually testing manually the image style preview, noticed that the cache bypass token is no longer displayed. Will work on a fix.
Comment #143
mondrakeSo, 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.
Comment #144
mondrake143: 1898420-twig-image-143.patch queued for re-testing.
Comment #145
star-szrAdding a placeholder for testing steps to the issue summary. Let's document how to test all these templates!
Comment #146
mondrake@Cottser can you point to a good example of such directions? Thanks
Comment #147
star-szrMany of the sub-issues of #1757550: [Meta] Convert core theme functions to Twig templates have these instructions in the issue summary. Here's a handful:
#1898442: responsive_image.module - Convert theme_ functions to Twig
#1898070: file.module - Convert theme_ functions to Twig
#1898416: filter.module - Convert theme_ functions to Twig
They are really helpful to reviewers, manual testers and profilers :)
Comment #148
mondrakeUpdating issue summary to indicate where templates are used, preparing to add manual testing steps.
Comment #149
mondrakeComment #150
mondrakeAdded 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 :)
Comment #151
mondrakeAdded 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.
Comment #152
mondrake143: 1898420-twig-image-143.patch queued for re-testing.
Comment #153
mondrake143: 1898420-twig-image-143.patch queued for re-testing.
Comment #154
star-szrThank 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.
Comment #155
mondrakePer #154, added reference to templates used in the testing steps. Hopefully rightly ;)
Comment #156
mondrakeRerolled after #2171071: Rename drupal_add_library() to _drupal_add_library() and remove its uses
Comment #157
mondrakeRerolled 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
Comment #159
mondrakeUhm. Not sure how to deal with the newline characters that drupal_render appends at the end of the formatted HTML. Any help welcome.
Comment #160
star-szrThat'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!
Comment #161
claudiu.cristea@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 :)
Comment #162
claudiu.cristeaComment #163
filijonka CreditAttribution: filijonka commentedHi
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.
Comment #164
star-szrThank you all, I'm going to sprinting at MidCamp starting tomorrow so putting this on my TODO list :)
Comment #165
mondrakeNeeded a reroll because of #2136197: Move field/instance/widget/formatter settings out of annotation / plugin definition. Just change in the patch context.
Comment #166
mondrakeOne more reroll, due to #2221755: Remove uses of deprecated Element functions.
Comment #167
star-szrThank 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.
Comment #168
star-szrIt'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 :)
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.
Extra blank line at the top of this function can be removed.
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.
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.
Remove @ingroup themeable, it should be only used for themeable output in Twig templates (and theme functions but we're getting rid of 'em).
I don't think this blank line is hurting anyone, let's leave it be :)
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.
This threw me off at first but I think it's just sending this extra variable to '#theme' => 'image' which is cool by me.
This makes a lot of sense but why does ResizeImageEffect use a different pattern? All the others are consistent with CropImageEffect.
I would leave this kind of change to a minor cleanup issue instead.
You might be interested in #2227569: Add a helper method to WebTestBase for checking the output of render arrays and deprecate assertThemeOutput() :)
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.
Just have to say, great use of built-in Twig filters for this (abs). Much cleaner than the theme function version IMO :)
I could do without these closing Twig comments, I don't think they are necessary. The template is pretty self-contained.
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().
Comment #169
mondrakeok i'll take it on
Comment #170
mondrakeNew 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.
Comment #172
star-szrThank 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:
Adding these is messing things up, we're getting additional "url" attributes, so I'd suggest reverting these changes.
Comment #173
mondrakeThanks @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.
Comment #174
star-szrThat makes sense too :)
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
Comment #176
star-szrAfter looking at the test fails I'm not sure the crop/resize/scale summary templates need to be changed at all.
Comment #177
mondrake#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 :)
Comment #178
mondrakeFound a glitch while testing manually - the images on the image style preview won't size correctly
Comment #180
markhalliwellI canceled #177 since there's no point in having two run :)
Comment #182
mondrakeFailure 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?
Comment #183
mondrake178: 1898420-twig-image-178.patch queued for re-testing.
Comment #185
mondrake178: 1898420-twig-image-178.patch queued for re-testing.
Comment #186
mondrakegreen
Comment #187
star-szrCool, looking good and green! One more question before I start to poke at some manual testing :)
The 'style' lines here look suspect, are we really passing inline styles here or is this actually image style data that is being added?
Comment #188
mondrakeNo, 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 :))
Comment #189
mondrakeRe #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.
Comment #190
mondrake189: 1898420-twig-image-189.patch queued for re-testing.
Comment #191
mondrake189: 1898420-twig-image-189.patch queued for re-testing.
Comment #192
star-szrPutting this back on my list to continue testing and poking at :)
Thanks for the last clarification and keeping this up so fastidiously @mondrake!
Comment #193
mondrakeRerolled after #2204159: (Responsive) Image styles do not add correct cache tags, nor do they invalidate cache tags upon flushing, solved merge conficts.
Comment #194
star-szrWorking on testing this tonight. I'm removing the suggested commit message, the Dreditor-generated one is fine.
Comment #195
star-szrThe image style UI and field formatter components look great from a manual testing/markup perspective. There's even a markup fix in the rotate preview, changing
°
to°
:DThere seems to be a bug with the field widget changes where the image is being displayed twice. I don't get this on 8.x, only with the patch. Screenshot:
Comment #196
mondrakeHm. That's because the 'data' variable contains the 'preview' object too, so it gets rendered both as 'preview' and then as 'data.preview'. Here's the quickest patch I could think of. Maybe there are more clever ways...
Comment #197
mondrakeComment #198
mondrakeThis seems simpler.
Comment #199
Jeff Burnz CreditAttribution: Jeff Burnz commentedIs this a viable case for without filter:
{{ data|without('preview') }}
As a general approach or pattern I am generally against unsetting stuff in preprocess unless it's critically necessary to do so.
Comment #200
mondrakeCool :) That's the clever way I was looking for... but could not find this filter anywhere, is there any doc available? It's not on Twig website.
So this implements #199, and ends up being the only change vs. #193. Tested manually and looks ok to me, @Cottser the last word.
Thanks @Jeff Burns
Comment #201
star-szrNice!
'without' is an additional filter that we're adding as part of the core Twig extension. Full docs to come on all those, in the meantime here are two relevant change records regarding |without:
https://drupal.org/node/2212845
https://drupal.org/node/2240005
Comment #202
star-szrMight be worth a small comment in the Twig template to explain why the preview is being filtered out via |without.
Comment #203
mondrakeLike this?
Comment #204
mondrake203: 1898420-twig-image-203.patch queued for re-testing.
Comment #205
star-szrYes, it's about time I come back to this! I will see how far I can get today.
Comment #206
star-szrIt seems we have a duplicate div/ID happening with the image widget template.
<div id="edit-field-image-0-upload-ajax-wrapper">
occurs twice with the patch. Looks to be inside image-widget.html.twig after image-style.html.twig, coming from the data variable again.Comment #207
mondrakeRender data without #prefix and #suffix too.
Comment #209
mondrake207: 1898420-twig-image-207.patch queued for re-testing.
Comment #210
mondrakeThis needs a better solution. Working on a patch.
Comment #211
star-szrThanks @mondrake!
Comment #212
mondrakeThe original
theme_image_widget()
is usingdrupal_render_children()
to get the widget sub-elements rendered, i.e. avoiding element properties (array keys starting with a '#'). This is why #203 is rendering#prefix
and#suffix
- moving the entire$element
to$variables['data']
takes the properties too. So here I changed to build$variables['data']
by looping throughElement::children($element)
so we only take the widget's subelements in. Since 'preview' is also one of the element's subelements, I also changed the preprocess and the template not to have a separate 'preview' variable, but to keep it in the 'data' variable and render from there.Comment #214
mondrake212: 1898420-twig-image-212.patch queued for re-testing.
Comment #215
star-szrOh cool that seems way more sensible! Back on my list for more reviewing and such. Thanks @mondrake :)
Comment #216
joelpittetAwesome:)
Comment #217
mondrakeOne more thing. When there is no image uploaded yet in the widget, a
aria-describedby="edit-field-image-0--description"
attribute leaks into the image_widget div container. That's again because we are loading $element['#attributes'] in the attributes variable. Intheme_image_widget()
there's no evidence of needing to do this - it straight renders the class attribute. This patch adjusts to the original behaviour.Comment #218
star-szrCool, the only thing is currently Attribute is automatically instantiated for 'attributes', 'content_attributes', and 'title_attributes', see #2108771: Remove special cased title_attributes and content_attributes for Attribute creation
Comment #219
mondrakeAh ok didn't know that, thanks @Cottser
Comment #221
mondrake219: 1898420-twig-image-219.patch queued for re-testing.
Comment #223
joelpittetI think head is broken at the moment... wait to retest that for a bit.
Comment #224
star-szr219: 1898420-twig-image-219.patch queued for re-testing.
Comment #225
mondrakeGreen.
Comment #226
mondrakeReroll for #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4
Comment #228
mondrakeFailure seems unrelated
Comment #229
mondrake226: 1898420-twig-image-226.patch queued for re-testing.
Comment #231
mondrake226: 1898420-twig-image-226.patch queued for re-testing.
Comment #232
joelpittet@mondrake Thanks for being so diligent with this patch. As you are aware it's tricky to find people to review larger patches and at the twig hangout a while ago we discussed splitting the larger patches to help them to be easier reviewed.
We did that with system.module and form.inc and it showed some promise. @see #1987410: [meta] system.module - Convert theme_ functions to Twig 1 left and 4 left in #1898480: [meta] form.inc - Convert theme_ functions to Twig
Some of these functions are easy to review together as a group, but maybe you'd know of a good line of division to split this patch up and help get more people's eyes on the review? My thought was by that used in column if that makes sense?
Comment #233
mondrake@joelpittet - I was hoping @Cottser would finally RTBC this patch as a whole as he OKed image style UI and field formatter already in #195 and lately adjustments were only on the image field widget. I think this is overall pretty ready...
That said, yes, we could find ways to spin off subissues and make this a meta, but that would of course mean a lot more work and to some extend some steps back, and impact on time.
Maybe @Cottser can you share your opinion?
Comment #234
star-szrI haven't had as much time as I'd like to review and thoroughly vet this issue for RTBC, but I'm still hoping to be able to do so for Austin (maybe on the way there :)). So I think if this issue is not RTBC by June 4 (second day of DrupalCon, two days ahead of the big sprint Friday) we should split it.
I want to do one more round of manual testing/markup comparison on everything and a bit of profiling for the forward-facing components. If anyone wants to help with those that would be cool too :)
Comment #235
joelpittetOk, I just added a column to the Issue Summary for now to keep track of which comment has the review for which function for now. Because this is a large patch just need some things to keep track.
I added the #195 to formatter but not sure which applies to the style UI. You can fill that in as you see fit. And anybody is welcome to help review all or any one function to help out @Cottser review. I may jump in but my attention is on the autoescape on by default issue ATM.
Comment #236
star-szrI did profiling with 25 images on a node. I'm wondering if combining the image-style and image-formatter templates would help here (if that's even feasible).
The rest of the templates are backend only so don't need to be profiled.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=538e3ac0de109&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=538e3ac0de109&...
Comment #237
katiebot CreditAttribution: katiebot commentedThis has been tested manually and is RTBC.
Comment #238
katiebot CreditAttribution: katiebot commentedRather than link everything to my comment, I've removed the table as everything has been tested.
Comment #239
katiebot CreditAttribution: katiebot commentedAdding strikethrough to remaining tasks to indicate that they've been completed.
Comment #241
joelpittetComment #242
mondrakeReroll needed after #2251223: ConfigurableImageEffectInterface should use ConfigurablePluginInterface and PluginFormInterface. Only merge conflicts resolution.
Comment #243
mondrakeBack to RTBC.
Comment #244
webchickCommitted and pushed to 8.x. Thanks!