Rails core - Apr 2007 - Odd error handling in ActionView#compile_template causes WSOD

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


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On Apr 5, 2007, at 2:37 PM, Jon wrote:
>
> 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.
Is it only me or has the template lookup code in Rails become an  
unmanagable-unrefactorable pile of gunk? (blantly put)
-- 
Julian ''Julik'' Tarkhanov
please send all personal mail to
me at julik.nl



--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On 4/5/07, Jon <jdelStrother@gmail.com> wrote:
>
> 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 ?
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
http://lighthouseapp.com
http://weblog.techno-weenie.net
http://mephistoblog.com

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

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

-- 
Rick Olson
http://lighthouseapp.com
http://weblog.techno-weenie.net
http://mephistoblog.com

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On 5-Apr-07, at 9:01 AM, Julian Tarkhanov wrote:
> Is it only me or has the template lookup code in Rails become an
> unmanagable-unrefactorable pile of gunk? (blantly put)
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.

-- 

Thanks,

Dan
__________________________________________________________________

Dan Kubb
Autopilot Marketing Inc.

Email: dan.kubb@autopilotmarketing.com
Phone: 1 (604) 820-0212
Web:   http://autopilotmarketing.com/
__________________________________________________________________




--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

> 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


-- 
Rick Olson
http://lighthouseapp.com
http://weblog.techno-weenie.net
http://mephistoblog.com

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---