Skip to content

Commit

Permalink
All tests pass without memoizing view_context
Browse files Browse the repository at this point in the history
  • Loading branch information
Carlhuda committed Mar 18, 2010
1 parent 523d0f3 commit 71c9337
Show file tree
Hide file tree
Showing 11 changed files with 60 additions and 41 deletions.
16 changes: 13 additions & 3 deletions actionpack/lib/abstract_controller/rendering.rb
Expand Up @@ -32,7 +32,6 @@ def locale=(value)
module Rendering
extend ActiveSupport::Concern

include AbstractController::Assigns
include AbstractController::ViewPaths

# Overwrite process to setup I18n proxy.
Expand All @@ -53,7 +52,8 @@ def process(*) #:nodoc:
#
# Override this method in a module to change the default behavior.
def view_context
@_view_context ||= ActionView::Base.for_controller(self)
klass = ActionView::Base.for_controller(self)
klass.new(lookup_context, view_assigns, self)
end

# Normalize arguments, options and then delegates render_to_body and
Expand Down Expand Up @@ -82,7 +82,6 @@ def render_to_string(options={})
# Find and renders a template based on the options given.
# :api: private
def _render_template(options) #:nodoc:
_evaluate_assigns(view_context)
view_context.render(options)
end

Expand All @@ -105,6 +104,17 @@ def self.body_to_s(body)

private

# This method should return a hash with assigns.
# You can overwrite this configuration per controller.
# :api: public
def view_assigns
hash = {}
variables = instance_variable_names
variables -= protected_instance_variables if respond_to?(:protected_instance_variables)

This comment has been minimized.

Copy link
@janx

janx Apr 15, 2010

Contributor

I got an error today

can't convert nil into Array

