Odd error handling in ActionView#compile_template causes WSOD

[Jon1 (Jon)]

If a view file cannot be compiled (eg it has a block with a missing
'end' statement), I'm experiencing WSODs - the browser reports a lost
network connection, rather than the helpful compilation error that we
used to have.

When the compilation fails, ActionView#compile_template raises this
error :

TemplateError.new(find_base_path_for(file_name || template), file_name

template, @assigns, template, e)

...which appears to have a few things wrong with it.

First off, file_name is always absolute here - eg /Users/jon/
Developer/Stacker/app/views/kittens/index.rhtml. find_base_path_for
is going to fail since it tries to append that path onto the path from
@view_paths - typically /Users/jon/Developer/Stacker/app/views.
So, find_base_path_for returns nil. I *think* what happens is that
code inside template_error.rb later raises a second exception when it
tries to use that path as a string and finds it's nil, and that
exception isn't caught properly, so the response is never formed,
which produces the WSOD. If I replace it with
TemplateError.new(find_base_path_for(file_name || template) || "",
file_name || template, @assigns, template, e)
the compilation error is reported properly.

The reason I'm posting here rather than submitting a patch to Trac is
that I'm struggling to understand a few things about that line of
code, and so I'm not sure of the best way to fix it.

'template' is the contents of the template file. So why are we trying
to use it as a filename for find_base_path_for()? Is it supposed to
be find_base_path_for(file_name) || template ? If so, why are we
creating a TemplateError with base_path=<file contents> ? Same
applies to the second parameter - if file_name is nil, use the
template content??
Why is find_base_path_for being used at all? Why not File.dirname ?

Thanks for any help figuring this out...
Jon

[Julian_Julik_Tarkhan (Julian 'Julik' Tarkhanov)]

Is it only me or has the template lookup code in Rails become an unmanagable-unrefactorable pile of gunk? (blantly put)

[Rick_Olson]

Apparently this is the result of some incorrectly transcribed code
when I committed the multiple viewpath support.

BEFORE
raise TemplateError.new(@base_path, file_name || template, @assigns,
template, e)

AFTER
raise TemplateError.new(lookup_template_base_path_for(file_name ||
template), file_name || template, @assigns, template, e)

The reason we have to call #find_base_path_for is TemplateError wants
the current base path (app/views) instead of the path to the template
file (app/views/your-controller).

Since file_name is the full path, perhaps we just go through the
available base_paths, looking for any that match the beginning of
file_name. If none match, then just assume the first base_path (99%
of the time you'll only have 1 anyway).

Thanks for bringing this up!

[Rick_Olson]

URLs so you folks at home can play along:

BEFORE
raise TemplateError.new(@base_path, file_name || template, @assigns,
template, e)

http://dev.rubyonrails.org/browser/trunk/actionpack/lib/action_view/base.rb?rev=6119#L514

AFTER
raise TemplateError.new(lookup_template_base_path_for(file_name ||
template), file_name || template, @assigns, template, e)

http://dev.rubyonrails.org/browser/trunk/actionpack/lib/action_view/base.rb?rev=6120#L514

[Dan_Kubb]

Its not just you, I've tried to debug view problems before and often
get lost in the tangle of code that's present.

My biggest pet peeve is the way erb, builder and rjs handling is
hard coded within the system. I'd like to see the view code get
refactored so that erb, builder, rjs AND custom template types
get handled in the exact same way.

[Rick_Olson]

AFTER
raise TemplateError.new(lookup_template_base_path_for(file_name ||
template), file_name || template, @assigns, template, e)

The reason we have to call #find_base_path_for is TemplateError wants
the current base path (app/views) instead of the path to the template
file (app/views/your-controller).

Since file_name is the full path, perhaps we just go through the
available base_paths, looking for any that match the beginning of
file_name. If none match, then just assume the first base_path (99%
of the time you'll only have 1 anyway).

This should work:

http://dev.rubyonrails.org/changeset/6505