Problem/Motivation

menu.html.twig has a clever recursive macro, but according to the Twig documentation there is an unnecessary import inside of it.

We can see in the compiled Twig template that the import is doing an unnecessary variable reinitialization.

doDisplay method (main code path, doesn't change with this patch):

protected function doDisplay(array $context, array $blocks = array())
{
    // line 16
    $context["menus"] = $this;
    // line 17
    echo "
";
    // line 22
    echo $this->env->getExtension('drupal_core')->renderVar($context["menus"]->getmenu_links((isset($context["items"]) ? $context["items"] : null), (isset($context["attributes"]) ? $context["attributes"] : null), 0));
    echo "

";
}

Before

ob_start();
try {
    // line 25
    echo "  ";
    $context["menus"] = $this;
    // line 26
    echo "  ";
    if ((isset($context["items"]) ? $context["items"] : null)) {
        // line 27

After

ob_start();
try {
    // line 25
    echo "  ";
    if ((isset($context["items"]) ? $context["items"] : null)) {
        // line 26

Because this is the only Twig macro (and only recursive Twig macro) in core it should probably set a better example.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because nothing is broken
Issue priority Normal because nothing is broken
Unfrozen changes Only changes templates.
Prioritized changes Not a prioritized change.
Disruption Not disruptive at all. If someone has overridden this template they can keep the overridden template and won't have any issues.

Proposed resolution

Remove the unnecessary import from inside the macro.

Remaining tasks

Patch (both Classy and core's menu.html.twig)
Patch review

User interface changes

n/a

API changes

n/a

Files: 
CommentFileSizeAuthor
#8 menu_html_twig_has_an-2501491-8.patch1.51 KBChernous_dn
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,829 pass(es), 152 fail(s), and 83 exception(s). View
#4 menu_html_twig_has_an-2501491-4.patch1.94 KBChernous_dn
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,931 pass(es), 54 fail(s), and 2 exception(s). View
#1 menu_html_twig_has_an-2501491-1.patch1004 bytesCottser
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,595 pass(es), 153 fail(s), and 84 exception(s). View

Comments

Cottser’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
1004 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,595 pass(es), 153 fail(s), and 84 exception(s). View

Can't resist.

Cottser’s picture

Issue summary: View changes

Adding before/after of compiled Twig templates to the issue summary.

Status: Needs review » Needs work

The last submitted patch, 1: menu_html_twig_has_an-2501491-1.patch, failed testing.

Chernous_dn’s picture

FileSize
1.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,931 pass(es), 54 fail(s), and 2 exception(s). View

I added some changes to the patch #1 and add closing tags </ul>

Chernous_dn’s picture

Status: Needs work » Needs review
joelpittet’s picture

@Chernous_dn I think @Cottser realized we may still need this... check out if the toolbar still works?

Status: Needs review » Needs work

The last submitted patch, 4: menu_html_twig_has_an-2501491-4.patch, failed testing.

Chernous_dn’s picture

Status: Needs work » Needs review
FileSize
1.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,829 pass(es), 152 fail(s), and 83 exception(s). View

@joelpittet Yes, I made a mistake. Update patch.

Cottser’s picture

Status: Needs review » Closed (cannot reproduce)

Oops I thought I closed this. Pretty sure we do need the import :)

Status: Closed (cannot reproduce) » Needs work

The last submitted patch, 8: menu_html_twig_has_an-2501491-8.patch, failed testing.

Cottser’s picture

Status: Needs work » Closed (cannot reproduce)
Chernous_dn’s picture