zendframework / zend-expressive-twigrenderer

Twig integration for Expressive
BSD 3-Clause "New" or "Revised" License
25 stars 14 forks source link

Duplicate values for addDefaultParam when use TemplateRendererInterface::TEMPLATE_ALL #21

Closed rodrigowbazevedo closed 7 years ago

rodrigowbazevedo commented 7 years ago

When you add a default param for all templates and the value is an array like this:

$renderer = new \Zend\Expressive\Twig\TwigRenderer($twig);

$renderer->addDefaultParam(
    \Zend\Expressive\Template\TemplateRendererInterface::TEMPLATE_ALL,
    'foo',
    [
        [ 'bar' => 1 ],
        [ 'bar' => 2 ]
    ]
);

It duplicates the foo value into this:

    [
        [ 'bar' => 1 ],
        [ 'bar' => 2 ],
        [ 'bar' => 1 ],
        [ 'bar' => 2 ]
    ]

It happens at \Zend\Expressive\Twig\TwigRenderer::render because it calls mergeParams method two times, the first time with the requested template name ex:'default' and the second with the normalized template name 'default.html'

    public function render($name, $params = [])
    {
        // Merge parameters based on requested template name
        $params = $this->mergeParams($name, $this->normalizeParams($params));

        $name   = $this->normalizeTemplate($name);

        // Merge parameters based on normalized template name
        $params = $this->mergeParams($name, $params);

        return $this->template->render($name, $params);
    }

And at \Zend\Expressive\Template\DefaultParamsTrait::mergeParams it merge the TemplateRendererInterface::TEMPLATE_ALL:

    private function mergeParams($template, array $params)
    {
        $globalDefaults = isset($this->defaultParams[TemplateRendererInterface::TEMPLATE_ALL])
            ? $this->defaultParams[TemplateRendererInterface::TEMPLATE_ALL]
            : [];

        $templateDefaults = isset($this->defaultParams[$template])
            ? $this->defaultParams[$template]
            : [];

        $defaults = $this->merge($globalDefaults, $templateDefaults);

        return $this->merge($defaults, $params);
    }

So TemplateRendererInterface::TEMPLATE_ALL has been merged at both calls and if the value is an array it duplicates de values.

geerteltink commented 7 years ago

I've created a test case for this and it looks like it needs to be fixed in zend-expressive-template.

test/TestAsset/default-param-array.html

{% for row in foo %}{{ row.id }}{% endfor %}

test/TwigRendererTest.php

class TwigRendererTest extends TestCase
{
    // ...

    public function testDefaultParamArray()
    {
        $renderer = new TwigRenderer();
        $renderer->addPath(__DIR__ . '/TestAsset');
        $foo = [
            1 => ['id' => 1],
            2 => ['id' => 2],
            3 => ['id' => 3],
        ];

        // Test array doesn't have duplicated entries
        $renderer->addDefaultParam($renderer::TEMPLATE_ALL, 'foo', $foo);
        $result = $renderer->render('default-param-array');
        $this->assertEquals('123', $result);
    }

    public function testRenderParamArrayOverridesDefaultParamArray()
    {
        $renderer = new TwigRenderer();
        $renderer->addPath(__DIR__ . '/TestAsset');
        $foo = [
            1 => ['id' => 1],
            2 => ['id' => 2],
            3 => ['id' => 3],
        ];

        $renderer->addDefaultParam($renderer::TEMPLATE_ALL, 'foo', $foo);
        $result = $renderer->render('default-param-array', [
            'foo' => [
                3 => ['id' => 3],
                4 => ['id' => 4],
            ],
        ]);
        $this->assertEquals('1234', $result);
    }
}

The only way I could fix this was removing the merge method and use array_replace_recursive:

Zend\Expressive\Template\DefaultParamsTrait.php

trait DefaultParamsTrait
{
    // ...

    private function mergeParams($template, array $params)
    {
        $globalDefaults = isset($this->defaultParams[TemplateRendererInterface::TEMPLATE_ALL])
            ? $this->defaultParams[TemplateRendererInterface::TEMPLATE_ALL]
            : [];

        $templateDefaults = isset($this->defaultParams[$template])
            ? $this->defaultParams[$template]
            : [];

        // ====>>> Use array_replace_recursive in stead of the include merge method

        return array_replace_recursive($globalDefaults, $templateDefaults, $params);
    }

    // ...
}
rodrigowbazevedo commented 7 years ago

The only way I thought to fix this without change in zend-expressive-template, is to "override" the addDefaultParam trait method and call the normalizeTemplate at the template name before call the addDefaultParam trait method, so at the render method will not be necessary to call mergeParams two times.

But I don't know if it's the best way and I don't like the idea of "override" the trait method.