field.module - Convert theme_ functions to Twig

Per #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions rather than templates. See #1898062: field.module - Convert PHPTemplate templates to Twig for template conversion.

Updated issue summary.

Updated issue summary.

Updated issue summary.

#2: 1987398-2.patch queued for re-testing.

(removed)

#2: 1987398-2.patch queued for re-testing.

I am unable to apply this patch to the current 8.x branch. Not profiling.

confirmed by testbot that this patch fails

A reroll of #2

#10 needs manual testing and steps to profile and steps to manual test. Tagging.

manual testing is a novice task

I manually tested the following 3 things:

A field with single value

A taxonomy term reference field in bartik

A field with two values

Changing status <3.

Updated issue summary.

Lets get some profiling of this change...

changing back to NR

I was going to profile this, but it needs a re-roll so I'm re-rolling :)

Re-roll of patch

Re-roll to remove the following lines from tests

+ $this->installSchema('user', array('role_permission'));

role_permission table was dropped with the commit from https://drupal.org/node/1872876

Scenario:

- Set image field on Article to multiple value
- Generate 50 articles

=== 8.x..8.x compared (51ec623000e0c..51ec6582d0962):

ct  : 793,342|793,342|0|0.0%
wt  : 2,931,835|2,934,401|2,566|0.1%
cpu : 2,894,807|2,896,008|1,201|0.0%
mu  : 20,495,008|20,495,008|0|0.0%
pmu : 20,729,944|20,729,944|0|0.0%

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

=== 8.x..1987398-field.module compared (51ec623000e0c..51ec66a5a877d):

ct  : 793,342|797,488|4,146|0.5%
wt  : 2,931,835|2,934,185|2,350|0.1%
cpu : 2,894,807|2,898,250|3,443|0.1%
mu  : 20,495,008|20,574,496|79,488|0.4%
pmu : 20,729,944|20,802,744|72,800|0.4%

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

Re-rolled

#25: 1987398-25-twig-theme-field.patch queued for re-testing.

1. +++ b/core/themes/bartik/templates/field--taxonomy-term-reference.html.twig
   @@ -0,0 +1,32 @@
   + @todo Remove even/odd striping once http://drupal.org/node/1649780 lands.
   

   We can't have a @todo in here, if this is going to land then we should get that one going, but in the meantime I think we just remove the @todo.

2. +++ b/core/themes/bartik/templates/field--taxonomy-term-reference.html.twig
   @@ -0,0 +1,32 @@
   +      {# @todo remove type casting once http://drupal.org/node/1970960 lands #}
   

   This @todo and it's related variable to string cast ('' ~) should no longer be needed due to #1975462: Allow and test for NULL and integer 0 values in Twig templates.

Working on it.

Working on it.

Re-relloded the patch addressed all the comments from @joelpittet #28

Removed string cast ('' ~) #25

Thanks @adsw12 and nice to meet you at drupalcon prague.

This issue is ready for manual testing and that should be it for this one:)

+++ b/core/modules/field/templates/field-multiple-value-form.html.twig
@@ -0,0 +1,39 @@
+ * @see template_process_field()

+++ b/core/themes/bartik/templates/field--taxonomy-term-reference.html.twig
@@ -0,0 +1,30 @@
+ * @see template_process_field()

These don't exist either.

Re-roll of #32.

Edited documentation to address #34 and #35.

Setting to needs review

Just going to check that I get the same results as #23. Crossing fingers because that would be great:)

Ok bench is the same scenario as #23 and I used the patch from #36.

Looks great still. Just need #37 fixed up and provide an interdiff.

=== 8.x..8.x compared (5251f592ab279..5251f691abb85):

ct  : 182,060|182,060|0|0.0%
wt  : 725,809|727,727|1,918|0.3%
cpu : 665,838|675,383|9,545|1.4%
mu  : 20,484,360|20,484,360|0|0.0%
pmu : 20,716,112|20,716,112|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5251f592ab279&...

=== 8.x..1987398-twig-field compared (5251f592ab279..5251f6e1e32d5):

ct  : 182,060|185,223|3,163|1.7%
wt  : 725,809|727,234|1,425|0.2%
cpu : 665,838|667,528|1,690|0.3%
mu  : 20,484,360|20,585,640|101,280|0.5%
pmu : 20,716,112|20,853,792|137,680|0.7%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5251f592ab279&...

I took the missing files:

core/modules/field/templates/field-multiple-value-form.html.twig
core/themes/bartik/templates/field--taxonomy-term-reference.html.twig

from the patch on #36 and added them to the patch on #37.

errrgh

Okay

A few changes to the #43 patch.

As requested by @Cottser in #45:
Removed the @see template_process_field() lines mentioned by @joelpittet in #35.

