Issue #2152225 by steveoliver, joelpittet, hussainweb, shanethehat, jenlampton, kpa, AnythonyR, EVIIILJ, kgoel, Cottser, dsdeiz, hanpersand: Convert theme_select() to Twig

Task

Convert theme_select() to a Twig template.

Remaining tasks

  • Patch
  • Patch review
  • Manual testing
  • Profiling

Steps to test

@todo

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Issue summary: View changes

Adding a commit message to the issue summary so the folks who already worked on #1898480: [meta] form.inc - Convert theme_ functions to Twig and in the Twig sandbox get credit.

JeroenT’s picture

Status: Active » Needs review
FileSize
2.2 KB

Converted theme_select to twig.

Patch attached.

Status: Needs review » Needs work

The last submitted patch, 2: drupal-convert_theme_select_to_twig-2152225-2.patch, failed testing.

JeroenT’s picture

Status: Needs work » Needs review
star-szr’s picture

star-szr’s picture

joelpittet’s picture

Here is a manual test of http://d8.dev/admin/config/people/accounts/display
There is no difference in the markup except whitespace.


joelpittet’s picture

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

Some nitpicks to take care of.

  1. +++ b/core/includes/form.inc
    @@ -812,7 +812,9 @@ function form_process_select($element) {
    + * Preprocesses variables for theme_select().
    

    This should be Prepares variables for select element templates.

  2. +++ b/core/includes/form.inc
    @@ -812,7 +812,9 @@ function form_process_select($element) {
    + * Default template: select.html.twig
    

    Needs a period.

  3. +++ b/core/modules/system/templates/select.html.twig
    @@ -0,0 +1,15 @@
    + * Available variables
    

    Needs a colon ':' at the end of this line.

  4. +++ b/core/modules/system/templates/select.html.twig
    @@ -0,0 +1,15 @@
    + * - options: HTML string of options.
    

    Don't write the variable types. This should be something like "The option element children." or something along those lines. https://drupal.org/node/1823416#datatypes

longwave’s picture

Status: Needs work » Needs review
FileSize
2.21 KB
1.1 KB
joelpittet’s picture

Issue tags: -Novice

Thanks @longwave! This is ready for profiling and then RTBC.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -needs profiling

Scenario:
admin/structure/types/manage/article/form-display
with a datetime field too.

=== 8.x..8.x compared (530ef14e56c65..530ef19752730):

ct  : 139,156|139,156|0|0.0%
wt  : 783,110|773,268|-9,842|-1.3%
cpu : 748,393|739,540|-8,853|-1.2%
mu  : 36,372,936|36,372,936|0|0.0%
pmu : 36,487,352|36,487,352|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=530ef14e56c65&...

=== 8.x..2152225-twig-theme_select compared (530ef14e56c65..530ef2275628d):

ct  : 139,156|139,675|519|0.4%
wt  : 783,110|776,100|-7,010|-0.9%
cpu : 748,393|741,035|-7,358|-1.0%
mu  : 36,372,936|36,401,200|28,264|0.1%
pmu : 36,487,352|36,516,040|28,688|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=530ef14e56c65&...

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.