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

Already on GitHub? Sign in to your account

Exception handling bug fix #4

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
2 participants

ian-kent commented Jun 4, 2013

Fixes bug where exceptions thrown in helper methods called from Xslate templates disappear (resulting in empty page with 200 response or 404 not found page)

Owner

gray commented Jun 5, 2013

I'll use Mojo::Exception, but can't merge this. You removed Try::Tiny and didn't even replace it with an eval, but are still expecting $@ to be useful there. I'll push an update shortly.

@gray gray closed this Jun 5, 2013

ian-kent commented Jun 5, 2013

you're right, I did - using an eval swallows the exception (sounds crazy, looks crazy, but it works - and using an eval didn't!)

ian-kent commented Jun 5, 2013

here's the test script I was using

#!/usr/bin/env perl

use Mojolicious::Lite;

plugin 'xslate_renderer';

helper 'test' => sub {
    die("Died");
};

get '/xs' => sub {
    shift->render('index');
};
get '/ep' => sub {
    shift->render('index_ep');
};

app->start;
__DATA__

@@ index_ep.html.ep

<!DOCTYPE html>
<html>
    <head>
        <title>test</title>
    </head>
    <body>
        <% test(); %>
    </body>
</html>

@@ index.html.tx
<!DOCTYPE html>
<html>
    <head>
        <title>test</title>
    </head>
    <body>
        <: $c.test() :>
    </body>
</html>
Owner

gray commented Jun 5, 2013

Even simpler- this looks like it will work as well. But the xslate error is swallowed and a generic Died message is used. Not sure how to trap the xslate message yet.

$$output = $self->xslate->render($name, \%params);
die $@ if $@;

ian-kent commented Jun 5, 2013

Just tried your fix and it works (and the xslate error does get output for me using the test script above, not sure why it would be different for you - did you try the script? the test helper method dies with 'Died' which looks like a generic error when mojo renders it)

I added the local $@ bit to fix a weird bug where, after the first exception, $@ was still set even if xslate->render didn't error - I haven't got a script to replicate that bug, but I assume it would still happen

Owner

gray commented Jun 5, 2013

Just tried your fix and it works (and the xslate error does get output for me using the test script above, not sure why it would be different for you - did you try the script? the test helper method dies with 'Died' which looks like a generic error when mojo renders it)

I think that generic error is coming from Mojolicious, not Xslate. Try the following to see the error I'm expecting to see:

$ perl -MText::Xslate -E 'my $xs = Text::Xslate->new(cache_dir => "/tmp", path => "/tmp"); $xs->render("abc");'
Text::Xslate: LoadError: Cannot find 'abc' (path: /tmp) at -e line 1.

I added the local $@ bit to fix a weird bug where, after the first exception, $@ was still set even if xslate->render didn't error - I haven't got a script to replicate that bug, but I assume it would still happen

Makes sense

@gray gray reopened this Jun 6, 2013

Owner

gray commented Jun 6, 2013

fixed in 0.09

@gray gray closed this Jun 6, 2013

@gray gray referenced this pull request Apr 22, 2016

Merged

Test Xslate as default handler #6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment