Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Created a Raw handler for templates. #6309

Merged
merged 1 commit into from

3 participants

@steveklabnik
Collaborator

This addresses the issue brought up in #2394.

I built this based on @josevalim's suggestion, which involves creating a raw template and adding a deprecation message for default template handlers.

One issue with this as it stands is that the tests now throw some warnings; I should probably fix those up, but I wanted to get feedback on my test and this code first. I'm 99% sure that it's all good, but I've never dived into this part of Rails before.

@josevalim
Owner

Thanks @steveklabnik ! This looks great except the template is never being registered. So nobody can render it with a .raw extension. Could you please register it as well and add another test? Thanks!

@steveklabnik
Collaborator

I will do that later today.

@steveklabnik
Collaborator

Turns out I have a spare ten minutes here...

I've amended the commit to include the registration, but since there aren't any tests about the registration so far, I'm not sure how to write it. Any hints?

@josevalim
Owner

Well, I would just create a template with raw extension in the fixtures directory and render it by path.

@steveklabnik
Collaborator

Awesome, give me a few hours. Thanks for the help.

@steveklabnik
Collaborator

Okay, so I've added that test. But there are... 505 tests that throw this deprecation notice now. Ouch. I'm guessing it probably shouldn't output that, huh?

@josevalim
Owner

Yes, we should probably fix the majority of these before merging. On the good side, they will probably be fixed by simply renaming the files to have .erb extension.

@steveklabnik
Collaborator

I made a silly mistake: threw the warning no matter what! That's been fixed, and now we only get one warning, in the NullResolverTest. I'm not 100% sure how to fix it yet... gonna dig in at lunch, I think.

@steveklabnik
Collaborator

The tests now have zero deprecation warnings. We should be good to go!

actionpack/lib/action_view/template/handlers/raw.rb
@@ -0,0 +1,9 @@
+module ActionView
+ module Template::Handlers
+ class Raw
+ def call(template)
+ "'#{template.source}';"
@josevalim Owner

One last thing: wouldn't template.source.inspect be a better alternative here since it would automatically escape quotes?

@steveklabnik Collaborator

Quite possibly. I was copying what was done in the Builder handler. I can absolutely change that if you'd like, I have no idea what's the best.

@steveklabnik Collaborator

I'm not sure using inspect is good:

 1) Failure:
test_render_raw_template_with_handlers(CachedViewRenderTest) [/Users/steve/src/rails/actionpack/test/template/render_test.rb:83]:
--- expected
+++ actual
@@ -1,2 +1,2 @@
-"<%= hello_world %>
-"
+"\"<%= hello_world %>\
+\""


  2) Failure:
test_render_raw_template_with_handlers(LazyViewRenderTest) [/Users/steve/src/rails/actionpack/test/template/render_test.rb:83]:
--- expected
+++ actual
@@ -1,2 +1,2 @@
-"<%= hello_world %>
-"
+"\"<%= hello_world %>\
+\""



  3) Failure:
test_raw_template(TestERBTemplate) [/Users/steve/src/rails/actionpack/test/template/template_test.rb:69]:
Expected: "<%= hello %>"
  Actual: "\"<%= hello %>\""

Or is that what we'd want?

@josevalim Owner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@steveklabnik
Collaborator

Updated to explicitly test quoting.

@josevalim
Owner
@steveklabnik
Collaborator

Hahahaha. Lemme check that...

@steveklabnik
Collaborator

Apparently it does work. I updated the test to test a bunch of characters, regardless.

@steveklabnik
Collaborator

Actually, @evanphx says: It detects matching {}'s yes, but if there just a } it will blow up

@steveklabnik
Collaborator

So, this commit is now just a WIP, as it fails the test, but @rkh pointed out to me what tilt does: https://github.com/rtomayko/tilt/blob/master/lib/tilt/string.rb

Trying to port that here. Tests are failing, but I have a few hours where I can't work on this, so I figured i'd push my wip up

@steveklabnik
Collaborator

Okay! This now works. Some discussion here: https://gist.github.com/2720171

@josevalim
Owner

Merged, thanks a lot!

@josevalim josevalim merged commit f5ebc9a into from
@steveklabnik
Collaborator

Yay! :D

@carlosantoniodasilva

Are we going anywhere with this warning?

Collaborator

We could.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 17, 2012
  1. @steveklabnik
