field.module - Convert PHPTemplate templates to Twig

+++ b/core/modules/field/templates/field.html.twigundefined
@@ -0,0 +1,31 @@
+ * @see template_preprocess_field
+ * @see template_process_field

At a glance, all I see is that these should end in parens per http://drupal.org/node/1354#see. Other than that nitpick this is looking good!

After reviewing this patch again, assigning so I can roll a new patch to fix a few things relating to http://drupal.org/node/1920746 and documentation (including #6). Will post a new patch and interdiff within a couple days.

1. Added a docblock to field-multiple-value-form.html.twig as well as some other minor docs updates.
2. Converted to render arrays where feasible per http://drupal.org/node/1920746#render.

+++ b/core/modules/field/field.form.incundefined
@@ -67,21 +66,27 @@ function theme_field_multiple_value_form($variables) {
+    $variables['button'] = array();
+    if (isset($add_more_button)) {
+      $variables['button'] = $add_more_button;
+    }

This part might be a bit ugly, drupal_render() was in some cases hiding the fact that $add_more_button was not set at all. Improvements welcome!

Re-roll with the following changes:

- Updated preprocess docs per #1913208: [policy] Standardize template preprocess function documentation
- Fixed failing tests with multiform widgets. The problem was that the weight field element was added to both the field element column and the weight column, so it needs to be removed from the field element column.
- I've also condensed some of the code referenced in #9. The additional $add_more_button variable is unnecessary. Hopefully this is what you had in mind, Cottser :).

Great work @duellj, thanks! $variables['button'] looks much nicer now :)

