datetime.module - Convert theme_ functions to Twig

I'll take a stab at this.

I have a working patch. But still need to do some cleanup. Hope to post it later today.

It looks like I may not be able to get this done so I am going to un-assign myself.

Here is my 1st at converting datetime module to twig.

This patch removes brackets around title in datetime-wrapper. Still wrestling with php habits..

This holds up, good work @tlattimore.

A few minor tweaks may still be beneficial:

1.

 $required = !empty($element['#required']) ? theme('form_required_marker', array('element' => $element)) : '';
  if (!empty($element['#title'])) {
    $variables['title'] = t('!title!required', array('!title' => $element['#title'], '!required' => $required)) ;
  }

- convert this call to theme() into a renderable.
- call t() in the template.

2. wrap content of twig templates into {% spaceless %} tags.

working on it...

@Cottser,
{% spaceless %} is not needed - just thought I throw it in there for a more compact output. On second thought, the extra function call is probably not worth it. So thanks for questioning this.

Anyway, here's another patch addressing #11 (sans whitespace removal) and #12.

On a related note, I was not too stoked on the twig syntax for passing an assoc. array as function argument after I moved the translation call into the template

{{ '!title'|t({ '!title': title }) }}

IMO, this would be much easier to read in PHP code.

Is there a guideline on making these translation calls, template vs theme_preprocess function?
So far, I've been going by what I've seen, which is calling t() from within the template.

Thanks!

@Cottser, thanks for the quick answer.

Please see attached patch and interdiff for the changes requested in #17-19.

I also attached an alternative implementation that restores the original translation value but also removes the need to theme the marker within the preprocess function (see -alt files).

two more minor tweaks:

