Proposal: Exception handling in container classes

Ruby Apps Radiant CMS
#1

Right now, render_tag (in page_context.rb) rescues all exceptions and
turns them into HTML output. It’d be really nice to be able to use
exceptions within behaviors - especially within container tags.
F’rinstance, I’ve got a “style” tag that nests inside a “navigation”
container, and if that style tag is missing a required attribute, I’d
like to raise an exception to the navigation container so it doesn’t
continue trying to render. But the container calls tag.expand, which in
turn calls render_tag, which steals the exception from me.

I understand the intent, but I wonder if there’s a less intrusive way to
accomplish it, while still allowing outer tags to rescue their own
exceptions. What if that rescue were executed only on the outermost
tags? Would that break anything? It seems to me that this would still
prevent exceptions from crashing Rails, while allowing internal
exception-handling. Ya?

OT but related: I tried reopening PageContext in my own plugin to make
this modification, but even a simple

class PageContext
end

results in the apparent loss of the class; behavior.rb then complains
that PageContext.new expects 0 arguments. Same if I do

class PageContext < Radiant::Context
end

I’m new to reopening classes; what am I doing wrong?

Jay

#2

Jay L. wrote:

I understand the intent, but I wonder if there’s a less intrusive way to
accomplish it, while still allowing outer tags to rescue their own
exceptions. What if that rescue were executed only on the outermost
tags? Would that break anything?

I’ve submitted a patch that accomplishes this - ticket #177.

OT but related: I tried reopening PageContext in my own plugin to make
this modification

Solved: Rails loads models automagically through const_missing the first
time they’re referenced. Plugins load before models, so if you define
the class in the plugin, it’s no longer missing, and the model doesn’t
get loaded! If I wanted to reopen PageContext in a plugin, I’d need to
require “page_context” at the top so it gets loaded first.

Jay

#3

John W. Long wrote:

this can be applied to the core.
I’ve been thinking over how to unit test it - more so since I found two
bugs in it (one fixed in the second patch, one not yet fixed). The
problem, of course, is that disabling exceptions in “test” makes it
impossible to test the code path that would trap the exception!

I’m thinking the best way is to add a global like
RERAISE_EXCEPTIONS_IN_TEST, which the test itself would turn off and
would otherwise default on. Sound ok?

As for code that makes use of it, here:

tag “outer” do |tag|
contents = tag.expand
html = “” + do_something(contents) + “”
end

tag “inner” do |tag|
[‘height’, ‘font_size’].each do |attr|
unless tag.attr.include?(attr)
raise TagError, “the #{name} tag must include a ‘#{attr}’
attribute.”
end
end
end

With Radiant 0.5, the error returned by “inner” gets trapped by the
first render_tag it sees, and so the output is “the inner tag
must include a ‘height’ attribute”, which (in my case) isn’t the
desired result; outer depends on inner, and I want everything or
nothing.

With the patch, render_tag doesn’t rescue the exception when rendering
inner, so it goes up the stack. In this particular case, “outer”
doesn’t catch it either, so the render_tag for outer rescues it and
displays the error message in lieu of the outer tag.

If, in fact, I wanted to render outer no matter what, then I’d make
“outer” something like

tag “outer” do |tag|
begin
contents = tag.expand
rescue TagError => e
LOGGER.debug ("Inner error: #{e.message})
ensure
html = “” + do_something(contents) + “”
end
end

Jay

#4

Jay L. wrote:

Jay L. wrote:

I understand the intent, but I wonder if there’s a less intrusive way to
accomplish it, while still allowing outer tags to rescue their own
exceptions. What if that rescue were executed only on the outermost
tags? Would that break anything?

I’ve submitted a patch that accomplishes this - ticket #177.

This sounds like a useful idea. Can you post some code which
demonstrates why this would be useful? Also I’ll need unit tests before
this can be applied to the core.

–
John L.
http://wiseheartdesign.com

#5

Jay L. wrote:

I imagine if I understood continuations better, I’d know how to code
this; the inner tag should raise an exception, and the outer tag should
note that, continue its processing, and then, at the end, reraise or
render. Sadly, I’m too new to Ruby, and non-procedural languages in
general, to know how to do that.

Yeah, so, never mind that. I was thinking waaay to hard. I changed the
default behavior from “outer tag keeps getting rendered” to “outer tag
doesn’t render when inner tag has an error”, and then wondered why I
wasn’t seeing the outer tag.

I just have to keep the default behavior the way it is, and then change
it only for tags that want it. That seems doable. I’ll stop using the
list to talk to myself now.

Jay

#6

John W. Long wrote:

this can be applied to the core.
Argh! This is harder than it seemed. In my version, when
PageContext::render_tag reraises the exception, the parent tag rescues
it, of course - which means the parent tag never finishes what it’s
doing. So anything inside the parent tag following the exception is
lost. Oops.

I imagine if I understood continuations better, I’d know how to code
this; the inner tag should raise an exception, and the outer tag should
note that, continue its processing, and then, at the end, reraise or
render. Sadly, I’m too new to Ruby, and non-procedural languages in
general, to know how to do that.

If anyone else wants to take a crack at this, here’s how far I got… I’m
about to go read a bunch of “continuations made easy” articles.

def render_tag(name, attributes = {}, &block)
super

rescue Exception => e
nesting = current_nesting
@tag_binding_stack.pop

 if reraise_exception(nesting, name, e)
   p @page.behavior.parser
   raise e
 else
   render_error_message(e.message)
 end

end

…
def reraise_exception(nesting, name, e)
tag_pos = nesting.rindex(name)

   # Missing end tag
   return false if tag_pos.nil?

   # Let inner tags pass exceptions to outer tags.
   return true if tag_pos > 1

   # Let bugs report themselves to unit tests.
   return true if RAILS_ENV == "test" and not e.is_a?(TagError)

   false
 end