+++ b/core/modules/field/field.form.incundefined
@@ -55,6 +57,7 @@ function template_preprocess_field_multiple_value_form(&$variables) {
+      unset($item['_weight']);

I think this should have a comment to explain why it's being unset here.

Added commit message based on git blame and looking at sandbox issues. Please update if I missed anyone.

Added comment message per #12

#15: 1898062-15-twig-field.patch queued for re-testing.

#15: 1898062-15-twig-field.patch queued for re-testing.

Rerolled patch #15 against HEAD, since it no longer applies cleanly.

Applied patch and reviewed. Output looks the same on comparison of tpl.php and html.twig files.

All I see are some minor theme() -> render and nitpicky docs issues:

+++ b/core/modules/field/field.api.php
@@ -996,7 +996,7 @@ function hook_field_attach_update(\Drupal\Core\Entity\EntityInterface $entity) {
 /**
  * Alter field_attach_preprocess() variables.
  *
- * This hook is invoked while preprocessing the field.tpl.php template file in
+ * This hook is invoked while preprocessing the field.html.twig template file in
  * field_attach_preprocess().
  *

Instead of "... field.html.twig", let's say "This hook is invoked while preprocessing field templates."

+++ b/core/modules/field/field.form.inc
@@ -8,25 +8,25 @@
 use Drupal\Component\Utility\NestedArray;
 
 /**
- * Returns HTML for an individual form element.
+ * Prepare variables for individual form element templates.
+ *
+ * Default template: field-multiple-value-form.html.twig.
  *
  * Combines multiple values into a table with drag-n-drop reordering.
  *
- * @param $variables
+ * @param array $variables
  *   An associative array containing:
  *   - element: A render element representing the form element.

Prepare*s*.

+++ b/core/modules/field/field.form.inc
@@ -8,25 +8,25 @@
+function template_preprocess_field_multiple_value_form(&$variables) {
   $element = $variables['element'];
-  $output = '';
 
-  if ($element['#cardinality'] > 1 || $element['#cardinality'] == FIELD_CARDINALITY_UNLIMITED) {
+  $variables['multiple_cardinality'] = $element['#cardinality'] > 1 || $element['#cardinality'] == FIELD_CARDINALITY_UNLIMITED;
+
+  if ($variables['multiple_cardinality']) {
     $table_id = drupal_html_id($element['#field_name'] . '_values');
     $order_class = $element['#field_name'] . '-delta-order';
+    // @todo Can we get rid of this theme() call?
     $required = !empty($element['#required']) ? theme('form_required_marker', $variables) : '';
 

Re: this @todo: Yeah, we can return a render array here.

+++ b/core/modules/field/field.module
@@ -1107,12 +1110,38 @@ function field_page_build(&$page) {
-/**
- * Theme preprocess function for theme_field() and field.tpl.php.
- *
- * @see theme_field()
- * @see field.tpl.php
- */
+ /**
+  * Prepares variables for field templates.
+  *
+  * Default template: field.html.twig.
+  *
+  * To override output, copy the "field.html.twig" from the templates directory
+  * to your theme's directory and customize it, just like customizing other
+  * Drupal templates such as page.html.twig or node.html.twig.
+  *
+  * For example, for a field named 'body' displayed on the 'article'
+  * content type, any of the following templates will override this default
+  * implementation. The first of these templates that exists is used:
+  * - field--body--article.html.twig
+  * - field--article.html.twig
+  * - field--body.html.twig
+  * - field.html.twig
+  *
+  * @param array $variables
+  *   An associative array containing:
+  *   - label_hidden: A boolean indicating whether to show or hide the field
+  *     label.
+  *   - title_attributes: A string containing the attributes for the title.
+  *   - label: The label for the field.
+  *   - content_attributes: A string containing the attributes for the content's
+  *     div.
+  *   - items: An array of field items.
+  *   - item_attributes: An array of attributes for each item.
+  *   - classes: A string containing the classes for the wrapping div.
+  *   - attributes: A string containing the attributes for the wrapping div.
+  *
+  * @see field.html.twig
+  */

Let's remove this last @see field.html.twig.

+++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.php
@@ -373,7 +373,7 @@ public function buildOptionsForm(&$form, &$form_state) {
       '#title' => t('Use field template'),
       '#type' => 'checkbox',
       '#default_value' => $this->options['field_api_classes'],
-      '#description' => t('If checked, field api classes will be added using field.tpl.php (or equivalent). This is not recommended unless your CSS depends upon these classes. If not checked, template will not be used.'),
+      '#description' => t('If checked, field api classes will be added using field.html.twig (or equivalent). This is not recommended unless your CSS depends upon these classes. If not checked, template will not be used.'),
       '#fieldset' => 'style_settings',
       '#weight' => 20,
     );

Change "will be added using field.tpl.php (or equivalent)." to "...will be added by field templates."

Attached patch makes changes from #24.

Re: this @todo: Yeah, we can return a render array here

How can we use a render array here, if the $required marker is used in the table header. Can table headers take render arrays now?

m'hmm -- http://api.drupal.org/api/drupal/core!includes!theme.inc/function/_theme...

Very cool, thanks @steveoliver. I've removed the call to drupal_render and converted the header to a render array.

#27: 1898062-27-twig-field.patch queued for re-testing.

#27: 1898062-27-twig-field.patch queued for re-testing.

more nits:
field.html.twig
- Twig docblocks should not mention variable type (string, array, etc)
- Variables in Twig docblock need to be correct
- Twig whitespace controllers {{- need to be removed.
field-multiple-value-form.html.twig
- for loop syntax is incorrect

  {% for element in elements %}
    {{ element }}
  {% endfor %}

- we should put the if/else logic on separate lines than the markup, but if that messes up the markup we can use the spaceless tag:

  {% spaceless %}
  {% if description %}
    <div class="description">{{ description }}</div>
  {% endif %}
  {% if button %}
    <div class="clearfix">{{ button }}</div>
  {% endif %}
  {% endspaceless %}

template_preprocess_field
- available variables in docblock should only list the ones that were there before preprocess added them.

template_preprocess_field_multiple_value_form
- looks awesome! :)

new patch!

#33: core-update_field_to_twig-1898062-33.patch queued for re-testing.

#33: core-update_field_to_twig-1898062-33.patch queued for re-testing.

Both @joelpittet and myself have been unable to reproduce these failing tests locally. I also tested with a table prefix like testbot does and the failing tests still passed. #20 passed and I don't see between #20 and #25 what would cause the failures exceptions.

Tests indeed are passing when using the web browser, however not with run-tests.sh. I'm suspecting some kind of leaking somewhere, but I'm too tired to debug now, and if we want to fix that, that should happen in a separate issue anyway.

Patch attached fixes both tests as there's a call to user_access in user_template_preprocess_default_variables_alter and the role_permission table didn't exist in these Drupal Webunit tests.

Thanks @swentel. I did some git bisect and determined that when #1932382: Use DrupalUnitTestBase for Field API tests. was committed, this issue started seeing those failures (but only through run-tests.sh as you said). I'm not sure what this patch is doing to throw those exceptions, but thank you very much for stepping in with the fix :)

Yes, seen this before. #1950684: Mock and protect $GLOBALS['user'] in DUTB tests should fix it in making it consistently fail.

The reason is that DUBT tests currently don't switch the global user object, so it is whatever you're running tests with. In the UI, very likely uid 1, with run-tests.sh it is uid 0. uid 1 means access checks are skipped and permissions for the given roles aren't loaded.

You're more than welcome to confirm that with the patch from that issue and with the interdiff from #41, it should fail consistently in the UI and CLI.

Adding commit message

Updated issue summary.

Tagging.

I'm going to test this today.

Creating a new "Article" node and comparing the markup of the default fields and a few custom fields, the "class" attribute has a trailing space that it did not previously for .field-label, .field-items and .field-item.

Other than that, I've gone over a bunch of different combinations of fields and compared the markup. I haven't thoroughly reviewed the code in the patch yet but the markup looks good :)

I'll update the issue description with steps for testing if anyone else wants to look but I'm removing the testing tag for now.

Add conversion summary table

Also updating the issue summary to reflect that theme_field() has been removed, not converted.

Added instructions for manual testing.

Unfortunately, while the markup looks fine (other than the extra space in class attributes) I think the patch needs more work :(

Why does template_process_field() still exist when theme_field() has been removed. If we keep it, at the least the documentation for it needs updating, if we remove it we have to merge the functionality into template_preprocess_field(). I vote we remove it as we have no need for two functions processing variables. This is what is there currently:

/**
 * Theme process function for theme_field() and field.tpl.php.
 *
 * @see theme_field()
 * @see field.tpl.php
 */
function template_process_field(&$variables, $hook) {
  static $default_attributes;
  // The default theme implementation is a function, so template_process() does
  // not automatically run, so we need to flatten the classes and attributes
  // here. For best performance, only instantiate Drupal\Core\Template\Attribute
  // when needed, and note that template_preprocess_field() does not initialize
  // the *_attributes variables.
  if (!isset($default_attributes)) {
    $default_attributes = new Attribute;
  }
  $variables['attributes'] = isset($variables['attributes']) ? new Attribute($variables['attributes']) : clone $default_attributes;
  $variables['title_attributes'] = isset($variables['title_attributes']) ? new Attribute($variables['title_attributes']) : clone($default_attributes);
  $variables['content_attributes'] = isset($variables['content_attributes']) ? new Attribute($variables['content_attributes']) : clone($default_attributes);
  foreach ($variables['items'] as $delta => $item) {
    $variables['item_attributes'][$delta] = isset($variables['item_attributes'][$delta]) ? new Attribute($variables['item_attributes'][$delta]) : clone($default_attributes);
  }
}

The extra space in the class attribute is caused by lines in the template that look like this:

<div class="field-label {{ title_attributes.class }}"{{ title_attributes }}>{{ label }}:</div>

When foo_attributes.class is empty we end up with extra whitespace. The "field-label", "field-items", "field-item" classes in field.html.twig all should be merged in as default classes in the preprocess function rather than hardcoded into the Twig template.

In field-multiple-value-form.html.twig:

* - button: "Add more" button.

Actually, it's an "Add another item" button.

In field-multiple-value-form.html.twig:

{% if multiple_cardinality %}
<div class="form-item">
  {{ table }}
  {% spaceless %}
  {% if description %}
    <div class="description">{{ description }}</div>
  {% endif %}
  {% if button %}
    <div class="clearfix">{{ button }}</div>
  {% endif %}
  {% endspaceless %}
</div>
{% else %}

Is <div> not supposed to be indented when it's within an {% if %}?

I'm done testing this for now.

Rolling a new patch based on #48. I'm afraid of what will happen with RDF when we remove the process function but we'll see what testbot will say :)

Unfortunately, bartik_field__taxonomy_term_reference() needs to be converted here too. It can't be done in #1938840: bartik.theme - Convert PHPTemplate templates to Twig without the change to field_theme().

This (proof of concept) patch just merges template_preprocess_field() and template_process_field(), uploading so others can see. Fails some RDF tests locally, haven't run any others. Will set to CNR when I upload the patch based on the other feedback in #48.

I'm leaving the hardcoded classes for now, I could go either way on those.

Updated docs and indent level though, including indenting the contents of {% spaceless %}.

CNW to add the Bartik conversion here.

theme_field() was removed not converted.

Update remaining

Converted taxonomy field override in Bartik. One question: there's a difference in whitespace now that the template is in twig, as the old theme function added no newlines. Is this something we need to enforce here, since it looks a lot better with newlines and indentation?

Assigning to @steveoliver for another review.

Add bartik function to conversion table

Here is just the .tpl.php conversion.

Update conversion table

Back to you @steveoliver.

+++ b/core/modules/field/field.module
@@ -1002,12 +1002,31 @@ function field_page_build(&$page) {
-/**
- * Theme preprocess function for theme_field() and field.tpl.php.
- *
- * @see theme_field()
- * @see field.tpl.php
- */
+ /**
+  * Prepares variables for field templates.
+  *
+  * Default template: field.html.twig.
+  *
+  * To override output, copy the "field.html.twig" from the templates directory
+  * to your theme's directory and customize it, just like customizing other
+  * Drupal templates such as page.html.twig or node.html.twig.
+  *
+  * For example, for a field named 'body' displayed on the 'article'
+  * content type, any of the following templates will override this default
+  * implementation. The first of these templates that exists is used:
+  * - field--body--article.html.twig
+  * - field--article.html.twig
+  * - field--body.html.twig
+  * - field.html.twig
+  *
+  * @param array $variables
+  *   An associative array containing:
+  *   - element: A render element representing the field.
+  *   - attributes: A string containing the attributes for the wrapping div.
+  *   - title_attributes: A string containing the attributes for the title.
+  *   - content_attributes: A string containing the attributes for the content's
+  *     div.
+  */
 function template_preprocess_field(&$variables, $hook) {
   $element = $variables['element'];

1. Let's remove these docs from preprocess and merge them with the docs that already exist for field.tpl.php/html.twig, *keeping* the "THIS FILE IS NOT USED..." notice.

--- /dev/null
+++ b/core/modules/field/templates/field.html.twig
@@ -0,0 +1,31 @@
+{#
+/**
+ * @file
+ * Default theme implementation for a field.
+ *
+ * Available variables:
+ * - attributes: HTML attributes for the containing element.
+ * - label_hidden: Whether to show the field label or not.
+ * - title_attributes: HTML attributes for the title.
+ * - label: The label for the field.
+ * - content_attributes: HTML attributes for the content.
+ * - items: List of all the field items.
+ * - item_attributes: HTML attributes for each item.
+ *
+ * @see template_preprocess_field()
+ * @see template_process_field()
+ *
+ * @ingroup themeable
+ */
+ @todo Remove even/odd striping once http://drupal.org/node/1649780 lands.
+#}
+<div class="{{ attributes.class }}"{{ attributes }}>
+  {% if not label_hidden %}
+    <div class="field-label {{ title_attributes.class }}"{{ title_attributes }}>{{ label }}:</div>
+  {% endif %}
+  <div class="field-items {{ content_attributes.class }}"{{ content_attributes }}>
+    {% for delta, item in items %}
+      <div class="field-item {{ cycle(["even", "odd"], delta) }} {{ item_attributes[delta].class }}"{{ item_attributes[delta] }}>{{ item }}</div>
+    {% endfor %}
+  </div>

2. Remove @todo.
3. I could be convinced otherwise (this /is/ an example template), but I think we want to just print {{ *attributes }}, and not call out .class on it's own.
4. 'field-label' and 'field-items' classes can be added in preprocess. Why don't we do that?
5. Are we sure "cycle(["even", "odd"], delta)" is right? I'd think it goes 'odd', 'even' ? (Needs manual testing) ;)

It's not in this patch, but

6. template_process_field() looks completely pointless. Actually it is. See #1843728: Remove template_process_field().

Also,

7. change @see template_process_field() to @see theme_field().

+++ b/core/modules/field/field.module
@@ -1157,13 +1158,13 @@ function template_process_field(&$variables, $hook) {
  *   - items: An array of field items.
- *   - item_attributes: An array of attributes for each item.
+ *   - item_attributes_list: An array of attributes for each item.

#65 seemed to fail because RDF module is expceting item_attributes... at least part of the bot failures.

That item_attributes_list change seems a bit like an API change, how about doing that after conversion lands, especially if that's what's breaking tests?

+  $variables['title_attributes'] = new Attribute(array('class' => array('field-label')));
+  $variables['content_attributes'] = new Attribute(array('class' => array('field-items')));

I think I'm missing something really obvious, but why are we overriding 'title_attributes' and 'content_attributes'? don't we mean to concatenate our class onto them rather than replace the existing, passed in attributes?

+++ b/core/modules/field/field.moduleundefined
@@ -1057,13 +1073,17 @@ function template_preprocess_field(&$variables, $hook) {
+  $variables['title_attributes'] = new Attribute(array('class' => array('field-label')));
+  $variables['content_attributes'] = new Attribute(array('class' => array('field-items')));
+
 }

Agreed, and since these are standard variables we don't need to instantiate attribute objects so it should be something like:

$variables['title_attributes']['class'][] = 'field-label';
$variables['content_attributes']['class'][] = 'field-items';

We can also lose the extra line before the function's closing brace.

Can you take another look at this one @steveoliver?

Passes manual testing and code review. Needs only profiling before RTBC.

profiling...

Added three fields to Article.

20 full nodes on home page, 8.x vs. #73:

baseline check:

=== 50-node-view..8.x compared (5198f7eab7067..5198f92a4c3fa):

ct  : 162,702|162,702|0|0.0%
wt  : 2,189,665|2,188,652|-1,013|-0.0%
cpu : 1,656,104|1,656,104|0|0.0%
mu  : 64,621,224|64,621,224|0|0.0%
pmu : 65,061,152|65,061,152|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5198f7eab7067&...

patch #73 comparison:

=== 50-node-view..8.x-1898062-73 compared (5198f7eab7067..5198fa286ed27):

ct  : 162,702|166,200|3,498|2.1%
wt  : 2,189,665|2,227,149|37,484|1.7%
cpu : 1,656,104|1,688,106|32,002|1.9%
mu  : 64,621,224|64,633,568|12,344|0.0%
pmu : 65,061,152|65,074,440|13,288|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5198f7eab7067&...

/node/add/article, 8.x vs. #73:

=== node-add-article..8.x compared (519a4774889dd..519a4887d3a25):

ct  : 71,984|71,984|0|0.0%
wt  : 3,763,071|18,944,908|15,181,837|403.4%
cpu : 900,056|904,057|4,001|0.4%
mu  : 67,051,296|67,052,984|1,688|0.0%
pmu : 67,189,464|67,190,968|1,504|0.0%

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

=== node-add-article..8.x-1898062-73 compared (519a4774889dd..519a496084867):

ct  : 71,984|72,170|186|0.3%.
wt  : 3,763,071|6,242,031|2,478,960|65.9%
cpu : 900,056|896,056|-4,000|-0.4%
mu  : 67,051,296|67,064,920|13,624|0.0%
pmu : 67,189,464|67,203,968|14,504|0.0%

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

The wall time fluctuation on node/add/article is discouraging -- what's up with that? I'm running a Core2 Duo with APC enabled...

Leaving tagged "Needs profiling" so someone else can take a look.

Scenario: 20 nodes with 3 fields, in a view. Cottser pointed out that the template is not used unless it's in your theme.. So, using stark, copied field.tpl.php to stark theme folder in 8.x branch, and field.html.twig to stark folder in feature branch, and commit.

Comparing 8.x (96b23549) to #73...

=== 8.x..8.x compared (519aaadf29b61..519aac051ded1):

ct  : 314,374|314,374|0|0.0%
wt  : 2,453,399|2,449,598|-3,801|-0.2%
cpu : 2,444,153|2,440,152|-4,001|-0.2%
mu  : 10,627,680|10,627,680|0|0.0%
pmu : 12,540,564|12,540,564|0|0.0%

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

=== 8.x..field-1898062-73 compared (519aaadf29b61..519aada7a0b07):

ct  : 314,374|344,025|29,651|9.4%
wt  : 2,453,399|2,645,131|191,732|7.8%
cpu : 2,444,153|2,636,165|192,012|7.9%
mu  : 10,627,680|10,655,736|28,056|0.3%
pmu : 12,540,564|12,563,644|23,080|0.2%

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

not sure if 7.8% is too much, so leaving needs profiling tag. I think the node/add/article test is not needed b/c it doesn't use the template?

Went over the result in #79 with Cottser, and discovered a) looks like there were 250 fields, not 20 * 4 (I think i devel generated without deleting prev content), and b) I made fields with unlimited cardinality (thinking more is better!).

Here's a new test with 40 nodes with 5 fields (including body), all cardinality = 1. Same branches as #79.

=== 8.x..8.x compared (519abb02c8da6..519abc60afda3):

ct  : 274,347|274,347|0|0.0%
wt  : 2,112,323|2,111,255|-1,068|-0.1%
cpu : 2,104,132|2,100,131|-4,001|-0.2%
mu  : 9,428,732|9,428,732|0|0.0%
pmu : 10,198,204|10,198,204|0|0.0%

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

=== 8.x..field-1898062-73 compared (519abb02c8da6..519abd5ae932a):

ct  : 274,347|295,348|21,001|7.7%
wt  : 2,112,323|2,249,733|137,410|6.5%
cpu : 2,104,132|2,240,141|136,009|6.5%
mu  : 9,428,732|9,461,104|32,372|0.3%
pmu : 10,198,204|10,223,972|25,768|0.3%

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

Still bad at 6.5%, but this is possibly okay for this template?

That's pretty bad. Can anyone explain what's going on here?

This is one of the only ones left for re-profiling, so the more data the better. Profile away!

…and @rcaracaus is having dinner with us so he is not profiling right now :)

Re-profiled with the scenario from #80:

=== field-8.x..field-8.x compared (519dd86a86681..519dd95192eac):

ct  : 228,188|228,188|0|0.0%
wt  : 815,132|816,287|1,155|0.1%
cpu : 786,358|786,821|463|0.1%
mu  : 16,785,120|16,785,120|0|0.0%
pmu : 17,734,840|17,734,840|0|0.0%

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

=== field-8.x..field-1898062-72 compared (519dd86a86681..519dd9ab8b74d):

ct  : 228,188|250,589|22,401|9.8%
wt  : 815,132|876,374|61,242|7.5%
cpu : 786,358|846,956|60,598|7.7%
mu  : 16,785,120|16,838,448|53,328|0.3%
pmu : 17,734,840|17,786,624|51,784|0.3%

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

This conversion got a bit out of scope in trying to fix an existing core bug/lacking - the inability to add classes to the attributes arrays/objects in this template. There is a lot of extra Attribute object instantiation in preprocess that is actually not needed as well. So this reverts some of those changes for the sake of performance, and can we please address that bug in another issue.

Also removed some unneeded code from template_process_field() that is no longer needed now that #1982024: Lazy-load Attribute objects later in the rendering process only if needed is in.

Profiling results for attached patch, same scenario as above:

=== field-8.x..field-8.x compared (519de8fbb9479..519de96d52213):

ct  : 219,681|219,681|0|0.0%
wt  : 790,731|789,085|-1,646|-0.2%
cpu : 763,605|761,786|-1,819|-0.2%
mu  : 16,780,112|16,780,112|0|0.0%
pmu : 17,785,432|17,785,432|0|0.0%

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

=== field-8.x..field-1898062-73-profile compared (519de8fbb9479..519de9d0c2818):

ct  : 219,681|233,082|13,401|6.1%
wt  : 790,731|824,600|33,869|4.3%
cpu : 763,605|796,924|33,319|4.4%
mu  : 16,780,112|16,833,400|53,288|0.3%
pmu : 17,785,432|17,839,936|54,504|0.3%

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

@Cottser, the lazy loaded attributes only work for the templates not the theme_ functions, maybe it should also work for the theme_*() functions as well?

After some discussion with @carl and @joelpittet I think we can get this to pass by moving the location that Attributes is instantiated in theme.inc. I will work on a re-roll of #86 with this change.

Ran another profiling pass following #80

Scenario
* Create 8.x branch, copy field.tpl.php to core/themes/stark/templates
* Create test branch, copy field.html.twig to core/themes/stark/templates
* Configure Article with 5 fields, including body (see attached config)
* Generate 40 nodes
* Configure frontpage view to show full node content of 40 nodes
* Place block with full node view in sidebar

=== field-base-8.x..field-base-8.x compared (519e62c02d767..519e633ddef65):

ct  : 151,892|151,892|0|0.0%
wt  : 582,054|581,661|-393|-0.1%
cpu : 563,711|563,090|-621|-0.1%
mu  : 16,435,144|16,435,144|0|0.0%
pmu : 16,877,640|16,877,640|0|0.0%

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

Run 519e62c02d767 uploaded successfully for drupal-perf-drupalcon.
Run 519e636f9416a uploaded successfully for drupal-perf-drupalcon.
=== field-base-8.x..1898062-86-field compared (519e62c02d767..519e636f9416a):

ct  : 151,892|158,027|6,135|4.0%
wt  : 582,054|598,276|16,222|2.8%
cpu : 563,711|578,815|15,104|2.7%
mu  : 16,435,144|16,492,104|56,960|0.3%
pmu : 16,877,640|16,934,536|56,896|0.3%

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

Ah right. Thanks everyone. To be clear, the profiling in #90 is showing 110 fields, not 240.

@tlattimore et al - I don't think removing those Attribute instantiations from template_process_field() saved us much anyway so we could just add them back.

Here is change to the patch from #86 that adds back in some variables for template_process_field() that we need to prevent exceptions being thrown.

The patch from #92 also needs profiling.

Re-profiled, exact same scenario.

=== field-8.x..field-8.x compared (519ea884c38e9..519ea91263307):

ct  : 219,681|219,681|0|0.0%
wt  : 791,885|791,358|-527|-0.1%
cpu : 764,006|764,401|395|0.1%
mu  : 16,780,112|16,780,112|0|0.0%
pmu : 17,785,432|17,785,432|0|0.0%

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

=== field-8.x..field-1898062-92-profile compared (519ea884c38e9..519ea984d991e):

ct  : 219,681|233,082|13,401|6.1%
wt  : 791,885|824,009|32,124|4.1%
cpu : 764,006|796,888|32,882|4.3%
mu  : 16,780,112|16,833,400|53,288|0.3%
pmu : 17,785,432|17,839,936|54,504|0.3%

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

Looks good, re-reviewed the patch, profiling data is solid, the field.html.twig is not used by default in core. Follow-up issue has been created.

=> RTBC

Committed 624f9e6 and pushed to 8.x. Thanks!

add related as mentioned in issue summary of #1987398: field.module - Convert theme_ functions to Twig