Convert theme_tablesort_indicator() to Twig

new line at end of file

@hefox, nice work!

We may not really need a twig template for this one. Possibly:
keep your preprocess and replace:
theme('image', ...)
with
array('#theme' => 'image', ...)

Delete the twig file and the 'template' from the theme_hook.

That would be closer to ideal for me... but at the vary least we need to drop the theme() calls from this one. What do you think about dropping the twig file?

I admit I was feeling rather silly making a .html.twig with just '{{ image }}' in it.

How about this? Less work for when seven needs to change just the uri

@hefox, hell yeah! I would have not thought to use 'theme_hook_suggestions', does that have some benefits you would think over '#theme' => 'image'?

Not saying it's wrong, just haven't seen it used in the conversions yet, so I am curious, maybe I should be doing that as well in some cases.

Truefully, I haven't used 7/8 enough to say which is better. Did a dsm to see what was in $variables, saw that, and figured I'd try it and it worked XD

Let's see if this passes If so, it's what I'd do. Not sure about the t calls here.

I don't think there is any tests that test if the tablesort icon correctly shows

It doesn't show as far as I can see http://gyazo.com/ef2a4867b9e90694b83697e3d98511cb ; also tried it with '#theme' = 'image'. No error messages. I don't know how the details of how the theme system works in 7/8, but I suspect it's too late to do that in a preprocess cause it's theme( and not render(.

I also tried 'image' => array('#theme' ... but didn't work.

#9, yeah, you're probably right. In that case, your #5 looks pretty good.

Here is the before after for #5

Before:

After:

It's changed colours (look closely) as the override is needed in the seven theme #1938848: seven.theme - Convert PHPTemplate templates to Twig

I'll take it.

Add testing steps

Changes from #19 implemented. Does the @todo in the tablesort-indicator.html.twig also need to conform to the 80 character limit? If not, please review.

Changes from #22 implemented.

Wrapped comment in tablesort-indicator.html.twig. Please review.

Nitpick but, trailing whitespace was reintroduced in #24.

First line of:

+{# 
+  @todo Remove this and consolidate with 'image', see

Whitespace removed per #25. (What a newb, this 2ndMile guy...gees)

#26, I know I'm being picky, sorry!

+ * - src: URL to ascending or descending image.
+ * - attributes: Image attributes. Includes: width, height, alt, title.

The template uses attributes.src which is not documented here. Each property of attributes should be described on an indented new line - there's nothing official around that yet but pretty much all the other patches are doing that so it's kind of becoming a de facto standard.

Let me know if this is correct.

Implemented #30

rm snad

wow, this looks really great. I almost marked it RTBC but found one small gramatical error, so decided to reroll. While in there I also added a link to a new issue I created specifically to remove this sucker. bwahahahaha!

I added #1973418: Remove tablesort-indicator.html.twig, use CSS instead as an "after commit" issue on #1757550: [Meta] Convert core theme functions to Twig templates.

I'm going to review this now, hopefully mark it RTBC :)

Nitpick: Not even sure if it justifies a re-roll before RTBC considering we're planning to remove the whole template but:

+ */
+  @todo Remove this template and consolidate with 'image' as per
+  http://drupal.org/node/1973418
+#}

Shouldn't the @todo be in it's own comment or part of the @file block? it seems awkward to have it "floating" just below the */

About to manually test the markup...

Here's my manual check of the markup, confirming #33 but for patch #35:

before (link to ascending):

<img typeof="foaf:Image" src="http://localhost:8888/d8raw/core/misc/arrow-asc.png" width="13" height="13" alt="sort ascending" title="sort ascending" />

after (link to ascending):

<img typeof="foaf:Image" src="http://localhost:8888/d8html/core/misc/arrow-asc.png" width="13" height="13" alt="sort ascending" title="sort ascending" />

before (link to descending):

<img typeof="foaf:Image" src="http://localhost:8888/d8raw/core/misc/arrow-desc.png" width="13" height="13" alt="sort descending" title="sort descending" />

after (link to descending):

<img typeof="foaf:Image" src="http://localhost:8888/d8html/core/misc/arrow-desc.png" width="13" height="13" alt="sort descending" title="sort descending" />

Doesn't the change introduced in #33 enforce an RDF attribute even when the RDF module is disabled? I thought it would be preferable to pass a render array using #theme => image to the template, so that any registered image theme hooks would still be applied. If I'm wrong, please correct me because it will impact the work I'm done on #1898420: image.module - Convert theme_ functions to Twig.

Fixes the question from #39, and adds spaceless tags to ensure no trailing spaces after an inline element.

yeah I'll do it.

Just checking the "after" HTML since the "before" HTML is already in #38:

<img typeof="foaf:Image" src="http://localhost:8888/d8raw/core/misc/arrow-asc.png" width="13" height="13" alt="sort ascending" title="sort ascending" />

<img typeof="foaf:Image" src="http://localhost:8888/d8raw/core/misc/arrow-desc.png" width="13" height="13" alt="sort descending" title="sort descending" />

I'll profile this one.

Finished profiling:

=== 8.x..8.x compared (519fd0873c1d7..519fd0d6038b3):

ct  : 124,821|124,821|0|0.0%
wt  : 621,279|621,348|69|0.0%
cpu : 616,039|616,039|0|0.0%
mu  : 14,840,488|14,840,488|0|0.0%
pmu : 15,176,552|15,176,552|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fd0873c1d7&...

=== 8.x..1939090-twig-sort-indicator compared (519fd0873c1d7..519fd13122e91):

ct  : 124,821|124,847|26|0.0%
wt  : 621,279|620,227|-1,052|-0.2%
cpu : 616,039|616,038|-1|-0.0%
mu  : 14,840,488|14,828,400|-12,088|-0.1%
pmu : 15,176,552|15,177,448|896|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fd0873c1d7&...

Since this was RTBC prior to the profiling request, and since profiling shows a slight performance improvement, I'm going to go ahead and set this back to RTBC.

Hold on. I double-checked this patch by enabling twig_debug, and the template is not showing up. Trying to figure out what's happening...

Ok, my first profiling runs used Seven, where the tablesort indicator is overriden, and the twig template file is not used. I re-ran the numbers, this time using Stark and making sure that the tablesort-indicator.html.twig template is used:

=== 8.x..8.x compared (519fe4bc90ab9..519fe53eaaaf2):

ct  : 130,464|130,464|0|0.0%
wt  : 646,158|644,671|-1,487|-0.2%
cpu : 640,039|640,041|2|0.0%
mu  : 15,265,824|15,265,824|0|0.0%
pmu : 15,571,320|15,571,320|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fe4bc90ab9&...

Run 519fe4bc90ab9 uploaded successfully for drupal-perf-mrbaileys.
Run 519fe58959c26 uploaded successfully for drupal-perf-mrbaileys.
=== 8.x..1939090-twig-sort-indicator compared (519fe4bc90ab9..519fe58959c26):

ct  : 130,464|130,550|86|0.1%
wt  : 646,158|646,620|462|0.1%
cpu : 640,039|640,040|1|0.0%
mu  : 15,265,824|15,327,384|61,560|0.4%
pmu : 15,571,320|15,650,000|78,680|0.5%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fe4bc90ab9&...

Great work @mr.baileys thanks for profiling this!
Seems to be within the margin of error.

Trying reroll.

Committed 9ee90ca and pushed to 8.x. Thanks!