Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bad output when exception is thrown #11

Closed
mahagr opened this issue Sep 3, 2021 · 8 comments · Fixed by #12
Closed

Bad output when exception is thrown #11

mahagr opened this issue Sep 3, 2021 · 8 comments · Fixed by #12

Comments

@mahagr
Copy link

mahagr commented Sep 3, 2021

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
Copy link
Owner

rybakit commented Sep 4, 2021

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

@mahagr
Copy link
Author

mahagr commented Sep 5, 2021

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
Copy link
Author

mahagr commented Sep 14, 2021

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
Copy link
Owner

rybakit commented Sep 17, 2021

@mahagr Could you please test #12?

@mahagr
Copy link
Author

mahagr commented Sep 17, 2021

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

@mahagr
Copy link
Author

mahagr commented Sep 17, 2021

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 added a commit that referenced this issue Sep 17, 2021
Remove obsolete deferred blocks on error, improve template compilation

See #11.
rybakit added a commit that referenced this issue Sep 17, 2021
Remove obsolete deferred blocks on error, improve template compilation

See #11.
@rybakit
Copy link
Owner

rybakit commented Sep 17, 2021

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.

rybakit added a commit that referenced this issue Sep 17, 2021
Remove obsolete deferred blocks on error, improve template compilation

See #11.
@mahagr
Copy link
Author

mahagr commented Sep 20, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants