color.module - Convert theme_ functions to Twig

+++ b/core/modules/color/color.moduleundefined
@@ -244,31 +245,25 @@ function theme_color_scheme_form($variables) {
+  $variables['scheme'] = drupal_render($form['scheme']);
...
+  $variables['children'] = drupal_render_children($form);
...
+  $variables['html_preview'] = file_get_contents($preview_html_path);

The drupal_render is not needed.

The drupal_render_children() might not be needed, might need to add a:

#theme => 'children'

currently discussing within #drupal-twig.

---

The last one is fine, because it is always printed and big part of the template.

The $form has an empty #children element, in Bartik at least.

Should we be using:

$variables['children'] = drupal_render_children($form, '#children');

Or can we remove children from the template?

Thanks for your work on this, @taslett!

Some newly created documentation here on Twig preprocess best practices: http://drupal.org/node/1920746

As @Fabianx mentioned, you should be able to just remove those two three drupal_render() calls, we're working on the best way to deal with drupal_render_children() in #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead so that can be left for now.

+++ b/core/modules/color/color.moduleundefined
@@ -244,31 +245,25 @@ function theme_color_scheme_form($variables) {
-  // @todo Transform to add library.

This @todo should probably stay unless we have a reason for removing it.

One other thing I noticed, color-scheme-form.html.twig needs a newline at the end of the file, spotted with Dreditor.

Thanks Cottser,
I have re-added the comment, fixed the drupal_render_children call, and added the newline to the twig template file.

Great, thanks @taslett! I see a couple more minor tweaks that can be done.

+++ b/core/modules/color/color.moduleundefined
@@ -236,39 +237,34 @@ function color_scheme_form($complete_form, &$form_state, $theme) {
-
+  ¶

Some trailing whitespace was introduced here in the latest patch. If you can configure your editor/IDE to automatically remove these it'll make your life much easier :)

+++ b/core/modules/color/templates/color-scheme-form.html.twigundefined
@@ -0,0 +1,29 @@
+ * - colors: An array of colors

I didn't notice this one before, this comment should end in a period per http://drupal.org/node/1354#drupal.

Thanks for your work on this taslett! A few more things:

1) We need to remove all mention of data types in the docs at the top of template files. Anyone working in these files doesn't need to know if something is an object, array, or string anymore, all they need to do is know it's name and what it is, so they can print it.

 * - colors: An array of colors.

2) Giving people a variable named 'rows' also doesn't help them understand what's inside. Let's name the variable in a way where they understand what is being printed. The docs at the top of the template call these colors, so let's stick with that :)

Here's a go at an updated template file with more verbose docs:

{#
/**
 * @file
 * Default theme implementation for a theme's color form.
 *
 * Available variables:
 * - scheme: A color scheme form element. For example: a select element with
 *     color theme presets, or a color picker widget.
 * - colors: Colors that can be changed by entering in a new hex value.
 * - children: Children elements of the form that still need to be printed.
 * - html_preview: A HTML preview of the theme's current color scheme.
 *
 * @see template_preprocess()
 * @see template_preprocess_color_scheme_form()
 *
 * @ingroup themeable
 */
#}
<div class="color-form clearfix">
  {{ scheme }}
  <div id="palette" class="clearfix">
    {% for color in colors %}
      {{ color }}
    {% endfor %}
  </div>
  {{ children }}
  <h2>{{ 'Preview'|t }}</h2>
  {{ html_preview }}
</div>

I also ran into some problems related to #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead that was causing the whole form to be rendered inside itself (though not infinitely). I've made some changes in the preprocess function to stop this, but it will need to be reworked later. (Marked clearly with @todos)

Mostly docs updates here. Reformatted preprocess docs per the pending standards and removed a few comments from the preprocess function, same idea as the "Set the template variables" comments here http://drupal.org/node/1898428#comment-7106282.

#14: 1898052-14.patch queued for re-testing.

#14: 1898052-14.patch queued for re-testing.

added a To Test section

Added a commit message to the issue summary, I wasn't able to find any issues in the sandbox and git blame says credit goes to @jenlampton on this one. If I'm missing anyone, please edit.

This one looks great to me! Thanks for the docs cleanup @Cottser :)

+1 for RTBC, please get this in.

#14: 1898052-14.patch queued for re-testing.

#14: 1898052-14.patch queued for re-testing.

Add commit message

Add conversion summary table

#14: 1898052-14.patch queued for re-testing.

Remove sandbox link

--- Edit : bad scenario, will redo it.

Scenario

1 - Put admin/appearance/settings/bartik as homepage
2 - Gives anonymous permission to administer theme settings

Settings

Twig cache on
APC class loader on

