195695 - Requesting a non-existant format results in an Internal Error

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[Dave Miller [:justdave]]

This situation, because it's most likely user error (requesting a format that
doesn't exist) should ThrowUserError.  It winds up doing ThrowTemplateError
because it blindly tries to run the template of that format even though it
doesn't exist.

Comment 1

[Bradley Baetz (:bbaetz)]

We had this discussion before, and ISTR that I wanted that, but I lost :)

The issue is complicated by languages, too. You could probably find out if the
template exists by playing with $template->provider, which would be easier than
the file::find stuff.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Unless we know for a fact that it's user error, and possibly even then, we
shouldn't blame the user, because it doesn't do any good and makes the user unhappy.

Comment 3

[Gervase Markham [:gerv]]

It's all very well to say:

> it blindly tries to run the template of that format even though it
> doesn't exist.

but it's not as simple as that. Bugzilla has no idea what template actually gets
run - this is decided by Template Toolkit's search capabilities modified by
whatever search path we give it. And when the error comes back "template not
found", we don't know which template they _meant_ - and so it's hard to tell
whether the problem is an invalid format, an invalid ctype, a valid format but a
missing corresponding ctype, or a bug in Bugzilla.

However, we could do better than currently. in ThrowTemplateError(), we could
detect the "missing template" error and do something different. However, I'm not
completely sure how we do the detection, since all we currently get is a
"Template::Exception at" message. Isn't there supposed to be some fix for that?

Gerv

Comment 4

[Bradley Baetz (:bbaetz)]

We can try looking for the template via
Bugzilla->template->provider->fetch("template_name"), and checking for
STATUS_DECLINED as the return value to mean 'not found'. We may be able to just
test teh ->process call for STATUS_DECLINED, but I suspect that we'd pick up on
filters and the like not being found, then.

Using ->provider also means taht we can check without processing, and error out
early.

Comment 5

[Bradley Baetz (:bbaetz)]

Oh, and the error message fix is in TT devel versions - theres no workarround we
can do from within Bugzilla.

Comment 6

[Gervase Markham [:gerv]]

Bug 195686 is related.

bbaetz: can you knock up a Template::Provider patch for this bug? It seems we do
need to check a template exists before invoking it, for both this reason and the
one in 195686.

Gerv

Comment 7

[Bradley Baetz (:bbaetz)]

Hmm. I have half a patch moving that stuff into Bugzilla::Template. Let me
finish that off.

Comment 8

[Bradley Baetz (:bbaetz)]

Attachment: patch

This is the simple fix.

On IRC, we spoke about returning undef and letting the caller handle it, but I
didn't do that because:

a) All the callers, both now and in the foreseeable future, are going to want
to treat this as a hard error; and
b) GetFormat knows more about what its doing than its caller, because it knows
the default ctype (which is admittedly very unlikely to change, so we could
hard code it into the output templte, or just not print it if it wasn't
explicitly specified);
c) ThrowUserError will one day be catchable via eval, for any caller who does
(for some unknown reason) want to catch this.

(c) is a bit trickier, though, so don't count on it happening too quickly.
Since I can't think why someone would want to ignore the requested types, thats
ok.

Comment 9

[Gervase Markham [:gerv]]

Comment on attachment 117365 [details] [diff] [review]
patch

>Index: globals.pl
>===================================================================
>@@ -1505,12 +1508,31 @@
>     $template .= ($format ? "-$format" : "");
>     $template .= ".$ctype.tmpl";
> 
>-    return
>-    {
>-        'template'    => $template ,
>-        'extension'   => $ctype ,
>-        'ctype' => $::contenttypes->{$ctype} || "text/plain" ,
>-    };
>+    # Now check that the template actually exists
>+    # We only want to check existance, so we have to iterate

"existence". 

>+    # over the LOAD_TEMPLATES list ourself.

"ourselves".

>+    # Template::Context->template doesn't let us distinguish between
>+    # file not found, and other errors (such as syntax errors) without
>+    # fragile stuff like parsing the error message

I'm confused by this comment. You say we can't distinguish between FNF and
other errors, but then below you seem to do exactly that.

If it's a problem, have you filed a bug on TT about this?

>+    foreach my $provider (@{Bugzilla->template->context->{ LOAD_TEMPLATES }}) {
>+        (undef, my $status) = $provider->fetch($template);
>+        if ($status != Template::Constants::STATUS_DECLINED) {
>+            # We have found something.
>+            # It may be an error, but if so then the template still exists,
>+            # so let the caller handle it in ->process
>+            return
>+              {
>+               'template'    => $template ,
>+               'extension'   => $ctype ,
>+               'ctype' => $::contenttypes->{$ctype},

Nit: why not line up the hash properly? :-)

Gerv

Comment 10

[Bradley Baetz (:bbaetz)]

I said that Template::Context->template doesn't let us. I'm not using that - I'm
manually iterating over the providers instead.

Let me mail the TT list and see.

Comment 11

[Bradley Baetz (:bbaetz)]

Attachment: v2

So it turns out that it is OK to just parse the error text. See the URL in the
patch.

Comment 12

[Bradley Baetz (:bbaetz)]

Comment on attachment 117710 [details] [diff] [review]
v2

gerv, try this one?

Comment 13

[Gervase Markham [:gerv]]

Comment on attachment 117710 [details] [diff] [review]
v2

>Index: globals.pl
>===================================================================
>+
>+use Template::Constants;

Are we using this any more?

>         'template'    => $template ,
>         'extension'   => $ctype ,
>-        'ctype' => $::contenttypes->{$ctype} || "text/plain" ,
>+        'ctype' => $::contenttypes->{$ctype},

Nits; align hash, missing space before comma for consistency.

r=gerv.

Gerv

Comment 14

[Bradley Baetz (:bbaetz)]

Fixed trivally locally, and I removed the use too

Comment 15

[Bradley Baetz (:bbaetz)]

Fixed.