Also, @Cottser mentioned in #34 that theme_field() is gone so I removed a line in the comment of field.html.twig and a sentence from a comment on line 694 of field.module that referenced theme_field().

errrgh

Third times the charm!

errrgh

Sorry for the spam! : /

@Tim Bozeman now worries for the spam, thank you very much for doing those changes:)

There has been manual testing in #14 but we should probably do another round just to be safe we didn't make any mistakes. @Tim Bozeman you're welcome to give that a try too or someone else can pick this issue up from here.

Added to test section :).

trying to clarify and update cause refereneced issues are closed and such

#53 was stale, rerolled.

@joelpittet, could you double check this this? Thanks!

Looking:)

Reviewed the markup from #56

The following templates were rendered onto the article page I tested:

core/modules/field/templates/field.html.twig
core/modules/field/templates/field-multiple-value-form.html.twig
core/themes/bartik/templates/field--taxonomy-term-reference.html.twig

Other than whitepace differences there was issues.
The two files uploaded were used for comparison, the modifications were to the hash keys and whitespace to make the diffs easier to parse.

Thanks @Tim Bozeman for finishing that up.
Profiling done in #41 is still good, no changes to speak of that would make a difference between #36 and #56

Changes to the FieldDefinitionInterface and the placing of '#cardinality_multiple' as a key in $variables broke the old patch. The patch also had snippets of $output being constructed so I've stripped those too.

Missed out a line from #62, oops. Between the patch on 2013-10-22 and now CARDINALITY_UNLIMITED was moved to the FieldDefinitionInterface and a '#cardinality_multiple' key has been added to $element alongside cardinality.

65: drupal8.field-system.1987398-65.patch queued for re-testing.

65: drupal8.field-system.1987398-65.patch queued for re-testing.

Since the last reroll the format of the taxonomy field has changed within Bartik - introduced bartik_preprocess_field to conditionally apply .visually-hidden - this lets screen readers still read the label even when it's hidden.

70: drupal8.field-system.1987398-70.patch queued for re-testing.

Same patch that passed testing above (first fail was due to broken HEAD) but with some comment errors corrected.

@Nigel_S thanks for the patches, great to see them green. Here's a little nit-pick doc change as well. And I think this is ready for manual testing again.

+++ b/core/themes/bartik/templates/field--taxonomy-term-reference.html.twig
@@ -0,0 +1,28 @@
+ * - title_attributes: HTML attributes for the title.

This is not being used. we should remove it.

@joelpittet - thanks :) have removed that line of the comment - everything else unchanged

75: drupal8.field-system.1987398-75.patch queued for re-testing.

Oh alright :P apparently I also killed some whitespace

75: drupal8.field-system.1987398-75.patch queued for re-testing.

Sorry for the re-test spam; it seems testbots are dying due to out-of-memory errors - will hold off on re-testing again until https://drupal.org/node/2141501 is dealt with.

75: drupal8.field-system.1987398-75.patch queued for re-testing.

+++ b/core/modules/field/templates/field-multiple-value-form.html.twig
@@ -0,0 +1,38 @@
+{% if multiple_cardinality %}

Could we just use "multiple"?

Haven't tested but great patch, looks good.

@Jeff Burnz that sounds like a slightly easier to understand variable name. Cardinality is a $100 word for a 5cent concept. Let's make that change.

OK :) These changes switch the variable to 'multiple' - I also noticed when looking at it that check_plain is now deprecated so I've switched template_preprocess_field to a direct call to String::checkPlain.

OK, done :)

@Nigel_S wanna grab the re-roll on this one?

89: drupal8.field-system.1987398-89.patch queued for re-testing.

Yeah sure, I shall take a look at it

Re-rolling

Re-rolling end fix with respect to comment #96

The reason for the failure became #732022: drupal_add_tabledrag() still using drupal_add_(js|library)(), should return array for #attached after the patch which drupal_add_tabledrab function was not valid. In this patch fixes bugs and made ​​reroll.

Reroll

103: drupal-field_module_convert_theme_functions_to_twig-1987398-103.patch queued for re-testing.

#103 was stale, rerolled.

106: drupal-field_module_convert_theme_functions_to_twig-1987398-106.patch queued for re-testing.

Couple little things and I'll do another round of manual testing.

1. +++ b/core/modules/field/field.form.inc
   @@ -69,8 +84,8 @@ function theme_field_multiple_value_form($variables) {
   +      '#theme' => 'table',
   

   This should say #type=>table. Just realized that. It may need to be re-worked to be a proper type table without #rows, but I'll leave that out of this conversion.

2. +++ b/core/themes/bartik/bartik.theme
   @@ -175,25 +176,20 @@ function bartik_menu_tree__shortcut_default($variables) {
   + * @todo Use bartik_theme_prepare_field__taxonomy_term_reference when
   + * https://drupal.org/node/2035055 is resolved
   

   Can you remove this @todo. I'd rather not have that in with core.

Here's a revised patch with the suggested changes. I also fixed a deprecated function, element_children. Sorry if that's out of scope.

@mdrummond Thanks for the revised patch! I'm fine with those changes, there is probably another issue outside this one dealing with it, which will cause that to be re-rolled or may be closed as duplicate, but same thing for String::checkPlain(). It's either this gets in first or it gets re-rolled.

+++ b/core/modules/field/field.form.inc
@@ -85,19 +101,15 @@ function theme_field_multiple_value_form($variables) {
+    foreach (Element::children($element) as $key) {
+      $variables['elements'][] = $element[$key];
     }

+++ b/core/modules/field/templates/field-multiple-value-form.html.twig
@@ -0,0 +1,38 @@
+  {% for element in elements %}
+    {{ element }}
+  {% endfor %}

Also love this, makes things much more useful in the template. No drupal_render_children() FTW!

+  if ($variables['multiple']) {
 
-  if ($element['#cardinality_multiple']) {
-    $form_required_marker = array('#theme' => 'form_required_marker');

This extra space below the "if" makes me feel weird in an OCD way. Can we remove it?

-        'data' => '<h4 class="label">' . t('!title !required', array('!title' => $element['#title'], '!required' => $required)) . "</h4>",
+        'data' => array(
+          '#prefix' => '<h4 class="label">',
+          'title' => array(
+            '#markup' => $element['#title'],
+          ),
+          '#suffix' => '</h4>',
+        ),

Is there a reason we no longer run #title through t()? I can see why required is not, as it's just a '*' character.

-  $output .= ($variables['element']['#label_display'] == 'inline') ? '<ul class="links inline">' : '<ul class="links">';

This looks like the conditional "inline" class would be broken now in the bartik template.

#2094585: [policy, no patch] Core review bonus for #2181507: Remove the deprecated usages of element_* methods.

Here's a reroll that also addresses the concerns raised in #114.

Some nit picky notes:

1. +++ b/core/modules/field/field.form.inc
   @@ -85,19 +99,15 @@ function theme_field_multiple_value_form($variables) {
   +    $variables['description'] = $element['#description'] ? $element['#description'] : '';
   

   This can likely be just
   $variables['description'] = $element['#description'];
   The condition doesn't help the statement any.

2. +++ b/core/modules/field/field.module
   @@ -7,8 +7,9 @@
   -use Drupal\Core\Template\Attribute;
   ...
   +use Drupal\Core\Template\Attribute;
   +use Drupal\Component\Utility\String;
   

   The Attribute order doesn't need to change here they were in the right order and Component\String should go before Xss.

3. +++ b/core/modules/field/templates/field-multiple-value-form.html.twig
   @@ -0,0 +1,38 @@
   +    {% spaceless %}
   ...
   +    {% endspaceless %}
   

   Any chance we can do without the spaceless? I'd rather tweak the spaces in the tests if they fail.

116: drupal-field_module_convert_theme_functions_to_twig-1987398-116.patch queued for re-testing.

Rerolled and addressed Joel's comments in 117.

I checked @LewisNyman patch and works well. Just fixed some indentation and missing periods.

+++ b/core/modules/field/field.form.inc
@@ -7,46 +7,55 @@
-        'data' => '<h4 class="label">' . t('!title !required', array('!title' => $element['#title'], '!required' => $required)) . "</h4>",
+        'data' => array(
+          '#prefix' => '<h4 class="label">',
+          'title' => array(
+            '#markup' => t($element['#title']),
+          ),
+          '#suffix' => '</h4>',
+        ),

This is just reformatting existing code, so not a commit-blocker. But I'm wondering why we don't move that <h4> and other associated markup to field-multiple-value-form.html.twig? Seems unfortunate to have to override a preprocess function in order to get at that, when everything else is so nicely "Twiggy."

According to Cottser, this is because we're putting things in the format that #type table expects. Hm. Still seems unfortunate, but also seems to be the status quo, so...

Committed and pushed to 8.x. Thanks!

To help further explain: it's used to wrap the title in the table header. I am guessing this was to get a desired font size and indicate some importance for this header. The table is rendered before being sent to the template and only provided as a single {{ table }} variable in the template to be printed. It is next to impossible to inject this small bit of markup in this particular template.

I agree though, using #markup, #prefix and #suffix is definitely not the best approach or Best Practices™ anymore. A better approach would be to simply add a class for that header and then styling it via CSS if it needs to look "bigger" (similar to an h4).

FWIW, this was a "convert" issue. We can create a follow-up "consolidate/cleanup" issue for field.module if it is really warranted.