Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Recognize Path-like objects in Templates#compile_template #529

Closed
wants to merge 1 commit into from

2 participants

@blambeau

The following commit aims at allowing the following behavior

get '/:some/:parameters' do

  #
  # Find an existing file that correspond to the request. Make some checks.
  # Returns a Pathname or a Path instance, responding to #path
  #
  filepath = ...

  #
  # Render found file, reusing sinatra template cache, options, etc.
  #
  erb filepath, ...

end

The pull request allows this provided that the located file is inside the views folder. This is to match the current behavior that does not allow rendering files outside it (without extending find_template).

I'm not sure this pull request makes sense. However, I'm not really confortable with the following workarounds:

# assuming a relative_from method that returns another Path(name)
erb filepath.relative_from(settings.views).to_s.to_sym

# manually use Tilt, no cache support
Tilt.new(filepath).render ...

# explicit use of the template cache
template_cache.fetch(filepath){ ... }.render ...

What do you think?

@travisbot

This pull request fails (merged 985f11b into 8752085).

@blambeau

Further thoughts about this. An additional problem I encounter is that, currently, in Sinatra knowing the name or file of a view is not sufficient for rendering it. You also need to know which engine has to be used:

def render(engine, data, options={}, locals={}, &block)
def compile_template(engine, data, options, views)
def find_template(views, name, engine)

All those methods require knowing the engine a priori. That seems more restrictive than Tilt itself, which returns a template if you provide a file... However, using Tilt directly or indirectly through the template cache seems difficult since the engine is used as a cache key:

template_cache.fetch engine, data, options

Now, it is true that having only the name of a view might be ambiguous if multiple files have the same name but different extensions. Still, such ambiguity already hurts sinatra internals somewhat, in find_template...

By the way, Tilt's support for smart bind between extensions and engines is difficult to use in Sinatra for the same reasons.

@blambeau

Ok, I've got a full example of what I would like to achieve:

https://github.com/blambeau/wlang#sinatra-integration

The current implementation works, but needs serious workarounds that use private APIs:

https://github.com/blambeau/wlang/blob/wlang2/lib/wlang/scope/sinatra_scope.rb#L16-40

Let me know what you think!

@blambeau

Forget about this one, too specific. I'll make another proposal for better integration with Tilt.

@blambeau blambeau closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 31 additions and 1 deletion.
  1. +8 −1 lib/sinatra/base.rb
  2. +23 −0 test/templates_test.rb
View
9 lib/sinatra/base.rb
@@ -738,7 +738,14 @@ def compile_template(engine, data, options, views)
path, line = settings.caller_locations.first
template.new(path, line.to_i, options, &body)
else
- raise ArgumentError, "Sorry, don't know how to render #{data.inspect}."
+ if data.respond_to?(:path) and path = data.path
+ path = File.absolute_path(path, views)
+ throw :layout_missing unless path.index(File.expand_path(views))==0
+ throw :layout_missing if eat_errors and not File.exists?(path)
+ template.new(path, 1, options)
+ else
+ raise ArgumentError, "Sorry, don't know how to render #{data.inspect}."
+ end
end
end
end
View
23 test/templates_test.rb
@@ -14,6 +14,8 @@ def evaluate(scope, locals={}, &block)
Tilt.register 'test', self
end
+Pathlike = Struct.new(:path)
+
class TemplatesTest < Test::Unit::TestCase
def render_app(base=Sinatra::Base, options = {}, &block)
base, options = Sinatra::Base, base if base.is_a? Hash
@@ -52,6 +54,27 @@ def with_default_layout
assert_equal "Hello World!\n", body
end
+ it 'renders Path-like objects given a relative path to views' do
+ render_app{ render(:test, Pathlike.new('hello.test')) }
+ assert ok?
+ assert_equal "Hello World!\n", body
+ end
+
+ it 'renders Path-like objects given an absolute path' do
+ render_app{ render(:test, Pathlike.new(File.expand_path("../views/hello.test", __FILE__))) }
+ assert ok?
+ assert_equal "Hello World!\n", body
+ end
+
+ it 'does not render Path-like objects outside views' do
+ mock_app do
+ get '/' do
+ render :test, Pathlike.new(File.expand_path(__FILE__))
+ end
+ end
+ assert_throws(:layout_missing){ get '/' }
+ end
+
it 'uses the default layout template if not explicitly overridden' do
with_default_layout do
render_app { render(:test, :hello) }
Something went wrong with that request. Please try again.