Task

Convert theme_field__node__title() to a Twig template.

This was added in #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title but now theme_field() is gone so we shouldn't be overriding it anymore.

Remaining tasks

  • Patch
  • Patch review
  • Manual testing

Steps to test

  1. Create a node.
  2. Visit the homepage (default homepage view). The node title of the teaser should be rendered via field--node--title.html.twig.
Files: 
CommentFileSizeAuthor
#7 interdiff.txt1.47 KBjoelpittet
#7 2231505-7-twig-theme_field__node__title.patch3.02 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,571 pass(es). View

Comments

baisong’s picture

Status: Active » Needs review
FileSize
1.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,019 pass(es), 2 fail(s), and 0 exception(s). View

Patch attached. This is my first twig conversion patch, so please let me know what might be wrong!

Cottser’s picture

Status: Needs review » Needs work

Thanks @baisong, this looks like a great start!

  1. +++ b/core/modules/node/templates/field--node--title.html.twig
    @@ -0,0 +1,18 @@
    + * This is an override of theme_field() for the node title field. See that
    + * function for documentation about its details and overrides.
    ...
    + * @see theme_field()
    

    We should refer to field.html.twig instead of the now-defunct theme_field() function.

  2. +++ b/core/modules/node/templates/field--node--title.html.twig
    @@ -0,0 +1,18 @@
    + * - items:
    

    This could be something like "List of all the field items."

  3. +++ b/core/modules/node/templates/field--node--title.html.twig
    @@ -0,0 +1,18 @@
    +<span {{ attributes }}>{{ items }}</span>
    

    No space between <span and {{ attributes }} here, the Attribute object automatically inserts whitespace as needed so that we don't end up with things like <span > :)

baisong’s picture

Status: Needs work » Needs review
FileSize
1.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,983 pass(es), 2 fail(s), and 0 exception(s). View

Thanks, Cottser! Incorporated feedback.

The last submitted patch, 1: core-node-convert-title-twig-2231505-1.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: core-node-convert-title-twig-2231505-3.patch, failed testing.

baisong’s picture

It looks like it failed on these 2 assertions in LocalePathTest.php:

Line 143

$elements = $this->xpath('//a[@href=:href and .=:title]', array(':href' => $custom_path_url, ':title' => $first_node->label()));
$this->assertTrue(!empty($elements), 'First node links to the path alias.');

Line 145

$elements = $this->xpath('//a[@href=:href and .=:title]', array(':href' => $custom_path_url, ':title' => $second_node->label()));
$this->assertTrue(!empty($elements), 'Second node links to the path alias.');

Trying to debug through the test locally...

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.02 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,571 pass(es). View
1.47 KB

Those are just whitespace errors because of the newline at the end of the twig file.

That xpath looks suspicious as it says .= when there is also a span element in the a tag. This changes that.

@baisong thanks for pointing out the error messages and putting together this patch! I need this for another issue I'm working on:)

#2229435: Clean up the way attributes are printed in field.html.twig

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the quick fix @joelpittet and thanks again @baisong for working on this!

The markup is identical here and this unblocks #2229435: Clean up the way attributes are printed in field.html.twig and #2226805: Remove unneeded code from template_preprocess_field(). I think this is ready to go.

Before:

<h2>
        <a href="/node/1"><span class="field field-node--title field-name-title field-type-string field-label-hidden" data-edit-field-id="node/1/title/en/teaser">Food bar</span></a>
      </h2>

After (including debug output):

<h2>
        <a href="/node/1">

<!-- THEME DEBUG -->
<!-- CALL: _theme('field') -->
<!-- FILE NAME SUGGESTIONS:
   * field--node--title--article.html.twig
   x field--node--title.html.twig
   * field--node--article.html.twig
   * field--title.html.twig
   * field--string.html.twig
   * field.html.twig
-->
<!-- BEGIN OUTPUT from 'core/modules/node/templates/field--node--title.html.twig' -->
<span class="field field-node--title field-name-title field-type-string field-label-hidden" data-edit-field-id="node/1/title/en/teaser">Food bar</span>

<!-- END OUTPUT from 'core/modules/node/templates/field--node--title.html.twig' -->

</a>
      </h2>
Cottser’s picture

Issue summary: View changes

Filling in testing steps in the issue summary.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 2231505-7-twig-theme_field__node__title.patch, failed testing.

Cottser’s picture

Cottser’s picture

Status: Needs work » Reviewed & tested by the community

Looks like a random test fail.

webchick’s picture

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

Hm. This is used in lots of places, so let's please get some profiling on this change.

Cottser’s picture

Assigned: Unassigned » Cottser

Agreed, I can profile this.

Cottser’s picture

Assigned: Cottser » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs profiling

Profiling scenario: 50 nodes in teaser mode on the default frontpage view (adjusted to display 50 nodes).

Total: 50 uses of this template.

The numbers look fine to me, keep in mind that this page isn't actually this slow, I had to disable render cache to actually get a comparison: https://gist.github.com/Cottser/10997441

=== 8.x..8.x compared (5350e128cfd2b..5350e1a85fcec):

ct  : 322,151|322,151|0|0.0%
wt  : 1,067,606|1,068,516|910|0.1%
cpu : 1,051,282|1,052,411|1,129|0.1%
mu  : 47,301,936|47,301,936|0|0.0%
pmu : 47,472,968|47,472,968|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5350e128cfd2b&...

=== 8.x..twig-field--node--title-2231505-7 compared (5350e128cfd2b..5350e2663e694):

ct  : 322,151|322,290|139|0.0%
wt  : 1,067,606|1,069,206|1,600|0.1%
cpu : 1,051,282|1,053,013|1,731|0.2%
mu  : 47,301,936|47,325,152|23,216|0.0%
pmu : 47,472,968|47,495,544|22,576|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5350e128cfd2b&...


Here's a run from 8.x with render cache enabled:

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?extra=8.x..8.x&sour...
(wall time 685,129 microsecs)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Oh shoot, I'm so sorry! Thought I committed this one already. :(

Committed and pushed to 8.x. Thanks!

  • Commit 87fe2f4 on 8.x by webchick:
    Issue #2231505 by joelpittet, baisong | Cottser: Convert...
joelpittet’s picture

Thank you!

Status: Fixed » Closed (fixed)

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