stack trace shows the nil come from actionpack-3.0.0.beta3/lib/abstract_controller/rendering.rb:129:in `-'

maybe here need a guard? like:

variables -= (protected_instance_variables || []) if respond_to?(:protected_instance_variables)

This comment has been minimized.

Copy link
@foca

foca Apr 15, 2010

Contributor

It should actually be config.protected_instance_variables if config.respond_to?(:protected_instance_variables). I have a fix for it in https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/4404-patch-remove-deprecation-warnings-and-avoid-typeerrors-in-actionpacklibabstract_controllerrenderingrb that needs to be merged :)

This comment has been minimized.

Copy link
@janx

janx Apr 15, 2010

Contributor

cool :)

variables.each { |name| hash[name.to_s[1..-1]] = instance_variable_get(name) }
hash
end

# Normalize options by converting render "foo" to render :action => "foo" and
# render "foo/bar" to render :file => "foo/bar".
def _normalize_args(action=nil, options={})
Expand Down
19 changes: 0 additions & 19 deletions actionpack/lib/action_controller/caching/fragments.rb
Expand Up @@ -34,25 +34,6 @@ def fragment_cache_key(key)
ActiveSupport::Cache.expand_cache_key(key.is_a?(Hash) ? url_for(key).split("://").last : key, :views)
end

def fragment_for(name = {}, options = nil, &block) #:nodoc:
if perform_caching
if fragment_exist?(name, options)
read_fragment(name, options)
else
# VIEW TODO: Make #capture usable outside of ERB
# This dance is needed because Builder can't use capture
buffer = view_context.output_buffer
pos = buffer.length
yield
fragment = buffer.slice!(pos..-1)
write_fragment(name, fragment, options)
end
else
ret = yield
ActiveSupport::SafeBuffer.new(ret) if ret.is_a?(String)
end
end

# Writes <tt>content</tt> to the location signified by <tt>key</tt> (see <tt>expire_fragment</tt> for acceptable formats)
def write_fragment(key, content, options = nil)
return content unless cache_configured?
Expand Down
2 changes: 1 addition & 1 deletion actionpack/lib/action_controller/metal/renderers.rb
Expand Up @@ -87,7 +87,7 @@ def self._write_render_options
end

add :update do |proc, options|
_evaluate_assigns(view_context)
view_context = self.view_context
generator = ActionView::Helpers::PrototypeHelper::JavaScriptGenerator.new(view_context, &proc)
self.content_type = Mime::JS
self.response_body = generator.to_s
Expand Down
5 changes: 2 additions & 3 deletions actionpack/lib/action_dispatch/testing/assertions/routing.rb
Expand Up @@ -154,15 +154,14 @@ def with_routing
# TODO: Make this unnecessary
if @controller
@controller.singleton_class.send(:include, @router.url_helpers)
@controller.class._helper_serial += 1
@controller.view_context.singleton_class.send(:include, @router.url_helpers)
@controller.class._helper_serial = AbstractController::Helpers.next_serial + 1
end
yield @router
ensure
@router = old_routes
if @controller
@controller = old_controller
@controller.class._helper_serial += 1 if @controller
@controller.class._helper_serial = AbstractController::Helpers.next_serial + 1 if @controller
end
end

Expand Down
2 changes: 0 additions & 2 deletions actionpack/lib/action_view/base.rb
Expand Up @@ -250,8 +250,6 @@ def inspect
else
klass = self
end

klass.new(controller.lookup_context, {}, controller)
end

def initialize(lookup_context = nil, assigns_for_first_render = {}, controller = nil, formats = nil) #:nodoc:
Expand Down
22 changes: 21 additions & 1 deletion actionpack/lib/action_view/helpers/cache_helper.rb
Expand Up @@ -32,9 +32,29 @@ module CacheHelper
# <i>Topics listed alphabetically</i>
# <% end %>
def cache(name = {}, options = nil, &block)
safe_concat controller.fragment_for(name, options, &block)
safe_concat fragment_for(name, options, &block)
nil
end

private
# TODO: Create an object that has caching read/write on it
def fragment_for(name = {}, options = nil, &block) #:nodoc:
if controller.perform_caching
if controller.fragment_exist?(name, options)
controller.read_fragment(name, options)
else
# VIEW TODO: Make #capture usable outside of ERB
# This dance is needed because Builder can't use capture
pos = output_buffer.length
yield
fragment = output_buffer.slice!(pos..-1)
controller.write_fragment(name, fragment, options)
end
else
ret = yield
ActiveSupport::SafeBuffer.new(ret) if ret.is_a?(String)
end
end
end
end
end
8 changes: 6 additions & 2 deletions actionpack/test/controller/caching_test.rb
Expand Up @@ -616,8 +616,10 @@ def test_fragment_for_with_disabled_caching
@store.write('views/expensive', 'fragment content')
fragment_computed = false

view_context = @controller.view_context

buffer = 'generated till now -> '.html_safe
buffer << @controller.fragment_for('expensive') { fragment_computed = true }
buffer << view_context.send(:fragment_for, 'expensive') { fragment_computed = true }

assert fragment_computed
assert_equal 'generated till now -> ', buffer
Expand All @@ -627,8 +629,10 @@ def test_fragment_for
@store.write('views/expensive', 'fragment content')
fragment_computed = false

view_context = @controller.view_context

buffer = 'generated till now -> '.html_safe
buffer << @controller.fragment_for('expensive') { fragment_computed = true }
buffer << view_context.send(:fragment_for, 'expensive') { fragment_computed = true }

assert !fragment_computed
assert_equal 'generated till now -> fragment content', buffer
Expand Down
8 changes: 7 additions & 1 deletion actionpack/test/controller/render_test.rb
Expand Up @@ -18,6 +18,13 @@ class LabellingFormBuilder < ActionView::Helpers::FormBuilder

layout :determine_layout

def name
nil
end

private :name
helper_method :name

def hello_world
end

Expand Down Expand Up @@ -418,7 +425,6 @@ def render_to_string_with_inline_and_render

def rendering_with_conflicting_local_vars
@name = "David"
def view_context.name() nil end
render :action => "potential_conflicts"
end

Expand Down
2 changes: 1 addition & 1 deletion actionpack/test/template/capture_helper_test.rb
Expand Up @@ -114,7 +114,7 @@ def alt_encoding(output_buffer)
end

def view_with_controller
returning(ActionView::Base.for_controller(TestController.new)) do |view|
returning(TestController.new.view_context) do |view|
view.output_buffer = ActionView::OutputBuffer.new
end
end
Expand Down
15 changes: 8 additions & 7 deletions actionpack/test/template/output_buffer_test.rb
Expand Up @@ -10,6 +10,7 @@ def index
tests TestController

def setup
@vc = @controller.view_context
get :index
assert_equal ['foo'], body_parts
end
Expand All @@ -25,24 +26,24 @@ def setup
end

test 'flushing ignores empty output buffer' do
@controller.view_context.output_buffer = ''
@controller.view_context.flush_output_buffer
@vc.output_buffer = ''
@vc.flush_output_buffer
assert_equal '', output_buffer
assert_equal ['foo'], body_parts
end

test 'flushing appends the output buffer to the body parts' do
@controller.view_context.output_buffer = 'bar'
@controller.view_context.flush_output_buffer
@vc.output_buffer = 'bar'
@vc.flush_output_buffer
assert_equal '', output_buffer
assert_equal ['foo', 'bar'], body_parts
end

if '1.9'.respond_to?(:force_encoding)
test 'flushing preserves output buffer encoding' do
original_buffer = ' '.force_encoding(Encoding::EUC_JP)
@controller.view_context.output_buffer = original_buffer
@controller.view_context.flush_output_buffer
@vc.output_buffer = original_buffer
@vc.flush_output_buffer
assert_equal ['foo', original_buffer], body_parts
assert_not_equal original_buffer, output_buffer
assert_equal Encoding::EUC_JP, output_buffer.encoding
Expand All @@ -51,7 +52,7 @@ def setup

protected
def output_buffer
@controller.view_context.output_buffer
@vc.output_buffer
end

def body_parts
Expand Down
2 changes: 1 addition & 1 deletion actionpack/test/template/render_test.rb
Expand Up @@ -9,7 +9,7 @@ module RenderTestCases
def setup_view(paths)
@assigns = { :secret => 'in the sauce' }
@view = ActionView::Base.new(paths, @assigns)
@controller_view = ActionView::Base.for_controller(TestController.new)
@controller_view = TestController.new.view_context

# Reload and register danish language for testing
I18n.reload!
Expand Down

0 comments on commit 71c9337

Please sign in to comment.