This page is out of date. Refresh to see the latest.
View
2  actionpack/lib/action_view/template/handlers.rb
@@ -4,10 +4,12 @@ class Template
module Handlers #:nodoc:
autoload :ERB, 'action_view/template/handlers/erb'
autoload :Builder, 'action_view/template/handlers/builder'
+ autoload :Raw, 'action_view/template/handlers/raw'
def self.extended(base)
base.register_default_template_handler :erb, ERB.new
base.register_template_handler :builder, Builder.new
+ base.register_template_handler :raw, Raw.new
end
@@template_handlers = {}
View
11 actionpack/lib/action_view/template/handlers/raw.rb
@@ -0,0 +1,11 @@
+module ActionView
+ module Template::Handlers
+ class Raw
+ def call(template)
+ escaped = template.source.gsub(':', '\:')
+
+ '%q:' + escaped + ':;'
+ end
+ end
+ end
+end
View
4 actionpack/lib/action_view/template/resolver.rb
@@ -171,7 +171,9 @@ def mtime(p)
def extract_handler_and_format(path, default_formats)
pieces = File.basename(path).split(".")
pieces.shift
- handler = Template.handler_for_extension(pieces.pop)
+ extension = pieces.pop
+ ActiveSupport::Deprecation.warn "The file #{path} did not specify a template handler. The default is currently ERB, but will change to RAW in the future." unless extension
+ handler = Template.handler_for_extension(extension)
format = pieces.last && Mime[pieces.last]
[handler, format]
end
View
1  actionpack/test/fixtures/plain_text.raw
@@ -0,0 +1 @@
+<%= hello_world %>
View
1  actionpack/test/fixtures/plain_text_with_characters.raw
@@ -0,0 +1 @@
+Here are some characters: !@#$%^&*()-="'}{`
View
8 actionpack/test/template/render_test.rb
@@ -79,6 +79,14 @@ def test_render_template_with_handlers
assert_equal "<h1>No Comment</h1>\n", @view.render(:template => "comments/empty", :handlers => [:builder])
end
+ def test_render_raw_template_with_handlers
+ assert_equal "<%= hello_world %>\n", @view.render(:template => "plain_text")
+ end
+
+ def test_render_raw_template_with_quotes
+ assert_equal %q;Here are some characters: !@#$%^&*()-="'}{`; + "\n", @view.render(:template => "plain_text_with_characters")
+ end
+
def test_render_file_with_localization_on_context_level
old_locale, @view.locale = @view.locale, :da
assert_equal "Hey verden", @view.render(:file => "test/hello_world")
View
7 actionpack/test/template/template_test.rb
@@ -48,7 +48,7 @@ def my_buffer
end
def new_template(body = "<%= hello %>", details = {})
- ActionView::Template.new(body, "hello template", ERBHandler, {:virtual_path => "hello"}.merge!(details))
+ ActionView::Template.new(body, "hello template", details.fetch(:handler) { ERBHandler }, {:virtual_path => "hello"}.merge!(details))
end
def render(locals = {})
@@ -64,6 +64,11 @@ def test_basic_template
assert_equal "Hello", render
end
+ def test_raw_template
+ @template = new_template("<%= hello %>", :handler => ActionView::Template::Handlers::Raw.new)
+ assert_equal "<%= hello %>", render
+ end
+
def test_template_loses_its_source_after_rendering
@template = new_template
render
View
4 actionpack/test/template/testing/fixture_resolver_test.rb
@@ -8,8 +8,8 @@ def test_should_return_empty_list_for_unknown_path
end
def test_should_return_template_for_declared_path
- resolver = ActionView::FixtureResolver.new("arbitrary/path" => "this text")
- templates = resolver.find_all("path", "arbitrary", false, {:locale => [], :formats => [:html], :handlers => []})
+ resolver = ActionView::FixtureResolver.new("arbitrary/path.erb" => "this text")
+ templates = resolver.find_all("path", "arbitrary", false, {:locale => [], :formats => [:html], :handlers => [:erb]})
assert_equal 1, templates.size, "expected one template"
assert_equal "this text", templates.first.source
assert_equal "arbitrary/path", templates.first.virtual_path
View
4 actionpack/test/template/testing/null_resolver_test.rb
@@ -3,10 +3,10 @@
class NullResolverTest < ActiveSupport::TestCase
def test_should_return_template_for_any_path
resolver = ActionView::NullResolver.new()
- templates = resolver.find_all("path", "arbitrary", false, {:locale => [], :formats => [:html], :handlers => []})
+ templates = resolver.find_all("path.erb", "arbitrary", false, {:locale => [], :formats => [:html], :handlers => []})
assert_equal 1, templates.size, "expected one template"
assert_equal "Template generated by Null Resolver", templates.first.source
- assert_equal "arbitrary/path", templates.first.virtual_path
+ assert_equal "arbitrary/path.erb", templates.first.virtual_path
assert_equal [:html], templates.first.formats
end
end
Something went wrong with that request. Please try again.