Run 519fe6587b366 uploaded successfully for drupal-perf-drupalcon.
Run 519fe7e2d7c63 uploaded successfully for drupal-perf-drupalcon.
=== 8.x..8.x compared (519fe6587b366..519fe7e2d7c63):

ct  : 34,245|34,245|0|0.0%
wt  : 106,644|103,555|-3,089|-2.9%
cpu : 104,000|100,000|-4,000|-3.8%
mu  : 10,875,528|10,875,528|0|0.0%
pmu : 11,060,376|11,060,376|0|0.0%

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

Run 519fe6587b366 uploaded successfully for drupal-perf-drupalcon.
Run 519fe82fef731 uploaded successfully for drupal-perf-drupalcon.                                        
=== 8.x..1898052-14.patch compared (519fe6587b366..519fe82fef731):                                        

ct  : 34,245|34,384|139|0.4%
wt  : 106,644|102,437|-4,207|-3.9%
cpu : 104,000|100,000|-4,000|-3.8%                                                                        
mu  : 10,875,528|10,937,896|62,368|0.6%                                                                   
pmu : 11,060,376|11,122,272|61,896|0.6%                                                                   

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

Added reference to profiling tag to Remaining section

+++ b/core/modules/color/color.moduleundefined
@@ -233,17 +234,20 @@ function color_scheme_form($complete_form, &$form_state, $theme) {
+  // @todo Remove/refactor pending http://drupal.org/node/1920886.
+  $children = $variables['form'];

This @todo needs to be addressed? #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead is marked fixed.

+++ b/core/modules/color/color.moduleundefined
@@ -254,26 +258,22 @@ function theme_color_scheme_form($variables) {
+  // @todo Refactor when http://drupal.org/node/1920886 is resolved.
+  unset($children['palette'], $children['scheme']);

Same

@tlattimore removed the drupal_render_children bit.

@Pierre Paul Lefebvre thank you for the profiling!

Updated attribution.

Simplified preprocess a bit more and reprofiled.

=== 8.x..8.x compared (51aad15cb89bb..51aad207381a1):

ct  : 36,798|36,798|0|0.0%
wt  : 331,800|332,623|823|0.2%
cpu : 307,021|307,497|476|0.2%
mu  : 28,251,856|28,251,856|0|0.0%
pmu : 28,438,160|28,438,160|0|0.0%

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

=== 8.x..color-1898052-31 compared (51aad15cb89bb..51aad231c9c46):

ct  : 36,798|36,965|167|0.5%
wt  : 331,800|332,067|267|0.1%
cpu : 307,021|308,525|1,504|0.5%
mu  : 28,251,856|28,324,944|73,088|0.3%
pmu : 28,438,160|28,511,552|73,392|0.3%

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

Did markup comparison again via visual diff and DaisyDiff and this still checks out.

I tried to do that on scheme and palette but it didn't seem to fly, I see you got one of them done, nice work:)

Don't know why I didn't think of this before.

Oops, two interdiffs.

It's late ok? :)

Re-ran the markup comparison, #36 still looks good. Here are more profiling results. Less function calls!

=== 8.x..8.x compared (51aaed0fcbd4e..51aaed63de9a0):

ct  : 36,798|36,798|0|0.0%
wt  : 331,812|332,398|586|0.2%
cpu : 309,167|309,002|-165|-0.1%
mu  : 28,251,856|28,251,856|0|0.0%
pmu : 28,438,160|28,438,160|0|0.0%

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

=== 8.x..color-1898052-36 compared (51aaed0fcbd4e..51aaed996e9e3):

ct  : 36,798|36,916|118|0.3%
wt  : 331,812|332,553|741|0.2%
cpu : 309,167|308,720|-447|-0.1%
mu  : 28,251,856|28,315,704|63,848|0.2%
pmu : 28,438,160|28,502,560|64,400|0.2%

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

Nice work Scott! I had a look at it and it works great.
Nitpicks in docs and I'd say this is RTBC.

+++ b/core/modules/color/templates/color-scheme-form.html.twig
@@ -0,0 +1,27 @@
+ *   - scheme: A color scheme form element. For example: a select element with
+ *   color theme presets, or a color picker widget.
+ * - html_preview: A HTML preview of the theme's current color scheme.

Formatting the second line should be indented by two more spaces no? Also need to add form.palette

Ha, well I guess I RTBC'd it already. Here's the patch:S You can re-RTBC if the comment looks good.

Yeah the indent was my mistake and I forgot to update the colors/palette docs, thanks @joelpittet! Attached patch makes one more docs tweak, most notably adding the trailing colon before a sublist per http://drupal.org/node/1354#lists.

And RTBC again.

Committed 1e0200d and pushed to 8.x. Thanks!