1. labeled "required" template variable as optional (as requested in comment #17)

+++ b/core/modules/datetime/templates/datetime-wrapper.html.twig
@@ -7,7 +7,7 @@
  * - content: The form element to be output, usually a datelist, or datetime.
  * - title: The title of the form element.
  * - attributes: Remaining HTML attributes for the form wrapper.
- * - required: A required marker indicating that the form element is required.
+ * - required: (optional) A required marker indicating that the form element is required.

2. in the alt. implementation, i moved the temp. variable assignment/required-rendering inside the if statement.

+++ b/core/modules/datetime/templates/datetime-wrapper.html.twig
@@ -15,8 +15,11 @@
  * @ingroup themeable
  */
 #}
-{% set required_rendered %}{{ required }}{% endset %}
+
 {% if title %}
+  {%- set required_rendered -%}
+    {{ required }}
+  {%- endset -%}
   <h4 class="label">
     {{ '!title!required'|t({ '!title': title, '!required': required_rendered }) }}
   </h4>

Add conversion summary table

+++ b/core/modules/datetime/datetime.moduleundefined
@@ -307,10 +310,9 @@ function datetime_date_default_time($date) {
- * Returns HTML for a HTML5-compatible #datetime form element.
+ * Prepares variables for #datetime form element templates.
...
- * Wrapper around the date element type which creates a date and a time
- * component for a date.

@@ -361,27 +363,32 @@ function theme_datelist_form($variables) {
- * Returns HTML for a datetime form element.
+ * Prepares variables for datetime form element templates.

I'm not entirely sure the documentation was originally correct, because theme_datetime_form mentioned "wrapper" in its docblock, whereas theme_datetime_wrapper() ironically did not. But maybe it makes sense to not lose this comment line talking about "Wrapper around date element" which would help to distinguish and clarify the difference between the two preprocess comments, which in the current patch only differ by the presence of a '#' in front of the word datetime, which isn't very clear at all.

Alright. I salvaged the previously removed "wrapper" code comment and applied it to new comments. Hopefully this will provide more clarity on what these functions all are about.
I also changed any reference to the "#datetime" form element into "datetime" in the code comments throughout the script for consistency reasons.
Please review again, thanks!

Update remaining

Thanks @Cottser.
Improved the code comments as outlined above.

Per #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions, which are lower in priority than PHPTemplate conversion issues.

On node/add/article, the div-wrapper is still there. It doesn't have a class anymore though.

Removed drupal_render_children as per #1920886 and new Attribute() as per #1982024

Patch #34 still adds a div-wrapper on node/add/article, without a class

Indeed, div-wrapper is still there.

removed the extraneous div.

minor tweak to the patch in comment #38 - removed unwanted blank line. otherwise this is identical.

Manually checked, the html output is good. Ready for profiling.

patch in #39 passed my manual testing and daisydiff. Testscenario used as described in first post.

Did not profile, sorry, my mistake ;)

attempting to profile...

#39: 1963476-39.patch queued for re-testing.

Manually testing this one now.

i give up on profiling, can't seem to get any consistency in the baseline from xhprof at all.

someone else with a more stable environment please try, thanks.

HTML output matches.

diff attached.

save clash... unassigning widukind again.

Still needs profiling. Not something I'm going to try and attempt on a crusty little netbook.

I will attempt to profile now

bbranches 519ff5087988b 1963476-39.patch
Warning: Must specify directory location for XHProf runs. Trying /tmp as default. You can either pass the directory location as an argument to the constructor for XHProfRuns_Default() or set xhprof.output_dir ini param.
=== 8.x..8.x compared (519ff5087988b..519ff54646296):

ct : 40,386|40,386|0|0.0%
wt : 447,276|447,067|-209|-0.0%
cpu : 432,313|432,833|520|0.1%
mu : 40,759,592|40,759,592|0|0.0%
pmu : 40,892,112|40,892,112|0|0.0%

Profiler output

Warning: Must specify directory location for XHProf runs. Trying /tmp as default. You can either pass the directory location as an argument to the constructor for XHProfRuns_Default() or set xhprof.output_dir ini param.
=== 8.x..1963476-39.patch compared (519ff5087988b..519ff588c96fd):

ct : 40,386|40,386|0|0.0%
wt : 447,276|448,273|997|0.2%
cpu : 432,313|431,743|-570|-0.1%
mu : 40,759,592|40,758,456|-1,136|-0.0%
pmu : 40,892,112|40,890,624|-1,488|-0.0%

Profiler output

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

=== 8.x..8.x compared (51a0079b7e32a..51a008005daf0):

ct : 76,176|76,176|0|0.0%
wt : 659,364|661,572|2,208|0.3%
cpu : 632,898|633,364|466|0.1%
mu : 41,026,312|41,026,312|0|0.0%
pmu : 41,187,832|41,187,832|0|0.0%

Profiler output

=== 8.x..1963476-39.patch compared (51a0079b7e32a..51a0086001e4c):

ct : 76,176|76,491|315|0.4%
wt : 659,364|664,510|5,146|0.8%
cpu : 632,898|636,615|3,717|0.6%
mu : 41,026,312|41,139,800|113,488|0.3%
pmu : 41,187,832|41,301,000|113,168|0.3%

Profiler output

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

Add testing steps to summary

Seems to the correct patch is #39 so, i'm going to mark this back to "needs review"

Updated issue summary.

The patch needed a re-roll. I took a stab at it. Hopefully this will do the trick.

- Reroll conflict - Somethings missing here, this doesn't look right.

In file core/modules/datetime/datetime.module

  if (!empty($element['#title'])) {
    $variables['title'] = $element['#title'];
  }    

<<<<<<< HEAD
  if (!empty($element['#description'])) {
    $output .= '<div class="description">' . $element['#description'] . '</div>';
  }

  return $output;
=======
  $variables['content'] = $element['#children'];
>>>>>>> Applying patch from https://drupal.org/node/1963476#comment-7441282
}

/**
 * Expands a datetime element type into date and/or time elements.

Re-rolling again.

Please see #58 on re-roll issue

on it

rerolled, added description variable, removed @see template_preprocess. Needs another review :)

no no, that's not even the right patch...

The required variable needs to be defined, so I set it to NULL.

#67: 1963476-67-twig-datetime-theme.patch queued for re-testing.

Seems disabled attribute is getting passed to the wrapper attributes, so I had to add back in the wrapper attributes array init.

It looks like datetime-wrapper.html.twig and datelist-form.html.twig. are both being used in both the "Authoring Information" and the "Date field" while datetime-form.html.twig. does not seem to be used in either. What if we removed the datetime-form.html.twig.? The datetime-form.html.twig. is also identical to the datelist-form.html.twig.

#71: 1963476-71-twig-datetime-theme.patch queued for re-testing.

Good idea, but I'd prefer to remove datelist-form just for naming reasons. Taking a stab at that now.

oh yeah, status change.

dibs!

reviewing, I assume ;)

Patch worked! It succeeded testing with the steps below and overall no issues or problems were evident.

Testing steps

-Navigate to node/add/article, compare the markup for the 'Authored on' field in the 'Authoring information' section in the right sidebar. The wrapper should be rendered by datetime-wrapper.html.twig and the datetime element by datetime-form.html.twig.
-Add a date field to the article content type (select list widget) and navigate to node/add/article. Compare the markup for the date field you added. The wrapper should be rendered by datetime-wrapper.html.twig and the datelist element by datelist-form.html.twig.

Can we get a new profile as this patch has changed since it was done...

marking needs review, since it will need a profiling review too after that gets done, but the patch doesn't currently need work.

Scenario

Theme: stark
permissions: anonymous can "Administer content" and "Bypass content access control "
url: /node/add/article
Added a date field to the article type.

=== 8.x..8.x compared (51e30f7568a45..51e30feb01694):

ct  : 69,016|69,016|0|0.0%
wt  : 528,422|533,177|4,755|0.9%
cpu : 470,454|470,747|293|0.1%
mu  : 34,196,832|34,196,832|0|0.0%
pmu : 34,335,968|34,335,968|0|0.0%

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

=== 8.x..1963476-twig-datetime-theme compared (51e30f7568a45..51e3103d95a81):

ct  : 69,016|69,328|312|0.5%
wt  : 528,422|534,843|6,421|1.2%
cpu : 470,454|470,926|472|0.1%
mu  : 34,196,832|34,300,248|103,416|0.3%
pmu : 34,335,968|34,441,680|105,712|0.3%

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

Thanks @joelpittet for running the profile!

So - is that within acceptable limits?

@kattekrab I'd say yes, it's 0.3% (1.6ms) difference on the Wall Time between the two runs.

And it removes a theme_ function!

Committed 15d3012 and pushed to 8.x. Thanks!