Bad output when exception is thrown

mahagr commented 3 years ago

It looks like deferred acts erratically if an exception is thrown during rendering.

My proposed fix is something like this (against the legacy version, but should apply both):

DeferredExtension

    public function defer(\Twig_Template $template, $blockName)
    {
        ob_start();
        $templateName = $template->getTemplateName();
        $this->blocks[$templateName][] = [ob_get_level(), $blockName];
    }

    public function resolve(\Twig_Template $template, array $context, array $blocks)
    {
        $templateName = $template->getTemplateName();
        if (empty($this->blocks[$templateName])) {
            return;
        }

        while ($block = array_pop($this->blocks[$templateName])) {
            [$level, $blockName] = $block;
            if (ob_get_level() !== $level) {
                continue;
            }

            $buffer = ob_get_clean();

            $blocks[$blockName] = array($template, 'block_'.$blockName.'_deferred');
            $template->displayBlock($blockName, $context, $blocks);

            echo $buffer;
        }

        if ($parent = $template->getParent($context)) {
            $this->resolve($parent, $context, $blocks);
        }
    }

Basically, I store information about the ob level and ignore the buffer if the level doesn't match. I am not sure if this is the right approach, but at least it prevents too many ob_cleans from happening.

rybakit commented 3 years ago

Could you provide a reproducer showing the erratic behavior (ideally, a minimal template, and the actual and expected output)?

mahagr commented 3 years ago

I'll try to find some time. The template in question is a full HTML page with deferred CSS/JS loading in <head> and the exception is thrown in the body. After the exception is thrown, I show a different page instead. JS renders before the HTML of the shown error page.

mahagr commented 3 years ago

Here is the code to reproduce the issue:

#!/usr/bin/env php
<?php
use Twig\Environment;
use Twig\TwigFunction;
use Twig\Loader\ArrayLoader;
use Phive\Twig\Extensions\Deferred\DeferredExtension;

require __DIR__ . '/vendor/autoload.php';

$htmlTemplate = <<<HTML
{% block html %}
<html>
  <head>
  {% block head %}
    <meta http-equiv="X-UA-Compatible" content="IE=edge"/>
    <meta name="viewport" content="width=device-width, initial-scale=1"/>
  {% endblock %}

  {% block title %}
  <title>Page Title</title>
  {% endblock %}

  {% block assets deferred %}
    {{ style()|raw }}
  {% endblock %}
  </head>
  <body>
  {% block body %}
    {% block content %}
    {% endblock %}
  {% endblock %}
  </body>
</html>

{% endblock %}
HTML;
$errorTemplate = <<<ERROR
{% extends 'html' %}

{% block title %}
  <title>Error</title>
{% endblock %}

{% block content %}
  <h1>Error</h1>
{% endblock %}
ERROR;

$pageTemplate = <<<CONTENT
{% extends 'html' %}

{% block title %}
  <title>Page Title</title>
{% endblock %}

{% block content %}
  <h1>Page content</h1>
  <p>
    Content {{ error() }}
  </p>
{% endblock %}
CONTENT;

$loader = new ArrayLoader(['html' => $htmlTemplate, 'error' => $errorTemplate, 'page' => $pageTemplate]); 
$twig = new Environment($loader, []);
$twig->addExtension(new DeferredExtension());
$twig->addFunction(
    new TwigFunction('error', function () { throw new \Exception('Error!'); })
);
$twig->addFunction(
    new TwigFunction('style', function () { return '<style>head { text-color: red; }</style>'; })
);

$context = [];

try {
    $html = $twig->render('page', $context);
} catch (\Exception $e) {
    $html = $twig->render('error', $context);
}

echo $html;

If you run the code, it will render contents of style() before the document starts.

rybakit commented 3 years ago

@mahagr Could you please test https://github.com/rybakit/twig-deferred-extension/pull/12?

mahagr commented 3 years ago

Oh, that is a nice approach. I will test it when I have some extra time.

mahagr commented 3 years ago

BTW, your extension works as it is in the latest Twig 2.14 as well. I would add it to the supported list as long as you require also "php": ">=7.2.5".

Also Twig 1.44 support is easy to add, you only need this: https://github.com/getgrav/grav/blob/develop/system/src/Twig/DeferredExtension/DeferredNodeVisitorCompat.php https://github.com/getgrav/grav/blob/develop/system/src/Twig/DeferredExtension/DeferredExtension.php#L31-L34

rybakit commented 3 years ago

BTW, your extension works as it is in the latest Twig 2.14 as well. I would add it to the supported list as long as you require also "php": ">=7.2.5".

This will require updating CI to ensure that the tests pass on the lowest supported version of twig (2.14 in this case) as well as on the current 3.x. I'm not sure I have the time and interest to do that, but I might consider accepting a pull request if you're interested in working on it.

Also Twig 1.44 support is easy to add

If people are using such an ancient version, they probably aren't interested in any updates, fixes, or new features. In this case, they can continue using v1 of the extension.

mahagr commented 3 years ago

Some of us are stuck with the old version due to backward compatibility. :(

Anyways, I have the fix for those if they click the links above.

rybakit / twig-deferred-extension

Bad output when exception is thrown #11