From 91320f2a809d3a59efabd9f839fe9b879f4a9b29 Mon Sep 17 00:00:00 2001 From: Damian Janowski Date: Fri, 4 Jul 2008 17:12:30 -0300 Subject: [PATCH 01/74] Add :recursive option to javascript_include_tag and stylesheet_link_tag to be used along with :all. [#480 state:resolved] Signed-off-by: Pratik Naik --- actionpack/CHANGELOG | 2 + .../action_view/helpers/asset_tag_helper.rb | 56 ++++++++++++++----- .../public/javascripts/subdir/subdir.js | 1 + .../public/stylesheets/subdir/subdir.css | 1 + .../test/template/asset_tag_helper_test.rb | 43 ++++++++++++++ 5 files changed, 89 insertions(+), 14 deletions(-) create mode 100644 actionpack/test/fixtures/public/javascripts/subdir/subdir.js create mode 100644 actionpack/test/fixtures/public/stylesheets/subdir/subdir.css diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG index f86a3b21fb..5499cc56ae 100644 --- a/actionpack/CHANGELOG +++ b/actionpack/CHANGELOG @@ -1,5 +1,7 @@ *Edge* +* Add :recursive option to javascript_include_tag and stylesheet_link_tag to be used along with :all. #480 [Damian Janowski] + * Disable the Accept header by default [Michael Koziarski] The accept header is poorly implemented by browsers and causes strange diff --git a/actionpack/lib/action_view/helpers/asset_tag_helper.rb b/actionpack/lib/action_view/helpers/asset_tag_helper.rb index 0122de47af..bf13945844 100644 --- a/actionpack/lib/action_view/helpers/asset_tag_helper.rb +++ b/actionpack/lib/action_view/helpers/asset_tag_helper.rb @@ -209,6 +209,10 @@ module ActionView # Note that the default javascript files will be included first. So Prototype and Scriptaculous are available to # all subsequently included files. # + # If you want Rails to search in all the subdirectories under javascripts, you should explicitly set :recursive: + # + # javascript_include_tag :all, :recursive => true + # # == Caching multiple javascripts into one # # You can also cache multiple javascripts into one file, which requires less HTTP connections to download and can better be @@ -235,18 +239,23 @@ module ActionView # # javascript_include_tag "prototype", "cart", "checkout", :cache => "shop" # when ActionController::Base.perform_caching is true => # + # + # The :recursive option is also available for caching: + # + # javascript_include_tag :all, :cache => true, :recursive => true def javascript_include_tag(*sources) options = sources.extract_options!.stringify_keys cache = options.delete("cache") + recursive = options.delete("recursive") if ActionController::Base.perform_caching && cache joined_javascript_name = (cache == true ? "all" : cache) + ".js" joined_javascript_path = File.join(JAVASCRIPTS_DIR, joined_javascript_name) - write_asset_file_contents(joined_javascript_path, compute_javascript_paths(sources)) unless File.exists?(joined_javascript_path) + write_asset_file_contents(joined_javascript_path, compute_javascript_paths(sources, recursive)) unless File.exists?(joined_javascript_path) javascript_src_tag(joined_javascript_name, options) else - expand_javascript_sources(sources).collect { |source| javascript_src_tag(source, options) }.join("\n") + expand_javascript_sources(sources, recursive).collect { |source| javascript_src_tag(source, options) }.join("\n") end end @@ -332,13 +341,17 @@ module ActionView # # # - # You can also include all styles in the stylesheet directory using :all as the source: + # You can also include all styles in the stylesheets directory using :all as the source: # # stylesheet_link_tag :all # => # # # # + # If you want Rails to search in all the subdirectories under stylesheets, you should explicitly set :recursive: + # + # stylesheet_link_tag :all, :recursive => true + # # == Caching multiple stylesheets into one # # You can also cache multiple stylesheets into one file, which requires less HTTP connections and can better be @@ -362,18 +375,23 @@ module ActionView # # stylesheet_link_tag "shop", "cart", "checkout", :cache => "payment" # when ActionController::Base.perform_caching is true => # + # + # The :recursive option is also available for caching: + # + # stylesheet_link_tag :all, :cache => true, :recursive => true def stylesheet_link_tag(*sources) options = sources.extract_options!.stringify_keys cache = options.delete("cache") + recursive = options.delete("recursive") if ActionController::Base.perform_caching && cache joined_stylesheet_name = (cache == true ? "all" : cache) + ".css" joined_stylesheet_path = File.join(STYLESHEETS_DIR, joined_stylesheet_name) - write_asset_file_contents(joined_stylesheet_path, compute_stylesheet_paths(sources)) unless File.exists?(joined_stylesheet_path) + write_asset_file_contents(joined_stylesheet_path, compute_stylesheet_paths(sources, recursive)) unless File.exists?(joined_stylesheet_path) stylesheet_tag(joined_stylesheet_name, options) else - expand_stylesheet_sources(sources).collect { |source| stylesheet_tag(source, options) }.join("\n") + expand_stylesheet_sources(sources, recursive).collect { |source| stylesheet_tag(source, options) }.join("\n") end end @@ -556,18 +574,19 @@ module ActionView tag("link", { "rel" => "stylesheet", "type" => Mime::CSS, "media" => "screen", "href" => html_escape(path_to_stylesheet(source)) }.merge(options), false, false) end - def compute_javascript_paths(sources) - expand_javascript_sources(sources).collect { |source| compute_public_path(source, 'javascripts', 'js', false) } + def compute_javascript_paths(*args) + expand_javascript_sources(*args).collect { |source| compute_public_path(source, 'javascripts', 'js', false) } end - def compute_stylesheet_paths(sources) - expand_stylesheet_sources(sources).collect { |source| compute_public_path(source, 'stylesheets', 'css', false) } + def compute_stylesheet_paths(*args) + expand_stylesheet_sources(*args).collect { |source| compute_public_path(source, 'stylesheets', 'css', false) } end - def expand_javascript_sources(sources) + def expand_javascript_sources(sources, recursive = false) if sources.include?(:all) - all_javascript_files = Dir[File.join(JAVASCRIPTS_DIR, '*.js')].collect { |file| File.basename(file).gsub(/\.\w+$/, '') }.sort - @@all_javascript_sources ||= ((determine_source(:defaults, @@javascript_expansions).dup & all_javascript_files) + all_javascript_files).uniq + all_javascript_files = collect_asset_files(JAVASCRIPTS_DIR, ('**' if recursive), '*.js') + @@all_javascript_sources ||= {} + @@all_javascript_sources[recursive] ||= ((determine_source(:defaults, @@javascript_expansions).dup & all_javascript_files) + all_javascript_files).uniq else expanded_sources = sources.collect do |source| determine_source(source, @@javascript_expansions) @@ -577,9 +596,10 @@ module ActionView end end - def expand_stylesheet_sources(sources) + def expand_stylesheet_sources(sources, recursive) if sources.first == :all - @@all_stylesheet_sources ||= Dir[File.join(STYLESHEETS_DIR, '*.css')].collect { |file| File.basename(file).gsub(/\.\w+$/, '') }.sort + @@all_stylesheet_sources ||= {} + @@all_stylesheet_sources[recursive] ||= collect_asset_files(STYLESHEETS_DIR, ('**' if recursive), '*.css') else sources.collect do |source| determine_source(source, @@stylesheet_expansions) @@ -604,6 +624,14 @@ module ActionView FileUtils.mkdir_p(File.dirname(joined_asset_path)) File.open(joined_asset_path, "w+") { |cache| cache.write(join_asset_file_contents(asset_paths)) } end + + def collect_asset_files(*path) + dir = path.first + + Dir[File.join(*path.compact)].collect do |file| + file[-(file.size - dir.size - 1)..-1].sub(/\.\w+$/, '') + end.sort + end end end end diff --git a/actionpack/test/fixtures/public/javascripts/subdir/subdir.js b/actionpack/test/fixtures/public/javascripts/subdir/subdir.js new file mode 100644 index 0000000000..9d23a67aa1 --- /dev/null +++ b/actionpack/test/fixtures/public/javascripts/subdir/subdir.js @@ -0,0 +1 @@ +// subdir js diff --git a/actionpack/test/fixtures/public/stylesheets/subdir/subdir.css b/actionpack/test/fixtures/public/stylesheets/subdir/subdir.css new file mode 100644 index 0000000000..241152a905 --- /dev/null +++ b/actionpack/test/fixtures/public/stylesheets/subdir/subdir.css @@ -0,0 +1 @@ +/* subdir.css */ diff --git a/actionpack/test/template/asset_tag_helper_test.rb b/actionpack/test/template/asset_tag_helper_test.rb index 4a8117a88a..020e112fd0 100644 --- a/actionpack/test/template/asset_tag_helper_test.rb +++ b/actionpack/test/template/asset_tag_helper_test.rb @@ -83,6 +83,7 @@ class AssetTagHelperTest < ActionView::TestCase %(javascript_include_tag("common.javascript", "/elsewhere/cools")) => %(\n), %(javascript_include_tag(:defaults)) => %(\n\n\n\n), %(javascript_include_tag(:all)) => %(\n\n\n\n\n\n\n), + %(javascript_include_tag(:all, :recursive => true)) => %(\n\n\n\n\n\n\n\n), %(javascript_include_tag(:defaults, "test")) => %(\n\n\n\n\n), %(javascript_include_tag("test", :defaults)) => %(\n\n\n\n\n) } @@ -108,6 +109,7 @@ class AssetTagHelperTest < ActionView::TestCase %(stylesheet_link_tag("dir/file")) => %(), %(stylesheet_link_tag("style", :media => "all")) => %(), %(stylesheet_link_tag(:all)) => %(\n\n), + %(stylesheet_link_tag(:all, :recursive => true)) => %(\n\n\n), %(stylesheet_link_tag(:all, :media => "all")) => %(\n\n), %(stylesheet_link_tag("random.styles", "/css/stylish")) => %(\n), %(stylesheet_link_tag("http://www.example.com/styles/style")) => %() @@ -343,6 +345,27 @@ class AssetTagHelperTest < ActionView::TestCase FileUtils.rm_f(File.join(ActionView::Helpers::AssetTagHelper::JAVASCRIPTS_DIR, 'cache', 'money.js')) end + def test_caching_javascript_include_tag_with_all_and_recursive_puts_defaults_at_the_start_of_the_file + ENV["RAILS_ASSET_ID"] = "" + ActionController::Base.asset_host = 'http://a0.example.com' + ActionController::Base.perform_caching = true + + assert_dom_equal( + %(), + javascript_include_tag(:all, :cache => "combined", :recursive => true) + ) + + assert File.exist?(File.join(ActionView::Helpers::AssetTagHelper::JAVASCRIPTS_DIR, 'combined.js')) + + assert_equal( + %(// prototype js\n\n// effects js\n\n// dragdrop js\n\n// controls js\n\n// application js\n\n// bank js\n\n// robber js\n\n// subdir js\n\n\n// version.1.0 js), + IO.read(File.join(ActionView::Helpers::AssetTagHelper::JAVASCRIPTS_DIR, 'combined.js')) + ) + + ensure + FileUtils.rm_f(File.join(ActionView::Helpers::AssetTagHelper::JAVASCRIPTS_DIR, 'combined.js')) + end + def test_caching_javascript_include_tag_with_all_puts_defaults_at_the_start_of_the_file ENV["RAILS_ASSET_ID"] = "" ActionController::Base.asset_host = 'http://a0.example.com' @@ -373,6 +396,11 @@ class AssetTagHelperTest < ActionView::TestCase javascript_include_tag(:all, :cache => true) ) + assert_dom_equal( + %(\n\n\n\n\n\n\n\n), + javascript_include_tag(:all, :cache => true, :recursive => true) + ) + assert !File.exist?(File.join(ActionView::Helpers::AssetTagHelper::JAVASCRIPTS_DIR, 'all.js')) assert_dom_equal( @@ -380,6 +408,11 @@ class AssetTagHelperTest < ActionView::TestCase javascript_include_tag(:all, :cache => "money") ) + assert_dom_equal( + %(\n\n\n\n\n\n\n\n), + javascript_include_tag(:all, :cache => "money", :recursive => true) + ) + assert !File.exist?(File.join(ActionView::Helpers::AssetTagHelper::JAVASCRIPTS_DIR, 'money.js')) end @@ -432,6 +465,11 @@ class AssetTagHelperTest < ActionView::TestCase stylesheet_link_tag(:all, :cache => true) ) + assert_dom_equal( + %(\n\n\n), + stylesheet_link_tag(:all, :cache => true, :recursive => true) + ) + assert !File.exist?(File.join(ActionView::Helpers::AssetTagHelper::STYLESHEETS_DIR, 'all.css')) assert_dom_equal( @@ -439,6 +477,11 @@ class AssetTagHelperTest < ActionView::TestCase stylesheet_link_tag(:all, :cache => "money") ) + assert_dom_equal( + %(\n\n\n), + stylesheet_link_tag(:all, :cache => "money", :recursive => true) + ) + assert !File.exist?(File.join(ActionView::Helpers::AssetTagHelper::STYLESHEETS_DIR, 'money.css')) end end From 2b4eb586efa240dd985d8b5fe918084ab17fae2b Mon Sep 17 00:00:00 2001 From: "Hongli Lai (Phusion" Date: Wed, 9 Jul 2008 13:32:40 +0200 Subject: [PATCH 02/74] Plugin locator: sort directory listing because we can't assume that the OS will do it for us. This fixes some unit test failures. --- railties/lib/rails/plugin/locator.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/railties/lib/rails/plugin/locator.rb b/railties/lib/rails/plugin/locator.rb index 79c07fccd1..678b295dc9 100644 --- a/railties/lib/rails/plugin/locator.rb +++ b/railties/lib/rails/plugin/locator.rb @@ -63,7 +63,7 @@ module Rails # => # def locate_plugins_under(base_path) - Dir.glob(File.join(base_path, '*')).inject([]) do |plugins, path| + Dir.glob(File.join(base_path, '*')).sort.inject([]) do |plugins, path| if plugin = create_plugin(path) plugins << plugin elsif File.directory?(path) From 96708af6a58a48c2324a3bf8d34232bc29b398c9 Mon Sep 17 00:00:00 2001 From: Cheah Chu Yeow Date: Mon, 23 Jun 2008 20:51:38 +0800 Subject: [PATCH 03/74] Ensure url_for(nil) falls back to url_for({}). [#472 state:resolved] Signed-off-by: Pratik Naik --- actionpack/lib/action_controller/base.rb | 3 ++- actionpack/lib/action_view/helpers/url_helper.rb | 6 ++---- actionpack/test/template/url_helper_test.rb | 11 ++++++++++- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index c28e9005cf..f6d5491100 100755 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -613,7 +613,8 @@ module ActionController #:nodoc: # # This takes the current URL as is and only exchanges the action. In contrast, url_for :action => 'print' # would have slashed-off the path components after the changed action. - def url_for(options = {}) #:doc: + def url_for(options = {}) + options ||= {} case options when String options diff --git a/actionpack/lib/action_view/helpers/url_helper.rb b/actionpack/lib/action_view/helpers/url_helper.rb index d5b7255642..f6a1f271f0 100644 --- a/actionpack/lib/action_view/helpers/url_helper.rb +++ b/actionpack/lib/action_view/helpers/url_helper.rb @@ -63,17 +63,15 @@ module ActionView # # calls @workshop.to_s # # => /workshops/5 def url_for(options = {}) + options ||= {} case options when Hash - show_path = options[:host].nil? ? true : false - options = { :only_path => show_path }.update(options.symbolize_keys) + options = { :only_path => options[:host].nil? }.update(options.symbolize_keys) escape = options.key?(:escape) ? options.delete(:escape) : true url = @controller.send(:url_for, options) when String escape = true url = options - when NilClass - url = @controller.send(:url_for, nil) else escape = false url = polymorphic_path(options) diff --git a/actionpack/test/template/url_helper_test.rb b/actionpack/test/template/url_helper_test.rb index 3d5f7eae11..8e43629522 100644 --- a/actionpack/test/template/url_helper_test.rb +++ b/actionpack/test/template/url_helper_test.rb @@ -313,6 +313,10 @@ class UrlHelperWithControllerTest < ActionView::TestCase render :inline => "<%= show_named_route_#{params[:kind]} %>" end + def nil_url_for + render :inline => '<%= url_for(nil) %>' + end + def rescue_action(e) raise e end end @@ -329,7 +333,7 @@ class UrlHelperWithControllerTest < ActionView::TestCase assert_equal '/url_helper_with_controller/show_url_for', @response.body end - def test_named_route_shows_host_and_path + def test_named_route_url_shows_host_and_path with_url_helper_routing do get :show_named_route, :kind => 'url' assert_equal 'http://test.host/url_helper_with_controller/show_named_route', @response.body @@ -343,6 +347,11 @@ class UrlHelperWithControllerTest < ActionView::TestCase end end + def test_url_for_nil_returns_current_path + get :nil_url_for + assert_equal '/url_helper_with_controller/nil_url_for', @response.body + end + protected def with_url_helper_routing with_routing do |set| From 124d1016fa212c008e33853912493fa9ac15d086 Mon Sep 17 00:00:00 2001 From: Chris Cherry Date: Thu, 5 Jun 2008 23:26:35 -0700 Subject: [PATCH 04/74] Allow Infinity (1.0/0.0) to pass validates_numericality_of. [#354 state:resolved] Signed-off-by: Pratik Naik --- activerecord/lib/active_record/validations.rb | 2 +- activerecord/test/cases/validations_test.rb | 11 ++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/activerecord/lib/active_record/validations.rb b/activerecord/lib/active_record/validations.rb index 741649f764..1035308aa5 100755 --- a/activerecord/lib/active_record/validations.rb +++ b/activerecord/lib/active_record/validations.rb @@ -854,7 +854,7 @@ module ActiveRecord raw_value = raw_value.to_i else begin - raw_value = Kernel.Float(raw_value.to_s) + raw_value = Kernel.Float(raw_value) rescue ArgumentError, TypeError record.errors.add(attr_name, configuration[:message] || ActiveRecord::Errors.default_error_messages[:not_a_number]) next diff --git a/activerecord/test/cases/validations_test.rb b/activerecord/test/cases/validations_test.rb index 60566cd064..0742e2c632 100755 --- a/activerecord/test/cases/validations_test.rb +++ b/activerecord/test/cases/validations_test.rb @@ -1391,6 +1391,7 @@ class ValidatesNumericalityTest < ActiveRecord::TestCase INTEGERS = [0, 10, -10] + INTEGER_STRINGS BIGDECIMAL = BIGDECIMAL_STRINGS.collect! { |bd| BigDecimal.new(bd) } JUNK = ["not a number", "42 not a number", "0xdeadbeef", "00-1", "--3", "+-3", "+3-1", "-+019.0", "12.12.13.12", "123\nnot a number"] + INFINITY = [1.0/0.0] def setup Topic.instance_variable_set("@validate_callbacks", ActiveSupport::Callbacks::CallbackChain.new) @@ -1402,27 +1403,27 @@ class ValidatesNumericalityTest < ActiveRecord::TestCase Topic.validates_numericality_of :approved invalid!(NIL + BLANK + JUNK) - valid!(FLOATS + INTEGERS + BIGDECIMAL) + valid!(FLOATS + INTEGERS + BIGDECIMAL + INFINITY) end def test_validates_numericality_of_with_nil_allowed Topic.validates_numericality_of :approved, :allow_nil => true invalid!(BLANK + JUNK) - valid!(NIL + FLOATS + INTEGERS + BIGDECIMAL) + valid!(NIL + FLOATS + INTEGERS + BIGDECIMAL + INFINITY) end def test_validates_numericality_of_with_integer_only Topic.validates_numericality_of :approved, :only_integer => true - invalid!(NIL + BLANK + JUNK + FLOATS + BIGDECIMAL) + invalid!(NIL + BLANK + JUNK + FLOATS + BIGDECIMAL + INFINITY) valid!(INTEGERS) end def test_validates_numericality_of_with_integer_only_and_nil_allowed Topic.validates_numericality_of :approved, :only_integer => true, :allow_nil => true - invalid!(BLANK + JUNK + FLOATS + BIGDECIMAL) + invalid!(BLANK + JUNK + FLOATS + BIGDECIMAL + INFINITY) valid!(NIL + INTEGERS) end @@ -1443,7 +1444,7 @@ class ValidatesNumericalityTest < ActiveRecord::TestCase def test_validates_numericality_with_equal_to Topic.validates_numericality_of :approved, :equal_to => 10 - invalid!([-10, 11], 'must be equal to 10') + invalid!([-10, 11] + INFINITY, 'must be equal to 10') valid!([10]) end From 84af99e78dbf65b7faa92313acf8457cb0c2b510 Mon Sep 17 00:00:00 2001 From: Daniel Guettler Date: Wed, 9 Jul 2008 14:08:04 +0100 Subject: [PATCH 05/74] Ensure NamedScope#build/create/create!/new works as expected when named scope has hash conditions. [Daniel Guettler, Pratik Naik] [#419 state:resolved] Signed-off-by: Pratik Naik --- activerecord/lib/active_record/named_scope.rb | 3 ++- activerecord/test/cases/named_scope_test.rb | 26 +++++++++++++++++++ activerecord/test/models/topic.rb | 2 ++ 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/named_scope.rb b/activerecord/lib/active_record/named_scope.rb index eac61e9e43..080e3d0f5e 100644 --- a/activerecord/lib/active_record/named_scope.rb +++ b/activerecord/lib/active_record/named_scope.rb @@ -150,7 +150,8 @@ module ActiveRecord if scopes.include?(method) scopes[method].call(self, *args) else - with_scope :find => proxy_options do + with_scope :find => proxy_options, :create => proxy_options[:conditions].is_a?(Hash) ? proxy_options[:conditions] : {} do + method = :new if method == :build proxy_scope.send(method, *args, &block) end end diff --git a/activerecord/test/cases/named_scope_test.rb b/activerecord/test/cases/named_scope_test.rb index 7d73541ee1..0c1eb23428 100644 --- a/activerecord/test/cases/named_scope_test.rb +++ b/activerecord/test/cases/named_scope_test.rb @@ -183,4 +183,30 @@ class NamedScopeTest < ActiveRecord::TestCase topics.empty? # use loaded (no query) end end + + def test_should_build_with_proxy_options + topic = Topic.approved.build({}) + assert topic.approved + end + + def test_should_build_new_with_proxy_options + topic = Topic.approved.new + assert topic.approved + end + + def test_should_create_with_proxy_options + topic = Topic.approved.create({}) + assert topic.approved + end + + def test_should_create_with_bang_with_proxy_options + topic = Topic.approved.create!({}) + assert topic.approved + end + + def test_should_build_with_proxy_options_chained + topic = Topic.approved.by_lifo.build({}) + assert topic.approved + assert_equal 'lifo', topic.author_name + end end diff --git a/activerecord/test/models/topic.rb b/activerecord/test/models/topic.rb index 47b2eec938..39ca1bf42a 100755 --- a/activerecord/test/models/topic.rb +++ b/activerecord/test/models/topic.rb @@ -4,6 +4,8 @@ class Topic < ActiveRecord::Base { :conditions => ['written_on < ?', time] } } named_scope :approved, :conditions => {:approved => true} + named_scope :by_lifo, :conditions => {:author_name => 'lifo'} + named_scope :approved_as_hash_condition, :conditions => {:topics => {:approved => true}} named_scope 'approved_as_string', :conditions => {:approved => true} named_scope :replied, :conditions => ['replies_count > 0'] From 6b61e95dc8d717b4500ab623816863c0bb707e2b Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Wed, 9 Jul 2008 09:13:24 -0500 Subject: [PATCH 06/74] Changed PrototypeHelper#submit_to_remote to PrototypeHelper#button_to_remote to stay consistent with link_to_remote (submit_to_remote still works as an alias) (clemens) [#8994 status:closed] --- actionpack/CHANGELOG | 2 ++ actionpack/lib/action_view/helpers/prototype_helper.rb | 9 ++++++--- actionpack/test/template/prototype_helper_test.rb | 4 ++-- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG index 5499cc56ae..20e9da6338 100644 --- a/actionpack/CHANGELOG +++ b/actionpack/CHANGELOG @@ -1,5 +1,7 @@ *Edge* +* Changed PrototypeHelper#submit_to_remote to PrototypeHelper#button_to_remote to stay consistent with link_to_remote (submit_to_remote still works as an alias) #8994 [clemens] + * Add :recursive option to javascript_include_tag and stylesheet_link_tag to be used along with :all. #480 [Damian Janowski] * Disable the Accept header by default [Michael Koziarski] diff --git a/actionpack/lib/action_view/helpers/prototype_helper.rb b/actionpack/lib/action_view/helpers/prototype_helper.rb index a7c3b9ddc3..d0c281c803 100644 --- a/actionpack/lib/action_view/helpers/prototype_helper.rb +++ b/actionpack/lib/action_view/helpers/prototype_helper.rb @@ -397,7 +397,7 @@ module ActionView # # Generates: - # <%= submit_to_remote 'create_btn', 'Create', :url => { :action => 'create' } %> + # <%= button_to_remote 'create_btn', 'Create', :url => { :action => 'create' } %> # # # Submit to the remote action update and update the DIV succeed or fail based # # on the success or failure of the request @@ -405,11 +405,13 @@ module ActionView # # Generates: - # <%= submit_to_remote 'update_btn', 'Update', :url => { :action => 'update' }, + # <%= button_to_remote 'update_btn', 'Update', :url => { :action => 'update' }, # :update => { :success => "succeed", :failure => "fail" } # # options argument is the same as in form_remote_tag. - def submit_to_remote(name, value, options = {}) + # + # Note: This method used to be called submit_to_remote, but that's now just an alias for button_to_remote + def button_to_remote(name, value, options = {}) options[:with] ||= 'Form.serialize(this.form)' options[:html] ||= {} @@ -420,6 +422,7 @@ module ActionView tag("input", options[:html], false) end + alias_method :submit_to_remote, :button_to_remote # Returns 'eval(request.responseText)' which is the JavaScript function # that +form_remote_tag+ can call in :complete to evaluate a multiple diff --git a/actionpack/test/template/prototype_helper_test.rb b/actionpack/test/template/prototype_helper_test.rb index 60b83b476d..1d9bc5eb9b 100644 --- a/actionpack/test/template/prototype_helper_test.rb +++ b/actionpack/test/template/prototype_helper_test.rb @@ -201,9 +201,9 @@ class PrototypeHelperTest < PrototypeHelperBaseTest end - def test_submit_to_remote + def test_button_to_remote assert_dom_equal %(), - submit_to_remote("More beer!", 1_000_000, :update => "empty_bottle") + button_to_remote("More beer!", 1_000_000, :update => "empty_bottle") end def test_observe_field From 4ce9931f4f30045b2975328e7d42a02188e35079 Mon Sep 17 00:00:00 2001 From: Michael Koziarski Date: Wed, 9 Jul 2008 18:35:35 +0200 Subject: [PATCH 07/74] Reenable the use of the Accept header to give people a chance to update their applications and provide feedback. --- actionpack/CHANGELOG | 9 +++++---- actionpack/lib/action_controller/base.rb | 6 +++--- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG index 20e9da6338..89cd7c64af 100644 --- a/actionpack/CHANGELOG +++ b/actionpack/CHANGELOG @@ -4,15 +4,16 @@ * Add :recursive option to javascript_include_tag and stylesheet_link_tag to be used along with :all. #480 [Damian Janowski] -* Disable the Accept header by default [Michael Koziarski] +* Allow users to disable the use of the Accept header [Michael Koziarski] The accept header is poorly implemented by browsers and causes strange errors when used on public sites where crawlers make requests too. You - should use formatted urls (e.g. /people/1.xml) to support API clients. + can use formatted urls (e.g. /people/1.xml) to support API clients in a + much simpler way. - Alternatively to re-enable it you need to set: + To disable the header you need to set: - config.action_controller.use_accept_header = true + config.action_controller.use_accept_header = false * Do not stat template files in production mode before rendering. You will no longer be able to modify templates in production mode without restarting the server [Josh Peek] diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index f6d5491100..dd50f17c1c 100755 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -343,12 +343,12 @@ module ActionController #:nodoc: # Indicates whether the response format should be determined by examining the Accept HTTP header, # or by using the simpler params + ajax rules. # - # If this is set to +true+ then +respond_to+ and +Request#format+ will take the Accept header into - # account. If it is set to false (the default) then the request format will be determined solely + # If this is set to +true+ (the default) then +respond_to+ and +Request#format+ will take the Accept + # header into account. If it is set to false then the request format will be determined solely # by examining params[:format]. If params format is missing, the format will be either HTML or # Javascript depending on whether the request is an AJAX request. cattr_accessor :use_accept_header - self.use_accept_header = false + self.use_accept_header = true # Controls whether request forgergy protection is turned on or not. Turned off by default only in test mode. class_inheritable_accessor :allow_forgery_protection From 350faf14e80740a440ab16c7130c5262d603d283 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Mon, 7 Jul 2008 11:02:15 -0700 Subject: [PATCH 08/74] Pass caller to concat deprecation warning --- actionpack/lib/action_view/helpers/text_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionpack/lib/action_view/helpers/text_helper.rb b/actionpack/lib/action_view/helpers/text_helper.rb index a6c48737e9..3e3452b615 100644 --- a/actionpack/lib/action_view/helpers/text_helper.rb +++ b/actionpack/lib/action_view/helpers/text_helper.rb @@ -27,7 +27,7 @@ module ActionView # %> def concat(string, unused_binding = nil) if unused_binding - ActiveSupport::Deprecation.warn("The binding argument of #concat is no longer needed. Please remove it from your views and helpers.") + ActiveSupport::Deprecation.warn("The binding argument of #concat is no longer needed. Please remove it from your views and helpers.", caller) end output_buffer << string From 4354aa36fb0b94f3750256441054d42db9900bf5 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Mon, 7 Jul 2008 11:57:53 -0700 Subject: [PATCH 09/74] Rendering default template for missing actions works with non-word characters in action name --- actionpack/lib/action_view/template_file.rb | 2 +- actionpack/test/controller/new_render_test.rb | 5 +++++ actionpack/test/fixtures/test/hyphen-ated.erb | 1 + 3 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 actionpack/test/fixtures/test/hyphen-ated.erb diff --git a/actionpack/lib/action_view/template_file.rb b/actionpack/lib/action_view/template_file.rb index c38e8ed122..0aa16b5e70 100644 --- a/actionpack/lib/action_view/template_file.rb +++ b/actionpack/lib/action_view/template_file.rb @@ -74,7 +74,7 @@ module ActionView #:nodoc: # Returns file split into an array # [base_path, name, format, extension] def split(file) - if m = file.match(/^(.*\/)?(\w+)\.?(\w+)?\.?(\w+)?\.?(\w+)?$/) + if m = file.match(/^(.*\/)?([^\.]+)\.?(\w+)?\.?(\w+)?\.?(\w+)?$/) if m[5] # Mulipart formats [m[1], m[2], "#{m[3]}.#{m[4]}", m[5]] elsif m[4] # Single format diff --git a/actionpack/test/controller/new_render_test.rb b/actionpack/test/controller/new_render_test.rb index b2691d981b..b4dc2bb4dc 100644 --- a/actionpack/test/controller/new_render_test.rb +++ b/actionpack/test/controller/new_render_test.rb @@ -489,6 +489,11 @@ class NewRenderTest < Test::Unit::TestCase assert_equal "Hello world!", @response.body end + def test_renders_default_template_for_missing_action + get :'hyphen-ated' + assert_template 'test/hyphen-ated' + end + def test_do_with_render get :render_hello_world assert_template "test/hello_world" diff --git a/actionpack/test/fixtures/test/hyphen-ated.erb b/actionpack/test/fixtures/test/hyphen-ated.erb new file mode 100644 index 0000000000..cd0875583a --- /dev/null +++ b/actionpack/test/fixtures/test/hyphen-ated.erb @@ -0,0 +1 @@ +Hello world! From 7dc10478e53ca4f36951c85aab4885e5de6e11cb Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Mon, 7 Jul 2008 12:42:51 -0700 Subject: [PATCH 10/74] Use ActionController::Base.logger to report template compilation errors since there is no AV::Base.logger --- .../lib/action_view/template_handlers/compilable.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/actionpack/lib/action_view/template_handlers/compilable.rb b/actionpack/lib/action_view/template_handlers/compilable.rb index 256d8da031..9b9255579c 100644 --- a/actionpack/lib/action_view/template_handlers/compilable.rb +++ b/actionpack/lib/action_view/template_handlers/compilable.rb @@ -37,10 +37,10 @@ module ActionView file_name = template.filename || 'compiled-template' ActionView::Base::CompiledTemplates.module_eval(source, file_name, 0) rescue Exception => e # errors from template code - if Base.logger - Base.logger.debug "ERROR: compiling #{template.method} RAISED #{e}" - Base.logger.debug "Function body: #{source}" - Base.logger.debug "Backtrace: #{e.backtrace.join("\n")}" + if logger = ActionController::Base.logger + logger.debug "ERROR: compiling #{template.method} RAISED #{e}" + logger.debug "Function body: #{source}" + logger.debug "Backtrace: #{e.backtrace.join("\n")}" end raise ActionView::TemplateError.new(template, @view.assigns, e) From ee6bbcb6ae4488a6ab556236dfa1cf7358378dcd Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Mon, 7 Jul 2008 12:43:43 -0700 Subject: [PATCH 11/74] Put a newline rather than a semicolon at the end of RJS source to avoid parse errors with embedded heredocs --- actionpack/lib/action_view/template_handlers/rjs.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionpack/lib/action_view/template_handlers/rjs.rb b/actionpack/lib/action_view/template_handlers/rjs.rb index 31ce598c46..3892bf1d6e 100644 --- a/actionpack/lib/action_view/template_handlers/rjs.rb +++ b/actionpack/lib/action_view/template_handlers/rjs.rb @@ -5,7 +5,7 @@ module ActionView def compile(template) "controller.response.content_type ||= Mime::JS;" + - "update_page do |page|;#{template.source};end" + "update_page do |page|;#{template.source}\nend" end def cache_fragment(block, name = {}, options = nil) #:nodoc: From a6d0ae28e3ceef909f78afc27b936bca1e4b1021 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Mon, 7 Jul 2008 13:09:56 -0700 Subject: [PATCH 12/74] Fix teardown method name typo --- actionpack/test/controller/content_type_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionpack/test/controller/content_type_test.rb b/actionpack/test/controller/content_type_test.rb index 8f55974172..33aa4e49ee 100644 --- a/actionpack/test/controller/content_type_test.rb +++ b/actionpack/test/controller/content_type_test.rb @@ -124,7 +124,7 @@ class AcceptBasedContentTypeTest < ActionController::TestCase ActionController::Base.use_accept_header = true end - def tear_down + def teardown ActionController::Base.use_accept_header = false end From f82bd31cb013cfba5bf3f5cad7356070fcefcc22 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Tue, 8 Jul 2008 12:35:03 -0700 Subject: [PATCH 13/74] Request#accepts special-cases a single mime type --- actionpack/lib/action_controller/request.rb | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/actionpack/lib/action_controller/request.rb b/actionpack/lib/action_controller/request.rb index c76a93f7a1..7791e0696e 100755 --- a/actionpack/lib/action_controller/request.rb +++ b/actionpack/lib/action_controller/request.rb @@ -82,10 +82,16 @@ module ActionController # Returns the accepted MIME type for the request def accepts @accepts ||= - if @env['HTTP_ACCEPT'].to_s.strip.empty? - [ content_type, Mime::ALL ].compact # make sure content_type being nil is not included - else - Mime::Type.parse(@env['HTTP_ACCEPT']) + begin + header = @env['HTTP_ACCEPT'].to_s.strip + + if header.empty? + [content_type, Mime::ALL].compact + elsif header !~ /,/ + [Mime::Type.lookup(header)].compact + else + Mime::Type.parse(header) + end end end From 11252e35b1756b025d8778c151f9f9a24057d1b1 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Tue, 8 Jul 2008 17:32:06 -0700 Subject: [PATCH 14/74] Boolean type casting creates fewer objects --- .../connection_adapters/abstract/schema_definitions.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb index 2c03de0f17..d4c8a80448 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb @@ -138,7 +138,7 @@ module ActiveRecord if value == true || value == false value else - %w(true t 1).include?(value.to_s.downcase) + !(value.to_s !~ /\A(?:1|t|true)\Z/i) end end From ce4a1bb8538bd7cc5ee3cbf1156dc587482a7839 Mon Sep 17 00:00:00 2001 From: Cheah Chu Yeow Date: Thu, 26 Jun 2008 10:21:53 +0800 Subject: [PATCH 15/74] Remove some Symbol#to_proc usage in runtime code. [#484 state:resolved] --- actionpack/lib/action_controller/base.rb | 12 ++++++------ actionpack/lib/action_controller/filters.rb | 16 ++++++++++++---- activerecord/lib/active_record/associations.rb | 4 ++-- .../associations/association_collection.rb | 2 +- .../associations/has_many_association.rb | 4 ++-- .../core_ext/module/introspection.rb | 2 +- .../core_ext/object/instance_variables.rb | 2 +- activesupport/lib/active_support/dependencies.rb | 4 ++-- 8 files changed, 27 insertions(+), 19 deletions(-) diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index dd50f17c1c..9d9ff527c2 100755 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -412,7 +412,7 @@ module ActionController #:nodoc: # More methods can be hidden using hide_actions. def hidden_actions unless read_inheritable_attribute(:hidden_actions) - write_inheritable_attribute(:hidden_actions, ActionController::Base.public_instance_methods.map(&:to_s)) + write_inheritable_attribute(:hidden_actions, ActionController::Base.public_instance_methods.map { |m| m.to_s }) end read_inheritable_attribute(:hidden_actions) @@ -420,12 +420,12 @@ module ActionController #:nodoc: # Hide each of the given methods from being callable as actions. def hide_action(*names) - write_inheritable_attribute(:hidden_actions, hidden_actions | names.map(&:to_s)) + write_inheritable_attribute(:hidden_actions, hidden_actions | names.map { |name| name.to_s }) end - ## View load paths determine the bases from which template references can be made. So a call to - ## render("test/template") will be looked up in the view load paths array and the closest match will be - ## returned. + # View load paths determine the bases from which template references can be made. So a call to + # render("test/template") will be looked up in the view load paths array and the closest match will be + # returned. def view_paths @view_paths || superclass.view_paths end @@ -1201,7 +1201,7 @@ module ActionController #:nodoc: end def self.action_methods - @action_methods ||= Set.new(public_instance_methods.map(&:to_s)) - hidden_actions + @action_methods ||= Set.new(public_instance_methods.map { |m| m.to_s }) - hidden_actions end def add_variables_to_assigns diff --git a/actionpack/lib/action_controller/filters.rb b/actionpack/lib/action_controller/filters.rb index 155d8b480a..c3f1de971e 100644 --- a/actionpack/lib/action_controller/filters.rb +++ b/actionpack/lib/action_controller/filters.rb @@ -127,9 +127,9 @@ module ActionController #:nodoc: def included_in_action?(controller, options) if options[:only] - Array(options[:only]).map(&:to_s).include?(controller.action_name) + Array(options[:only]).map { |o| o.to_s }.include?(controller.action_name) elsif options[:except] - !Array(options[:except]).map(&:to_s).include?(controller.action_name) + !Array(options[:except]).map { |o| o.to_s }.include?(controller.action_name) else true end @@ -544,13 +544,21 @@ module ActionController #:nodoc: # Returns all the before filters for this class and all its ancestors. # This method returns the actual filter that was assigned in the controller to maintain existing functionality. def before_filters #:nodoc: - filter_chain.select(&:before?).map(&:method) + filters = [] + filter_chain.each do |filter| + filters << filter.method if filter.before? + end + filters end # Returns all the after filters for this class and all its ancestors. # This method returns the actual filter that was assigned in the controller to maintain existing functionality. def after_filters #:nodoc: - filter_chain.select(&:after?).map(&:method) + filters = [] + filter_chain.each do |filter| + filters << filter.method if filter.after? + end + filters end end diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 856d872bce..6931744058 100755 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1145,7 +1145,7 @@ module ActiveRecord end define_method("#{reflection.name.to_s.singularize}_ids") do - send(reflection.name).map(&:id) + send(reflection.name).map { |record| record.id } end end @@ -1490,7 +1490,7 @@ module ActiveRecord sql << " FROM #{connection.quote_table_name table_name} " if is_distinct - sql << distinct_join_associations.collect(&:association_join).join + sql << distinct_join_associations.collect { |assoc| assoc.association_join }.join add_joins!(sql, options, scope) end diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index bbd8af7e76..eb39714909 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -14,7 +14,7 @@ module ActiveRecord # If using a custom finder_sql, scan the entire collection. if @reflection.options[:finder_sql] expects_array = args.first.kind_of?(Array) - ids = args.flatten.compact.uniq.map(&:to_i) + ids = args.flatten.compact.uniq.map { |arg| arg.to_i } if ids.size == 1 id = ids.first diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index cb58f0be15..e6fa15c173 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -61,9 +61,9 @@ module ActiveRecord def delete_records(records) case @reflection.options[:dependent] when :destroy - records.each(&:destroy) + records.each { |r| r.destroy } when :delete_all - @reflection.klass.delete(records.map(&:id)) + @reflection.klass.delete(records.map { |record| record.id }) else ids = quoted_record_ids(records) @reflection.klass.update_all( diff --git a/activesupport/lib/active_support/core_ext/module/introspection.rb b/activesupport/lib/active_support/core_ext/module/introspection.rb index 40bbebb7c4..bb894ec080 100644 --- a/activesupport/lib/active_support/core_ext/module/introspection.rb +++ b/activesupport/lib/active_support/core_ext/module/introspection.rb @@ -70,6 +70,6 @@ class Module # Returns the names of the constants defined locally rather than the # constants themselves. See local_constants. def local_constant_names - local_constants.map(&:to_s) + local_constants.map { |c| c.to_s } end end diff --git a/activesupport/lib/active_support/core_ext/object/instance_variables.rb b/activesupport/lib/active_support/core_ext/object/instance_variables.rb index 9f1d4ed2aa..4ecaab3bbb 100644 --- a/activesupport/lib/active_support/core_ext/object/instance_variables.rb +++ b/activesupport/lib/active_support/core_ext/object/instance_variables.rb @@ -35,7 +35,7 @@ class Object # C.new(0, 1).instance_variable_names # => ["@y", "@x"] if RUBY_VERSION >= '1.9' def instance_variable_names - instance_variables.map(&:to_s) + instance_variables.map { |var| var.to_s } end else alias_method :instance_variable_names, :instance_variables diff --git a/activesupport/lib/active_support/dependencies.rb b/activesupport/lib/active_support/dependencies.rb index d3d9ff9de4..2f3fa72bb4 100644 --- a/activesupport/lib/active_support/dependencies.rb +++ b/activesupport/lib/active_support/dependencies.rb @@ -387,7 +387,7 @@ module ActiveSupport #:nodoc: ensure # Remove the stack frames that we added. if defined?(watch_frames) && ! watch_frames.blank? - frame_ids = watch_frames.collect(&:object_id) + frame_ids = watch_frames.collect { |frame| frame.object_id } constant_watch_stack.delete_if do |watch_frame| frame_ids.include? watch_frame.object_id end @@ -437,7 +437,7 @@ module ActiveSupport #:nodoc: protected def log_call(*args) if defined?(RAILS_DEFAULT_LOGGER) && RAILS_DEFAULT_LOGGER && log_activity - arg_str = args.collect(&:inspect) * ', ' + arg_str = args.collect { |arg| arg.inspect } * ', ' /in `([a-z_\?\!]+)'/ =~ caller(1).first selector = $1 || '' log "called #{selector}(#{arg_str})" From 0ce7fe5308337ae0b0394ae4c18b59cf64e33b38 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Tue, 8 Jul 2008 23:38:18 -0700 Subject: [PATCH 16/74] Don't repeatedly convert only/except options --- actionpack/lib/action_controller/filters.rb | 31 +++++++++++++++++++-- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/actionpack/lib/action_controller/filters.rb b/actionpack/lib/action_controller/filters.rb index c3f1de971e..fc63890d13 100644 --- a/actionpack/lib/action_controller/filters.rb +++ b/actionpack/lib/action_controller/filters.rb @@ -94,7 +94,7 @@ module ActionController #:nodoc: map! do |filter| if filters.include?(filter) new_filter = filter.dup - new_filter.options.merge!(options) + new_filter.update_options!(options) new_filter else filter @@ -104,6 +104,11 @@ module ActionController #:nodoc: end class Filter < ActiveSupport::Callbacks::Callback #:nodoc: + def initialize(kind, method, options = {}) + super + update_options! options + end + def before? self.class == BeforeFilter end @@ -116,6 +121,18 @@ module ActionController #:nodoc: self.class == AroundFilter end + # Make sets of strings from :only/:except options + def update_options!(other) + if other + convert_only_and_except_options_to_sets_of_strings(other) + if other[:skip] + convert_only_and_except_options_to_sets_of_strings(other[:skip]) + end + end + + options.update(other) + end + private def should_not_skip?(controller) if options[:skip] @@ -127,9 +144,9 @@ module ActionController #:nodoc: def included_in_action?(controller, options) if options[:only] - Array(options[:only]).map { |o| o.to_s }.include?(controller.action_name) + options[:only].include?(controller.action_name) elsif options[:except] - !Array(options[:except]).map { |o| o.to_s }.include?(controller.action_name) + !options[:except].include?(controller.action_name) else true end @@ -138,6 +155,14 @@ module ActionController #:nodoc: def should_run_callback?(controller) should_not_skip?(controller) && included_in_action?(controller, options) && super end + + def convert_only_and_except_options_to_sets_of_strings(opts) + [:only, :except].each do |key| + if values = opts[key] + opts[key] = Array(values).map(&:to_s).to_set + end + end + end end class AroundFilter < Filter #:nodoc: From d37e6413366c9a3fafa02c4298a2946dc8327a42 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Wed, 9 Jul 2008 11:30:18 -0700 Subject: [PATCH 17/74] Move accept header parsing shortcut to Mime::Type.parse --- actionpack/lib/action_controller/mime_type.rb | 100 +++++++++--------- actionpack/lib/action_controller/request.rb | 2 - 2 files changed, 52 insertions(+), 50 deletions(-) diff --git a/actionpack/lib/action_controller/mime_type.rb b/actionpack/lib/action_controller/mime_type.rb index fa123f7808..a7215e6ea3 100644 --- a/actionpack/lib/action_controller/mime_type.rb +++ b/actionpack/lib/action_controller/mime_type.rb @@ -72,57 +72,61 @@ module Mime end def parse(accept_header) - # keep track of creation order to keep the subsequent sort stable - list = [] - accept_header.split(/,/).each_with_index do |header, index| - params, q = header.split(/;\s*q=/) - if params - params.strip! - list << AcceptItem.new(index, params, q) unless params.empty? - end - end - list.sort! - - # Take care of the broken text/xml entry by renaming or deleting it - text_xml = list.index("text/xml") - app_xml = list.index(Mime::XML.to_s) - - if text_xml && app_xml - # set the q value to the max of the two - list[app_xml].q = [list[text_xml].q, list[app_xml].q].max - - # make sure app_xml is ahead of text_xml in the list - if app_xml > text_xml - list[app_xml], list[text_xml] = list[text_xml], list[app_xml] - app_xml, text_xml = text_xml, app_xml - end - - # delete text_xml from the list - list.delete_at(text_xml) - - elsif text_xml - list[text_xml].name = Mime::XML.to_s - end - - # Look for more specific XML-based types and sort them ahead of app/xml - - if app_xml - idx = app_xml - app_xml_type = list[app_xml] - - while(idx < list.length) - type = list[idx] - break if type.q < app_xml_type.q - if type.name =~ /\+xml$/ - list[app_xml], list[idx] = list[idx], list[app_xml] - app_xml = idx + if accept_header !~ /,/ + [Mime::Type.lookup(accept_header)] + else + # keep track of creation order to keep the subsequent sort stable + list = [] + accept_header.split(/,/).each_with_index do |header, index| + params, q = header.split(/;\s*q=/) + if params + params.strip! + list << AcceptItem.new(index, params, q) unless params.empty? end - idx += 1 end - end + list.sort! - list.map! { |i| Mime::Type.lookup(i.name) }.uniq! - list + # Take care of the broken text/xml entry by renaming or deleting it + text_xml = list.index("text/xml") + app_xml = list.index(Mime::XML.to_s) + + if text_xml && app_xml + # set the q value to the max of the two + list[app_xml].q = [list[text_xml].q, list[app_xml].q].max + + # make sure app_xml is ahead of text_xml in the list + if app_xml > text_xml + list[app_xml], list[text_xml] = list[text_xml], list[app_xml] + app_xml, text_xml = text_xml, app_xml + end + + # delete text_xml from the list + list.delete_at(text_xml) + + elsif text_xml + list[text_xml].name = Mime::XML.to_s + end + + # Look for more specific XML-based types and sort them ahead of app/xml + + if app_xml + idx = app_xml + app_xml_type = list[app_xml] + + while(idx < list.length) + type = list[idx] + break if type.q < app_xml_type.q + if type.name =~ /\+xml$/ + list[app_xml], list[idx] = list[idx], list[app_xml] + app_xml = idx + end + idx += 1 + end + end + + list.map! { |i| Mime::Type.lookup(i.name) }.uniq! + list + end end end diff --git a/actionpack/lib/action_controller/request.rb b/actionpack/lib/action_controller/request.rb index 7791e0696e..2d9f6c3e6f 100755 --- a/actionpack/lib/action_controller/request.rb +++ b/actionpack/lib/action_controller/request.rb @@ -87,8 +87,6 @@ module ActionController if header.empty? [content_type, Mime::ALL].compact - elsif header !~ /,/ - [Mime::Type.lookup(header)].compact else Mime::Type.parse(header) end From feb08984ea5517db5780a88584929feac1cafb59 Mon Sep 17 00:00:00 2001 From: Clemens Kofler Date: Wed, 9 Jul 2008 21:41:03 +0200 Subject: [PATCH 18/74] Added notes to Routing documentation and routes.rb regarding defaults routes opening the whole application for GET requests Signed-off-by: Michael Koziarski --- actionpack/lib/action_controller/routing.rb | 4 ++++ railties/configs/routes.rb | 2 ++ 2 files changed, 6 insertions(+) diff --git a/actionpack/lib/action_controller/routing.rb b/actionpack/lib/action_controller/routing.rb index 8846dcc504..dfbaa53b7c 100644 --- a/actionpack/lib/action_controller/routing.rb +++ b/actionpack/lib/action_controller/routing.rb @@ -88,6 +88,10 @@ module ActionController # # map.connect ':controller/:action/:id', :action => 'show', :defaults => { :page => 'Dashboard' } # + # Note: The default routes, as provided by the Rails generator, make all actions in every + # controller accessible via GET requests. You should consider removing them or commenting + # them out if you're using named routes and resources. + # # == Named routes # # Routes can be named with the syntax map.name_of_route options, diff --git a/railties/configs/routes.rb b/railties/configs/routes.rb index b579d6c7d1..4f3d9d22dd 100644 --- a/railties/configs/routes.rb +++ b/railties/configs/routes.rb @@ -36,6 +36,8 @@ ActionController::Routing::Routes.draw do |map| # See how all your routes lay out with "rake routes" # Install the default routes as the lowest priority. + # Note: These default routes make all actions in every controller accessible via GET requests. You should + # consider removing the them or commenting them out if you're using named routes and resources. map.connect ':controller/:action/:id' map.connect ':controller/:action/:id.:format' end From 7d5c447d9c4ebebd921a90810ff7547f8e40ea38 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Fri, 11 Jul 2008 11:12:53 -0500 Subject: [PATCH 19/74] Stubba is included in Mocha already --- actionmailer/test/abstract_unit.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/actionmailer/test/abstract_unit.rb b/actionmailer/test/abstract_unit.rb index 9b7a4661b6..11058a770d 100644 --- a/actionmailer/test/abstract_unit.rb +++ b/actionmailer/test/abstract_unit.rb @@ -32,8 +32,7 @@ end # Wrap tests that use Mocha and skip if unavailable. def uses_mocha(test_name) - gem 'mocha', ">=0.5" - require 'stubba' + gem 'mocha', ">=0.9.0" yield rescue Gem::LoadError $stderr.puts "Skipping #{test_name} tests (Mocha >= 0.5 is required). `gem install mocha` and try again." From 68289693f723f7a11ab13014a784d0a95abfd6ed Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Fri, 11 Jul 2008 11:14:59 -0500 Subject: [PATCH 20/74] Check for response in builder template since ActionMailer does not have one --- actionpack/lib/action_view/template_handlers/builder.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/actionpack/lib/action_view/template_handlers/builder.rb b/actionpack/lib/action_view/template_handlers/builder.rb index 2d413490bd..cbe53e11d8 100644 --- a/actionpack/lib/action_view/template_handlers/builder.rb +++ b/actionpack/lib/action_view/template_handlers/builder.rb @@ -6,7 +6,8 @@ module ActionView include Compilable def compile(template) - "controller.response.content_type ||= Mime::XML;" + + # ActionMailer does not have a response + "controller.respond_to?(:response) && controller.response.content_type ||= Mime::XML;" + "xml = ::Builder::XmlMarkup.new(:indent => 2);" + template.source + ";xml.target!;" From 15b21754268e6e9fd90e866096ebd60ee6fd972b Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Fri, 11 Jul 2008 11:44:24 -0500 Subject: [PATCH 21/74] Fixed teardown method typo (plus whitespace) --- actionpack/test/controller/resources_test.rb | 32 +++++++++----------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/actionpack/test/controller/resources_test.rb b/actionpack/test/controller/resources_test.rb index 0d089d0f23..0f7924649a 100644 --- a/actionpack/test/controller/resources_test.rb +++ b/actionpack/test/controller/resources_test.rb @@ -28,18 +28,16 @@ module Backoffice end class ResourcesTest < Test::Unit::TestCase - - # The assertions in these tests are incompatible with the hash method # optimisation. This could indicate user level problems def setup ActionController::Base.optimise_named_routes = false end - - def tear_down + + def teardown ActionController::Base.optimise_named_routes = true end - + def test_should_arrange_actions resource = ActionController::Resources::Resource.new(:messages, :collection => { :rss => :get, :reorder => :post, :csv => :post }, @@ -159,14 +157,14 @@ class ResourcesTest < Test::Unit::TestCase def test_with_collection_actions_and_name_prefix actions = { 'a' => :get, 'b' => :put, 'c' => :post, 'd' => :delete } - + with_restful_routing :messages, :path_prefix => '/threads/:thread_id', :name_prefix => "thread_", :collection => actions do assert_restful_routes_for :messages, :path_prefix => 'threads/1/', :name_prefix => 'thread_', :options => { :thread_id => '1' } do |options| actions.each do |action, method| assert_recognizes(options.merge(:action => action), :path => "/threads/1/messages/#{action}", :method => method) end end - + assert_restful_named_routes_for :messages, :path_prefix => 'threads/1/', :name_prefix => 'thread_', :options => { :thread_id => '1' } do |options| actions.keys.each do |action| assert_named_route "/threads/1/messages/#{action}", "#{action}_thread_messages_path", :action => action @@ -177,14 +175,14 @@ class ResourcesTest < Test::Unit::TestCase def test_with_collection_action_and_name_prefix_and_formatted actions = { 'a' => :get, 'b' => :put, 'c' => :post, 'd' => :delete } - + with_restful_routing :messages, :path_prefix => '/threads/:thread_id', :name_prefix => "thread_", :collection => actions do assert_restful_routes_for :messages, :path_prefix => 'threads/1/', :name_prefix => 'thread_', :options => { :thread_id => '1' } do |options| actions.each do |action, method| assert_recognizes(options.merge(:action => action, :format => 'xml'), :path => "/threads/1/messages/#{action}.xml", :method => method) end end - + assert_restful_named_routes_for :messages, :path_prefix => 'threads/1/', :name_prefix => 'thread_', :options => { :thread_id => '1' } do |options| actions.keys.each do |action| assert_named_route "/threads/1/messages/#{action}.xml", "formatted_#{action}_thread_messages_path", :action => action, :format => 'xml' @@ -279,7 +277,7 @@ class ResourcesTest < Test::Unit::TestCase end end end - + def test_with_new_action_with_name_prefix with_restful_routing :messages, :new => { :preview => :post }, :path_prefix => '/threads/:thread_id', :name_prefix => 'thread_' do preview_options = {:action => 'preview', :thread_id => '1'} @@ -293,7 +291,7 @@ class ResourcesTest < Test::Unit::TestCase end end end - + def test_with_formatted_new_action_with_name_prefix with_restful_routing :messages, :new => { :preview => :post }, :path_prefix => '/threads/:thread_id', :name_prefix => 'thread_' do preview_options = {:action => 'preview', :thread_id => '1', :format => 'xml'} @@ -307,7 +305,7 @@ class ResourcesTest < Test::Unit::TestCase end end end - + def test_override_new_method with_restful_routing :messages do assert_restful_routes_for :messages do |options| @@ -524,9 +522,9 @@ class ResourcesTest < Test::Unit::TestCase map.resources :messages, :collection => {:search => :get}, :new => {:preview => :any}, :name_prefix => 'thread_', :path_prefix => '/threads/:thread_id' map.resource :account, :member => {:login => :get}, :new => {:preview => :any}, :name_prefix => 'admin_', :path_prefix => '/admin' end - + action_separator = ActionController::Base.resource_action_separator - + assert_simply_restful_for :messages, :name_prefix => 'thread_', :path_prefix => 'threads/1/', :options => { :thread_id => '1' } assert_named_route "/threads/1/messages#{action_separator}search", "search_thread_messages_path", {} assert_named_route "/threads/1/messages/new", "new_thread_message_path", {} @@ -623,7 +621,7 @@ class ResourcesTest < Test::Unit::TestCase assert_simply_restful_for :products, :controller => "backoffice/products" end end - + def test_nested_resources_using_namespace with_routing do |set| set.draw do |map| @@ -795,7 +793,7 @@ class ResourcesTest < Test::Unit::TestCase yield options[:options] if block_given? end - + def assert_singleton_routes_for(singleton_name, options = {}) options[:options] ||= {} options[:options][:controller] = options[:controller] || singleton_name.to_s.pluralize @@ -855,7 +853,7 @@ class ResourcesTest < Test::Unit::TestCase actual = @controller.send(route, options) rescue $!.class.name assert_equal expected, actual, "Error on route: #{route}(#{options.inspect})" end - + def assert_resource_methods(expected, resource, action_method, method) assert_equal expected.length, resource.send("#{action_method}_methods")[method].size, "#{resource.send("#{action_method}_methods")[method].inspect}" expected.each do |action| From d106f2d08aad517c0bfa3a11e3eb87e14231e8ce Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Fri, 11 Jul 2008 11:49:22 -0500 Subject: [PATCH 22/74] Ensure use_accept_header is enabled for test_action_cache_conditional_options --- actionpack/test/controller/caching_test.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/actionpack/test/controller/caching_test.rb b/actionpack/test/controller/caching_test.rb index 7aa4e0cd93..71b53893b8 100644 --- a/actionpack/test/controller/caching_test.rb +++ b/actionpack/test/controller/caching_test.rb @@ -285,9 +285,11 @@ class ActionCacheTest < Test::Unit::TestCase end def test_action_cache_conditional_options + ActionController::Base.use_accept_header = true @request.env['HTTP_ACCEPT'] = 'application/json' get :index assert !fragment_exist?('hostname.com/action_caching_test') + ActionController::Base.use_accept_header = false end def test_action_cache_with_store_options From 04a87af5b7abd00e74b96c5b1bb789d4484da6d9 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Fri, 11 Jul 2008 11:51:35 -0500 Subject: [PATCH 23/74] Ensure use_accept_header is enabled for test_action_cache_conditional_options --- actionpack/test/controller/caching_test.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/actionpack/test/controller/caching_test.rb b/actionpack/test/controller/caching_test.rb index 71b53893b8..15db70e474 100644 --- a/actionpack/test/controller/caching_test.rb +++ b/actionpack/test/controller/caching_test.rb @@ -285,11 +285,12 @@ class ActionCacheTest < Test::Unit::TestCase end def test_action_cache_conditional_options + old_use_accept_header = ActionController::Base.use_accept_header ActionController::Base.use_accept_header = true @request.env['HTTP_ACCEPT'] = 'application/json' get :index assert !fragment_exist?('hostname.com/action_caching_test') - ActionController::Base.use_accept_header = false + ActionController::Base.use_accept_header = old_use_accept_header end def test_action_cache_with_store_options From f522a89d6447da306778b67353adaf679c26bbd1 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Fri, 11 Jul 2008 12:05:02 -0500 Subject: [PATCH 24/74] Revert "Fixed generator collisions for nested controller modules." This reverts commit 2d372d704987e05712ccd937e78d8dbd41242efe. --- railties/lib/rails_generator/commands.rb | 25 +++++++++--------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/railties/lib/rails_generator/commands.rb b/railties/lib/rails_generator/commands.rb index fb62ba6940..d258aeaa0a 100644 --- a/railties/lib/rails_generator/commands.rb +++ b/railties/lib/rails_generator/commands.rb @@ -154,35 +154,28 @@ HELP # Ruby or Rails. In the future, expand to check other namespaces # such as the rest of the user's app. def class_collisions(*class_names) - - # Initialize some check varibles - last_class = Object - current_class = nil - name = nil - class_names.flatten.each do |class_name| # Convert to string to allow symbol arguments. class_name = class_name.to_s # Skip empty strings. - class_name.strip.empty? ? next : current_class = class_name + next if class_name.strip.empty? # Split the class from its module nesting. nesting = class_name.split('::') name = nesting.pop # Extract the last Module in the nesting. - last = nesting.inject(last_class) { |last, nest| - break unless last_class.const_defined?(nest) - last_class = last_class.const_get(nest) + last = nesting.inject(Object) { |last, nest| + break unless last.const_defined?(nest) + last.const_get(nest) } - end - # If the last Module exists, check whether the given - # class exists and raise a collision if so. - - if last_class and last_class.const_defined?(name.camelize) - raise_class_collision(current_class) + # If the last Module exists, check whether the given + # class exists and raise a collision if so. + if last and last.const_defined?(name.camelize) + raise_class_collision(class_name) + end end end From c00baf496ef40d09962aabcb63d6d319cb8a0584 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Fri, 11 Jul 2008 12:09:25 -0500 Subject: [PATCH 25/74] Added tests to show that 2d372d7 breaks old generator behavior (#545 state:resolved) --- .../rails_controller_generator_test.rb | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/railties/test/generators/rails_controller_generator_test.rb b/railties/test/generators/rails_controller_generator_test.rb index 0090d21b85..8304fb5a01 100644 --- a/railties/test/generators/rails_controller_generator_test.rb +++ b/railties/test/generators/rails_controller_generator_test.rb @@ -17,4 +17,23 @@ class RailsControllerGeneratorTest < GeneratorTestCase assert_generated_functional_test_for "admin::products" assert_generated_helper_for "admin::products" end + + def test_controller_generates_namespaced_and_not_namespaced_controllers + run_generator('controller', %w(products)) + + # We have to require the generated helper to show the problem because + # the test helpers just check for generated files and contents but + # do not actually load them. But they have to be loaded (as in a real environment) + # to make the second generator run fail + require "#{RAILS_ROOT}/app/helpers/products_helper" + + assert_nothing_raised do + begin + run_generator('controller', %w(admin::products)) + ensure + # cleanup + Object.send(:remove_const, :ProductsHelper) + end + end + end end From 6b9f8adb3eff3be81653bd565be4ff9c63cd775d Mon Sep 17 00:00:00 2001 From: Michael Koziarski Date: Fri, 11 Jul 2008 19:22:43 +0200 Subject: [PATCH 26/74] Whitespace --- actionpack/lib/action_view/helpers/capture_helper.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/actionpack/lib/action_view/helpers/capture_helper.rb b/actionpack/lib/action_view/helpers/capture_helper.rb index 990c30b90d..720e2da8cc 100644 --- a/actionpack/lib/action_view/helpers/capture_helper.rb +++ b/actionpack/lib/action_view/helpers/capture_helper.rb @@ -34,9 +34,8 @@ module ActionView # Return captured buffer in erb. if block_called_from_erb?(block) with_output_buffer { block.call(*args) } - - # Return block result otherwise, but protect buffer also. else + # Return block result otherwise, but protect buffer also. with_output_buffer { return block.call(*args) } end end From 292501c7e01993faadfd953fd1b3154c470b65e2 Mon Sep 17 00:00:00 2001 From: Michael Koziarski Date: Fri, 11 Jul 2008 22:27:36 +0200 Subject: [PATCH 27/74] Use require_dependency 'application' not require in the console bootstraps to avoid requiring application.rb twice --- railties/lib/console_with_helpers.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/railties/lib/console_with_helpers.rb b/railties/lib/console_with_helpers.rb index 79018a9f76..be453a6896 100644 --- a/railties/lib/console_with_helpers.rb +++ b/railties/lib/console_with_helpers.rb @@ -16,7 +16,7 @@ def helper(*helper_names) end end -require 'application' +require_dependency 'application' class << helper include_all_modules_from ActionView From 6ebdd0e32b846e99a9885b06fbca2bed58149c7e Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Fri, 11 Jul 2008 15:39:22 -0500 Subject: [PATCH 28/74] Changed ActionView::TemplateHandler#render API method signature to render(template, local_assigns = {}) --- actionpack/CHANGELOG | 2 ++ actionpack/lib/action_view/base.rb | 4 ++-- actionpack/lib/action_view/renderable.rb | 2 +- actionpack/lib/action_view/template_handler.rb | 2 +- .../lib/action_view/template_handlers/compilable.rb | 4 ++-- actionpack/test/controller/layout_test.rb | 4 ++-- actionpack/test/template/render_test.rb | 9 ++++----- 7 files changed, 14 insertions(+), 13 deletions(-) diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG index 89cd7c64af..dc508f701a 100644 --- a/actionpack/CHANGELOG +++ b/actionpack/CHANGELOG @@ -1,5 +1,7 @@ *Edge* +* Changed ActionView::TemplateHandler#render API method signature to render(template, local_assigns = {}) [Josh Peek] + * Changed PrototypeHelper#submit_to_remote to PrototypeHelper#button_to_remote to stay consistent with link_to_remote (submit_to_remote still works as an alias) #8994 [clemens] * Add :recursive option to javascript_include_tag and stylesheet_link_tag to be used along with :all. #480 [Damian Janowski] diff --git a/actionpack/lib/action_view/base.rb b/actionpack/lib/action_view/base.rb index 66892fdabf..7ef90ddf5e 100644 --- a/actionpack/lib/action_view/base.rb +++ b/actionpack/lib/action_view/base.rb @@ -332,8 +332,8 @@ module ActionView #:nodoc: @assigns.each { |key, value| instance_variable_set("@#{key}", value) } end - def execute(template) - send(template.method, template.locals) do |*names| + def execute(template, local_assigns = {}) + send(template.method, local_assigns) do |*names| instance_variable_get "@content_for_#{names.first || 'layout'}" end end diff --git a/actionpack/lib/action_view/renderable.rb b/actionpack/lib/action_view/renderable.rb index 3a30e603fe..6a8a0e44fc 100644 --- a/actionpack/lib/action_view/renderable.rb +++ b/actionpack/lib/action_view/renderable.rb @@ -8,7 +8,7 @@ module ActionView def render prepare! - @handler.render(self) + @handler.render(self, @locals) end def method diff --git a/actionpack/lib/action_view/template_handler.rb b/actionpack/lib/action_view/template_handler.rb index 78586d6142..1afea21f67 100644 --- a/actionpack/lib/action_view/template_handler.rb +++ b/actionpack/lib/action_view/template_handler.rb @@ -8,7 +8,7 @@ module ActionView @view = view end - def render(template) + def render(template, local_assigns = {}) end def compile(template) diff --git a/actionpack/lib/action_view/template_handlers/compilable.rb b/actionpack/lib/action_view/template_handlers/compilable.rb index 9b9255579c..95ae75ca68 100644 --- a/actionpack/lib/action_view/template_handlers/compilable.rb +++ b/actionpack/lib/action_view/template_handlers/compilable.rb @@ -14,8 +14,8 @@ module ActionView end end - def render(template) - @view.send(:execute, template) + def render(template, local_assigns = {}) + @view.send(:execute, template, local_assigns) end # Compile and evaluate the template's code diff --git a/actionpack/test/controller/layout_test.rb b/actionpack/test/controller/layout_test.rb index 3dc311b78a..32be7d90ff 100644 --- a/actionpack/test/controller/layout_test.rb +++ b/actionpack/test/controller/layout_test.rb @@ -34,8 +34,8 @@ end class MabView < ActionView::TemplateHandler def initialize(view) end - - def render(template) + + def render(template, local_assigns) template.source end end diff --git a/actionpack/test/template/render_test.rb b/actionpack/test/template/render_test.rb index 726cf37026..cd004a9f6d 100644 --- a/actionpack/test/template/render_test.rb +++ b/actionpack/test/template/render_test.rb @@ -95,8 +95,8 @@ class ViewRenderTest < Test::Unit::TestCase end class CustomHandler < ActionView::TemplateHandler - def render(template) - [template.source, template.locals].inspect + def render(template, local_assigns) + [template.source, local_assigns].inspect end end @@ -115,18 +115,17 @@ class ViewRenderTest < Test::Unit::TestCase def compile(template) "@output_buffer = ''\n" + - "@output_buffer << 'locals: #{template.locals.inspect}, '\n" + "@output_buffer << 'source: #{template.source.inspect}'\n" end end def test_render_inline_with_compilable_custom_type ActionView::Template.register_template_handler :foo, CompilableCustomHandler - assert_equal 'locals: {}, source: "Hello, World!"', @view.render(:inline => "Hello, World!", :type => :foo) + assert_equal 'source: "Hello, World!"', @view.render(:inline => "Hello, World!", :type => :foo) end def test_render_inline_with_locals_and_compilable_custom_type ActionView::Template.register_template_handler :foo, CompilableCustomHandler - assert_equal 'locals: {:name=>"Josh"}, source: "Hello, <%= name %>!"', @view.render(:inline => "Hello, <%= name %>!", :locals => { :name => "Josh" }, :type => :foo) + assert_equal 'source: "Hello, <%= name %>!"', @view.render(:inline => "Hello, <%= name %>!", :locals => { :name => "Josh" }, :type => :foo) end end From 5e2e1ed9ffc481a91596d8c3fd9a68d7977e75ca Mon Sep 17 00:00:00 2001 From: Micah Wedemeyer Date: Fri, 11 Jul 2008 23:50:55 +0100 Subject: [PATCH 29/74] Ensure MysqlAdapter allows SSL connection when only sslca is supplied. [#253 state:resolved] Signed-off-by: Pratik Naik --- .../lib/active_record/connection_adapters/mysql_adapter.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb index c5962764f5..4b13ac8be0 100755 --- a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb @@ -69,7 +69,7 @@ module ActiveRecord MysqlCompat.define_all_hashes_method! mysql = Mysql.init - mysql.ssl_set(config[:sslkey], config[:sslcert], config[:sslca], config[:sslcapath], config[:sslcipher]) if config[:sslkey] + mysql.ssl_set(config[:sslkey], config[:sslcert], config[:sslca], config[:sslcapath], config[:sslcipher]) if config[:sslca] || config[:sslkey] ConnectionAdapters::MysqlAdapter.new(mysql, logger, [host, username, password, database, port, socket], config) end @@ -145,6 +145,7 @@ module ActiveRecord # * :password - Defaults to nothing. # * :database - The name of the database. No default, must be provided. # * :encoding - (Optional) Sets the client encoding by executing "SET NAMES " after connection. + # * :sslca - Necessary to use MySQL with an SSL connection. # * :sslkey - Necessary to use MySQL with an SSL connection. # * :sslcert - Necessary to use MySQL with an SSL connection. # * :sslcapath - Necessary to use MySQL with an SSL connection. @@ -507,7 +508,9 @@ module ActiveRecord @connection.options(Mysql::SET_CHARSET_NAME, encoding) rescue nil end - @connection.ssl_set(@config[:sslkey], @config[:sslcert], @config[:sslca], @config[:sslcapath], @config[:sslcipher]) if @config[:sslkey] + if @config[:sslca] || @config[:sslkey] + @connection.ssl_set(@config[:sslkey], @config[:sslcert], @config[:sslca], @config[:sslcapath], @config[:sslcipher]) + end @connection.real_connect(*@connection_options) From 50b5c6845ed1645cf25613024ef04187385f8dcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johan=20S=C3=B8rensen?= Date: Sat, 12 Jul 2008 00:57:38 +0100 Subject: [PATCH 30/74] Ensure mail_to label is obfuscated for javascript encoding. [#294 state:resolved] Signed-off-by: Pratik Naik --- actionpack/lib/action_view/helpers/url_helper.rb | 2 +- actionpack/test/template/url_helper_test.rb | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/actionpack/lib/action_view/helpers/url_helper.rb b/actionpack/lib/action_view/helpers/url_helper.rb index f6a1f271f0..e5178938fd 100644 --- a/actionpack/lib/action_view/helpers/url_helper.rb +++ b/actionpack/lib/action_view/helpers/url_helper.rb @@ -466,7 +466,7 @@ module ActionView email_address_obfuscated.gsub!(/\./, html_options.delete("replace_dot")) if html_options.has_key?("replace_dot") if encode == "javascript" - "document.write('#{content_tag("a", name || email_address, html_options.merge({ "href" => "mailto:"+email_address+extras }))}');".each_byte do |c| + "document.write('#{content_tag("a", name || email_address_obfuscated, html_options.merge({ "href" => "mailto:"+email_address+extras }))}');".each_byte do |c| string << sprintf("%%%x", c) end "" diff --git a/actionpack/test/template/url_helper_test.rb b/actionpack/test/template/url_helper_test.rb index 8e43629522..4ed5bc372f 100644 --- a/actionpack/test/template/url_helper_test.rb +++ b/actionpack/test/template/url_helper_test.rb @@ -292,6 +292,7 @@ class UrlHelperTest < ActionView::TestCase assert_dom_equal "My email", mail_to("me@domain.com", "My email", :encode => "hex", :replace_at => "(at)") assert_dom_equal "me(at)domain(dot)com", mail_to("me@domain.com", nil, :encode => "hex", :replace_at => "(at)", :replace_dot => "(dot)") assert_dom_equal "", mail_to("me@domain.com", "My email", :encode => "javascript", :replace_at => "(at)", :replace_dot => "(dot)") + assert_dom_equal "", mail_to("me@domain.com", nil, :encode => "javascript", :replace_at => "(at)", :replace_dot => "(dot)") end def protect_against_forgery? From e53f5fe696d692f1985981c34bb311e898fe3c72 Mon Sep 17 00:00:00 2001 From: Michael Koziarski Date: Sat, 12 Jul 2008 11:42:41 +0200 Subject: [PATCH 31/74] Restore support for partial matches in assert_redirected_to If both the actual redirection and the asserted redirection are hashes, succeed if the asserted redirection is a strict subset of the actual redirection. --- .../assertions/response_assertions.rb | 11 +++++++++-- actionpack/test/controller/redirect_test.rb | 5 +++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/actionpack/lib/action_controller/assertions/response_assertions.rb b/actionpack/lib/action_controller/assertions/response_assertions.rb index 3ecaee641f..64aaf6211f 100644 --- a/actionpack/lib/action_controller/assertions/response_assertions.rb +++ b/actionpack/lib/action_controller/assertions/response_assertions.rb @@ -63,11 +63,18 @@ module ActionController clean_backtrace do assert_response(:redirect, message) return true if options == @response.redirected_to + + # Support partial arguments for hash redirections + if options.is_a?(Hash) && @response.redirected_to.is_a?(Hash) + return true if options.all? {|(key, value)| @response.redirected_to[key] == value} + end + redirected_to_after_normalisation = normalize_argument_to_redirection(@response.redirected_to) options_after_normalisation = normalize_argument_to_redirection(options) - assert_equal redirected_to_after_normalisation, options_after_normalisation, - "Expected response to be a redirect to <#{options_after_normalisation}> but was a redirect to <#{redirected_to_after_normalisation}>" + if redirected_to_after_normalisation != options_after_normalisation + flunk "Expected response to be a redirect to <#{options_after_normalisation}> but was a redirect to <#{redirected_to_after_normalisation}>" + end end end diff --git a/actionpack/test/controller/redirect_test.rb b/actionpack/test/controller/redirect_test.rb index 8b72426d10..28da5c6163 100755 --- a/actionpack/test/controller/redirect_test.rb +++ b/actionpack/test/controller/redirect_test.rb @@ -227,6 +227,11 @@ class RedirectTest < Test::Unit::TestCase assert_redirected_to Workshop.new(5, true) end + def test_redirect_with_partial_params + get :module_redirect + assert_redirected_to :action => 'hello_world' + end + def test_redirect_to_nil assert_raises(ActionController::ActionControllerError) do get :redirect_to_nil From f90eb81c65d5841b591caf0f5e39ef774d02d06e Mon Sep 17 00:00:00 2001 From: Daniel Guettler Date: Mon, 23 Jun 2008 11:06:13 -0400 Subject: [PATCH 32/74] Ensure script/generate finds generators from symlinked plugins. [#449 state:resolved] Signed-off-by: Pratik Naik --- railties/lib/rails_generator/lookup.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/railties/lib/rails_generator/lookup.rb b/railties/lib/rails_generator/lookup.rb index 1f28c39d55..0526d526ad 100644 --- a/railties/lib/rails_generator/lookup.rb +++ b/railties/lib/rails_generator/lookup.rb @@ -108,7 +108,7 @@ module Rails sources << PathSource.new(:vendor, "#{::RAILS_ROOT}/vendor/generators") Rails.configuration.plugin_paths.each do |path| relative_path = Pathname.new(File.expand_path(path)).relative_path_from(Pathname.new(::RAILS_ROOT)) - sources << PathSource.new(:"plugins (#{relative_path})", "#{path}/**/{,rails_}generators") + sources << PathSource.new(:"plugins (#{relative_path})", "#{path}/*/**/{,rails_}generators") end end sources << PathSource.new(:user, "#{Dir.user_home}/.rails/generators") From b603de08825eec05f1b97d0f5d71462f3fa4c222 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sat, 12 Jul 2008 12:15:31 -0500 Subject: [PATCH 33/74] Improve test coverage and create fixtures for RenderPartialWithRecordIdentificationTests --- .../render_partial_with_record_identification_test.rb | 9 +++++++++ actionpack/test/fixtures/developers/_developer.erb | 1 + actionpack/test/fixtures/fun/games/_game.erb | 1 + actionpack/test/fixtures/fun/serious/games/_game.erb | 1 + actionpack/test/fixtures/projects/_project.erb | 1 + actionpack/test/fixtures/replies/_reply.erb | 1 + 6 files changed, 14 insertions(+) create mode 100644 actionpack/test/fixtures/developers/_developer.erb create mode 100644 actionpack/test/fixtures/fun/games/_game.erb create mode 100644 actionpack/test/fixtures/fun/serious/games/_game.erb create mode 100644 actionpack/test/fixtures/projects/_project.erb create mode 100644 actionpack/test/fixtures/replies/_reply.erb diff --git a/actionpack/test/activerecord/render_partial_with_record_identification_test.rb b/actionpack/test/activerecord/render_partial_with_record_identification_test.rb index af2725a99b..a9d3ff040f 100644 --- a/actionpack/test/activerecord/render_partial_with_record_identification_test.rb +++ b/actionpack/test/activerecord/render_partial_with_record_identification_test.rb @@ -56,26 +56,31 @@ class RenderPartialWithRecordIdentificationTest < ActiveRecordTestCase def test_rendering_partial_with_has_many_and_belongs_to_association get :render_with_has_many_and_belongs_to_association assert_template 'projects/_project' + assert_equal 'Active RecordActive Controller', @response.body end def test_rendering_partial_with_has_many_association get :render_with_has_many_association assert_template 'replies/_reply' + assert_equal 'Birdman is better!', @response.body end def test_rendering_partial_with_named_scope get :render_with_named_scope assert_template 'replies/_reply' + assert_equal 'Birdman is better!Nuh uh!', @response.body end def test_render_with_record get :render_with_record assert_template 'developers/_developer' + assert_equal 'David', @response.body end def test_render_with_record_collection get :render_with_record_collection assert_template 'developers/_developer' + assert_equal 'DavidJamisfixture_3fixture_4fixture_5fixture_6fixture_7fixture_8fixture_9fixture_10Jamis', @response.body end def test_rendering_partial_with_has_one_association @@ -165,11 +170,13 @@ class RenderPartialWithRecordIdentificationAndNestedControllersTest < ActiveReco def test_render_with_record_in_nested_controller get :render_with_record_in_nested_controller assert_template 'fun/games/_game' + assert_equal 'Pong', @response.body end def test_render_with_record_collection_in_nested_controller get :render_with_record_collection_in_nested_controller assert_template 'fun/games/_game' + assert_equal 'PongTank', @response.body end end @@ -184,10 +191,12 @@ class RenderPartialWithRecordIdentificationAndNestedDeeperControllersTest < Acti def test_render_with_record_in_deeper_nested_controller get :render_with_record_in_deeper_nested_controller assert_template 'fun/serious/games/_game' + assert_equal 'Chess', @response.body end def test_render_with_record_collection_in_deeper_nested_controller get :render_with_record_collection_in_deeper_nested_controller assert_template 'fun/serious/games/_game' + assert_equal 'ChessSudokuSolitaire', @response.body end end diff --git a/actionpack/test/fixtures/developers/_developer.erb b/actionpack/test/fixtures/developers/_developer.erb new file mode 100644 index 0000000000..904a3137e7 --- /dev/null +++ b/actionpack/test/fixtures/developers/_developer.erb @@ -0,0 +1 @@ +<%= developer.name %> \ No newline at end of file diff --git a/actionpack/test/fixtures/fun/games/_game.erb b/actionpack/test/fixtures/fun/games/_game.erb new file mode 100644 index 0000000000..d51b7b3ebc --- /dev/null +++ b/actionpack/test/fixtures/fun/games/_game.erb @@ -0,0 +1 @@ +<%= game.name %> \ No newline at end of file diff --git a/actionpack/test/fixtures/fun/serious/games/_game.erb b/actionpack/test/fixtures/fun/serious/games/_game.erb new file mode 100644 index 0000000000..d51b7b3ebc --- /dev/null +++ b/actionpack/test/fixtures/fun/serious/games/_game.erb @@ -0,0 +1 @@ +<%= game.name %> \ No newline at end of file diff --git a/actionpack/test/fixtures/projects/_project.erb b/actionpack/test/fixtures/projects/_project.erb new file mode 100644 index 0000000000..480c4c2af3 --- /dev/null +++ b/actionpack/test/fixtures/projects/_project.erb @@ -0,0 +1 @@ +<%= project.name %> \ No newline at end of file diff --git a/actionpack/test/fixtures/replies/_reply.erb b/actionpack/test/fixtures/replies/_reply.erb new file mode 100644 index 0000000000..68baf548d8 --- /dev/null +++ b/actionpack/test/fixtures/replies/_reply.erb @@ -0,0 +1 @@ +<%= reply.content %> \ No newline at end of file From 65fb2e76f2c4571b04458c7bf6a0c815972232ab Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sat, 12 Jul 2008 12:16:05 -0500 Subject: [PATCH 34/74] Removed a few implementation specific view path tests --- actionpack/test/controller/view_paths_test.rb | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/actionpack/test/controller/view_paths_test.rb b/actionpack/test/controller/view_paths_test.rb index 9401c87d10..b1c19384aa 100644 --- a/actionpack/test/controller/view_paths_test.rb +++ b/actionpack/test/controller/view_paths_test.rb @@ -146,18 +146,4 @@ class ViewLoadPathsTest < Test::Unit::TestCase assert_nothing_raised { C.view_paths << 'c/path' } assert_equal ['c/path'], C.view_paths end - - def test_find_template_file_for_path - assert_equal "test/hello_world.erb", @controller.view_paths.find_template_file_for_path("test/hello_world.erb").to_s - assert_equal "test/hello.builder", @controller.view_paths.find_template_file_for_path("test/hello.builder").to_s - assert_equal nil, @controller.view_paths.find_template_file_for_path("test/missing.erb") - end - - def test_view_paths_find_template_file_for_path - assert_equal "test/formatted_html_erb.html.erb", @controller.view_paths.find_template_file_for_path("test/formatted_html_erb.html").to_s - assert_equal "test/formatted_xml_erb.xml.erb", @controller.view_paths.find_template_file_for_path("test/formatted_xml_erb.xml").to_s - assert_equal "test/hello_world.erb", @controller.view_paths.find_template_file_for_path("test/hello_world.html").to_s - assert_equal "test/hello_world.erb", @controller.view_paths.find_template_file_for_path("test/hello_world.xml").to_s - assert_equal nil, @controller.view_paths.find_template_file_for_path("test/missing.html") - end end From 30204c4e66cea989c4ee48b52c8827c79e98f14a Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sat, 12 Jul 2008 14:11:51 -0500 Subject: [PATCH 35/74] Set global ActionController::Base.view_paths for test cases --- actionpack/test/abstract_unit.rb | 4 +++- .../render_partial_with_record_identification_test.rb | 8 -------- actionpack/test/controller/action_pack_assertions_test.rb | 8 -------- actionpack/test/controller/addresses_render_test.rb | 2 -- actionpack/test/controller/caching_test.rb | 3 --- actionpack/test/controller/capture_test.rb | 2 -- actionpack/test/controller/content_type_test.rb | 2 -- .../deprecation/deprecated_base_methods_test.rb | 2 -- actionpack/test/controller/mime_responds_test.rb | 2 -- actionpack/test/controller/new_render_test.rb | 3 --- actionpack/test/controller/render_test.rb | 3 --- actionpack/test/controller/send_file_test.rb | 2 -- actionpack/test/controller/view_paths_test.rb | 2 -- actionpack/test/template/render_test.rb | 2 +- actionpack/test/template/url_helper_test.rb | 8 -------- 15 files changed, 4 insertions(+), 49 deletions(-) diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb index 70f6a28a9c..0d2e0f273a 100644 --- a/actionpack/test/abstract_unit.rb +++ b/actionpack/test/abstract_unit.rb @@ -22,7 +22,9 @@ ActiveSupport::Deprecation.debug = true ActionController::Base.logger = nil ActionController::Routing::Routes.reload rescue nil -FIXTURE_LOAD_PATH = ActionView::ViewLoadPaths::LoadPath.new(File.join(File.dirname(__FILE__), 'fixtures')) +ActionView::Base.cache_template_loading = true +FIXTURE_LOAD_PATH = File.join(File.dirname(__FILE__), 'fixtures') +ActionController::Base.view_paths = FIXTURE_LOAD_PATH # Wrap tests that use Mocha and skip if unavailable. def uses_mocha(test_name) diff --git a/actionpack/test/activerecord/render_partial_with_record_identification_test.rb b/actionpack/test/activerecord/render_partial_with_record_identification_test.rb index a9d3ff040f..a82a1a3023 100644 --- a/actionpack/test/activerecord/render_partial_with_record_identification_test.rb +++ b/actionpack/test/activerecord/render_partial_with_record_identification_test.rb @@ -41,8 +41,6 @@ class RenderPartialWithRecordIdentificationController < ActionController::Base end end -RenderPartialWithRecordIdentificationController.view_paths = [FIXTURE_LOAD_PATH] - class RenderPartialWithRecordIdentificationTest < ActiveRecordTestCase fixtures :developers, :projects, :developers_projects, :topics, :replies, :companies, :mascots @@ -123,8 +121,6 @@ class RenderPartialWithRecordIdentificationController < ActionController::Base end end -RenderPartialWithRecordIdentificationController.view_paths = [FIXTURE_LOAD_PATH] - class Game < Struct.new(:name, :id) def to_param id.to_s @@ -142,8 +138,6 @@ module Fun end end - NestedController.view_paths = [FIXTURE_LOAD_PATH] - module Serious class NestedDeeperController < ActionController::Base def render_with_record_in_deeper_nested_controller @@ -154,8 +148,6 @@ module Fun render :partial => [ Game.new("Chess"), Game.new("Sudoku"), Game.new("Solitaire") ] end end - - NestedDeeperController.view_paths = [FIXTURE_LOAD_PATH] end end diff --git a/actionpack/test/controller/action_pack_assertions_test.rb b/actionpack/test/controller/action_pack_assertions_test.rb index f5cda99977..610e196362 100644 --- a/actionpack/test/controller/action_pack_assertions_test.rb +++ b/actionpack/test/controller/action_pack_assertions_test.rb @@ -164,14 +164,6 @@ module Admin end end -# --------------------------------------------------------------------------- - - -# tell the controller where to find its templates but start from parent -# directory of test_request_response to simulate the behaviour of a -# production environment -ActionPackAssertionsController.view_paths = [FIXTURE_LOAD_PATH] - # a test case to exercise the new capabilities TestRequest & TestResponse class ActionPackAssertionsControllerTest < Test::Unit::TestCase # let's get this party started diff --git a/actionpack/test/controller/addresses_render_test.rb b/actionpack/test/controller/addresses_render_test.rb index df87182082..b26cae24fb 100644 --- a/actionpack/test/controller/addresses_render_test.rb +++ b/actionpack/test/controller/addresses_render_test.rb @@ -19,8 +19,6 @@ class AddressesTestController < ActionController::Base def self.controller_path; "addresses"; end end -AddressesTestController.view_paths = [FIXTURE_LOAD_PATH] - class AddressesTest < Test::Unit::TestCase def setup @controller = AddressesTestController.new diff --git a/actionpack/test/controller/caching_test.rb b/actionpack/test/controller/caching_test.rb index 15db70e474..2e98837a35 100644 --- a/actionpack/test/controller/caching_test.rb +++ b/actionpack/test/controller/caching_test.rb @@ -6,7 +6,6 @@ CACHE_DIR = 'test_cache' FILE_STORE_PATH = File.join(File.dirname(__FILE__), '/../temp/', CACHE_DIR) ActionController::Base.page_cache_directory = FILE_STORE_PATH ActionController::Base.cache_store = :file_store, FILE_STORE_PATH -ActionController::Base.view_paths = [FIXTURE_LOAD_PATH] class PageCachingTestController < ActionController::Base caches_page :ok, :no_content, :if => Proc.new { |c| !c.request.format.json? } @@ -636,8 +635,6 @@ class FunctionalCachingController < ActionController::Base end end -FunctionalCachingController.view_paths = [FIXTURE_LOAD_PATH] - class FunctionalFragmentCachingTest < Test::Unit::TestCase def setup ActionController::Base.perform_caching = true diff --git a/actionpack/test/controller/capture_test.rb b/actionpack/test/controller/capture_test.rb index 87f9ce8ab3..5ded6a5d26 100644 --- a/actionpack/test/controller/capture_test.rb +++ b/actionpack/test/controller/capture_test.rb @@ -23,8 +23,6 @@ class CaptureController < ActionController::Base def rescue_action(e) raise end end -CaptureController.view_paths = [FIXTURE_LOAD_PATH] - class CaptureTest < Test::Unit::TestCase def setup @controller = CaptureController.new diff --git a/actionpack/test/controller/content_type_test.rb b/actionpack/test/controller/content_type_test.rb index 33aa4e49ee..d457d13aef 100644 --- a/actionpack/test/controller/content_type_test.rb +++ b/actionpack/test/controller/content_type_test.rb @@ -45,8 +45,6 @@ class ContentTypeController < ActionController::Base def rescue_action(e) raise end end -ContentTypeController.view_paths = [FIXTURE_LOAD_PATH] - class ContentTypeTest < Test::Unit::TestCase def setup @controller = ContentTypeController.new diff --git a/actionpack/test/controller/deprecation/deprecated_base_methods_test.rb b/actionpack/test/controller/deprecation/deprecated_base_methods_test.rb index f485500b7f..86555a77df 100644 --- a/actionpack/test/controller/deprecation/deprecated_base_methods_test.rb +++ b/actionpack/test/controller/deprecation/deprecated_base_methods_test.rb @@ -13,8 +13,6 @@ class DeprecatedBaseMethodsTest < Test::Unit::TestCase def rescue_action(e) raise e end end - Target.view_paths = [FIXTURE_LOAD_PATH] - def setup @request = ActionController::TestRequest.new @response = ActionController::TestResponse.new diff --git a/actionpack/test/controller/mime_responds_test.rb b/actionpack/test/controller/mime_responds_test.rb index 17f5e27232..1701431858 100644 --- a/actionpack/test/controller/mime_responds_test.rb +++ b/actionpack/test/controller/mime_responds_test.rb @@ -162,8 +162,6 @@ class RespondToController < ActionController::Base end end -RespondToController.view_paths = [FIXTURE_LOAD_PATH] - class MimeControllerTest < Test::Unit::TestCase def setup ActionController::Base.use_accept_header = true diff --git a/actionpack/test/controller/new_render_test.rb b/actionpack/test/controller/new_render_test.rb index b4dc2bb4dc..d2a3a2b0b0 100644 --- a/actionpack/test/controller/new_render_test.rb +++ b/actionpack/test/controller/new_render_test.rb @@ -465,9 +465,6 @@ class NewRenderTestController < ActionController::Base end end -NewRenderTestController.view_paths = [FIXTURE_LOAD_PATH] -Fun::GamesController.view_paths = [FIXTURE_LOAD_PATH] - class NewRenderTest < Test::Unit::TestCase def setup @controller = NewRenderTestController.new diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb index 10264dadaa..a857810b78 100644 --- a/actionpack/test/controller/render_test.rb +++ b/actionpack/test/controller/render_test.rb @@ -217,9 +217,6 @@ class TestController < ActionController::Base end end -TestController.view_paths = [FIXTURE_LOAD_PATH] -Fun::GamesController.view_paths = [FIXTURE_LOAD_PATH] - class RenderTest < Test::Unit::TestCase def setup @request = ActionController::TestRequest.new diff --git a/actionpack/test/controller/send_file_test.rb b/actionpack/test/controller/send_file_test.rb index ddec51d173..c003abf094 100644 --- a/actionpack/test/controller/send_file_test.rb +++ b/actionpack/test/controller/send_file_test.rb @@ -19,8 +19,6 @@ class SendFileController < ActionController::Base def rescue_action(e) raise end end -SendFileController.view_paths = [FIXTURE_LOAD_PATH] - class SendFileTest < Test::Unit::TestCase include TestFileUtils diff --git a/actionpack/test/controller/view_paths_test.rb b/actionpack/test/controller/view_paths_test.rb index b1c19384aa..85fa58a45b 100644 --- a/actionpack/test/controller/view_paths_test.rb +++ b/actionpack/test/controller/view_paths_test.rb @@ -1,8 +1,6 @@ require 'abstract_unit' class ViewLoadPathsTest < Test::Unit::TestCase - ActionController::Base.view_paths = [FIXTURE_LOAD_PATH] - class TestController < ActionController::Base def self.controller_path() "test" end def rescue_action(e) raise end diff --git a/actionpack/test/template/render_test.rb b/actionpack/test/template/render_test.rb index cd004a9f6d..cc5b4900dc 100644 --- a/actionpack/test/template/render_test.rb +++ b/actionpack/test/template/render_test.rb @@ -4,7 +4,7 @@ require 'controller/fake_models' class ViewRenderTest < Test::Unit::TestCase def setup @assigns = { :secret => 'in the sauce' } - @view = ActionView::Base.new([FIXTURE_LOAD_PATH], @assigns) + @view = ActionView::Base.new(ActionController::Base.view_paths, @assigns) end def test_render_file diff --git a/actionpack/test/template/url_helper_test.rb b/actionpack/test/template/url_helper_test.rb index 4ed5bc372f..91d5c6ffb5 100644 --- a/actionpack/test/template/url_helper_test.rb +++ b/actionpack/test/template/url_helper_test.rb @@ -302,8 +302,6 @@ end class UrlHelperWithControllerTest < ActionView::TestCase class UrlHelperController < ActionController::Base - self.view_paths = [FIXTURE_LOAD_PATH] - def self.controller_path; 'url_helper_with_controller' end def show_url_for @@ -366,8 +364,6 @@ end class LinkToUnlessCurrentWithControllerTest < ActionView::TestCase class TasksController < ActionController::Base - self.view_paths = [FIXTURE_LOAD_PATH] - def self.controller_path; 'tasks' end def index @@ -458,8 +454,6 @@ end class PolymorphicControllerTest < ActionView::TestCase class WorkshopsController < ActionController::Base - self.view_paths = [FIXTURE_LOAD_PATH] - def self.controller_path; 'workshops' end def index @@ -476,8 +470,6 @@ class PolymorphicControllerTest < ActionView::TestCase end class SessionsController < ActionController::Base - self.view_paths = [FIXTURE_LOAD_PATH] - def self.controller_path; 'sessions' end def index From 73b34e9f75d33dc0709d4ad36c912bdbb8977994 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sat, 12 Jul 2008 14:33:46 -0500 Subject: [PATCH 36/74] Refactor template preloading. New abstractions include Renderable mixins and a refactored Template class. --- actionmailer/lib/action_mailer/base.rb | 2 +- actionpack/CHANGELOG | 2 + .../assertions/response_assertions.rb | 2 +- actionpack/lib/action_controller/base.rb | 9 +- actionpack/lib/action_controller/layout.rb | 2 +- .../lib/action_controller/test_process.rb | 10 +- actionpack/lib/action_view.rb | 8 +- actionpack/lib/action_view/base.rb | 79 +++++----- actionpack/lib/action_view/inline_template.rb | 13 +- .../lib/action_view/partial_template.rb | 69 --------- actionpack/lib/action_view/partials.rb | 58 ++++--- actionpack/lib/action_view/paths.rb | 85 ++++++++++ actionpack/lib/action_view/renderable.rb | 83 +++++++--- .../lib/action_view/renderable_partial.rb | 19 +++ actionpack/lib/action_view/template.rb | 146 +++++++++++------- actionpack/lib/action_view/template_error.rb | 2 +- actionpack/lib/action_view/template_file.rb | 88 ----------- .../template_handlers/compilable.rb | 50 ------ actionpack/lib/action_view/view_load_paths.rb | 103 ------------ actionpack/test/controller/layout_test.rb | 1 + railties/lib/initializer.rb | 2 +- 21 files changed, 361 insertions(+), 472 deletions(-) delete mode 100644 actionpack/lib/action_view/partial_template.rb create mode 100644 actionpack/lib/action_view/paths.rb create mode 100644 actionpack/lib/action_view/renderable_partial.rb delete mode 100644 actionpack/lib/action_view/template_file.rb delete mode 100644 actionpack/lib/action_view/view_load_paths.rb diff --git a/actionmailer/lib/action_mailer/base.rb b/actionmailer/lib/action_mailer/base.rb index 1518e23dfe..5a71935009 100644 --- a/actionmailer/lib/action_mailer/base.rb +++ b/actionmailer/lib/action_mailer/base.rb @@ -426,7 +426,7 @@ module ActionMailer #:nodoc: end def template_root=(root) - write_inheritable_attribute(:template_root, ActionView::ViewLoadPaths.new(Array(root))) + write_inheritable_attribute(:template_root, ActionView::PathSet.new(Array(root))) end end diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG index dc508f701a..507a2b69bf 100644 --- a/actionpack/CHANGELOG +++ b/actionpack/CHANGELOG @@ -1,5 +1,7 @@ *Edge* +* Refactor template preloading. New abstractions include Renderable mixins and a refactored Template class [Josh Peek] + * Changed ActionView::TemplateHandler#render API method signature to render(template, local_assigns = {}) [Josh Peek] * Changed PrototypeHelper#submit_to_remote to PrototypeHelper#button_to_remote to stay consistent with link_to_remote (submit_to_remote still works as an alias) #8994 [clemens] diff --git a/actionpack/lib/action_controller/assertions/response_assertions.rb b/actionpack/lib/action_controller/assertions/response_assertions.rb index 64aaf6211f..cb36405700 100644 --- a/actionpack/lib/action_controller/assertions/response_assertions.rb +++ b/actionpack/lib/action_controller/assertions/response_assertions.rb @@ -93,7 +93,7 @@ module ActionController if expected.nil? !@response.rendered_with_file? else - expected == rendered + rendered.match(expected) end end end diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index 9d9ff527c2..6926941396 100755 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -431,7 +431,7 @@ module ActionController #:nodoc: end def view_paths=(value) - @view_paths = ActionView::ViewLoadPaths.new(Array(value)) if value + @view_paths = ActionView::PathSet.new(Array(value)) if value end # Adds a view_path to the front of the view_paths array. @@ -652,7 +652,7 @@ module ActionController #:nodoc: end def view_paths=(value) - @template.view_paths = ViewLoadPaths.new(value) + @template.view_paths = PathSet.new(value) end # Adds a view_path to the front of the view_paths array. @@ -1248,9 +1248,8 @@ module ActionController #:nodoc: end def template_exempt_from_layout?(template_name = default_template_name) - extension = @template && @template.pick_template_extension(template_name) - name_with_extension = !template_name.include?('.') && extension ? "#{template_name}.#{extension}" : template_name - @@exempt_from_layout.any? { |ext| name_with_extension =~ ext } + template_name = @template.pick_template(template_name).to_s if @template + @@exempt_from_layout.any? { |ext| template_name =~ ext } end def default_template_name(action_name = self.action_name) diff --git a/actionpack/lib/action_controller/layout.rb b/actionpack/lib/action_controller/layout.rb index d0c717ff67..8b6febe254 100644 --- a/actionpack/lib/action_controller/layout.rb +++ b/actionpack/lib/action_controller/layout.rb @@ -304,7 +304,7 @@ module ActionController #:nodoc: end def layout_directory?(layout_name) - @template.view_paths.find_template_file_for_path("#{File.join('layouts', layout_name)}.#{@template.template_format}.erb") ? true : false + @template.file_exists?("#{File.join('layouts', layout_name)}.#{@template.template_format}") end end end diff --git a/actionpack/lib/action_controller/test_process.rb b/actionpack/lib/action_controller/test_process.rb index 8ae73a66f4..52884a93f4 100644 --- a/actionpack/lib/action_controller/test_process.rb +++ b/actionpack/lib/action_controller/test_process.rb @@ -207,13 +207,9 @@ module ActionController #:nodoc: # Returns the template path of the file which was used to # render this response (or nil) - def rendered_file(with_controller=false) - unless template.first_render.nil? - unless with_controller - template.first_render - else - template.first_render.split('/').last || template.first_render - end + def rendered_file(with_controller = false) + if template.first_render + template.first_render.to_s end end diff --git a/actionpack/lib/action_view.rb b/actionpack/lib/action_view.rb index 8f928d6fdf..9ab615c7a5 100644 --- a/actionpack/lib/action_view.rb +++ b/actionpack/lib/action_view.rb @@ -21,14 +21,14 @@ # WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. #++ -require 'action_view/template_handlers' -require 'action_view/template_file' -require 'action_view/view_load_paths' +require 'action_view/template_handlers' require 'action_view/renderable' +require 'action_view/renderable_partial' + require 'action_view/template' -require 'action_view/partial_template' require 'action_view/inline_template' +require 'action_view/paths' require 'action_view/base' require 'action_view/partials' diff --git a/actionpack/lib/action_view/base.rb b/actionpack/lib/action_view/base.rb index 7ef90ddf5e..9d4d897dbc 100644 --- a/actionpack/lib/action_view/base.rb +++ b/actionpack/lib/action_view/base.rb @@ -3,6 +3,12 @@ module ActionView #:nodoc: end class MissingTemplate < ActionViewError #:nodoc: + def initialize(paths, path, template_format = nil) + full_template_path = path.include?('.') ? path : "#{path}.erb" + display_paths = paths.join(':') + template_type = (path =~ /layouts/i) ? 'layout' : 'template' + super("Missing #{template_type} #{full_template_path} in view path #{display_paths}") + end end # Action View templates can be written in three ways. If the template file has a .erb (or .rhtml) extension then it uses a mixture of ERb @@ -216,12 +222,14 @@ module ActionView #:nodoc: attr_reader :view_paths def view_paths=(paths) - @view_paths = ViewLoadPaths.new(Array(paths)) + @view_paths = PathSet.new(Array(paths)) end # Renders the template present at template_path (relative to the view_paths array). # The hash in local_assigns is made available as local variables. def render(options = {}, local_assigns = {}, &block) #:nodoc: + local_assigns ||= {} + if options.is_a?(String) render_file(options, nil, local_assigns) elsif options == :update @@ -270,21 +278,40 @@ module ActionView #:nodoc: end def file_exists?(template_path) - view_paths.template_exists?(template_file_from_name(template_path)) + pick_template(template_path) ? true : false + rescue MissingTemplate + false end # Gets the extension for an existing template with the given template_path. # Returns the format with the extension if that template exists. # - # pick_template_extension('users/show') - # # => 'html.erb' + # pick_template('users/show') + # # => 'users/show.html.erb' # - # pick_template_extension('users/legacy') - # # => "rhtml" + # pick_template('users/legacy') + # # => 'users/legacy.rhtml' # - def pick_template_extension(template_path) - if template = template_file_from_name(template_path) - template.extension + def pick_template(template_path) + path = template_path.sub(/^\//, '') + if m = path.match(/(.*)\.(\w+)$/) + template_file_name, template_file_extension = m[1], m[2] + else + template_file_name = path + end + + # OPTIMIZE: Checks to lookup template in view path + if template = self.view_paths["#{template_file_name}.#{template_format}"] + template + elsif template = self.view_paths[template_file_name] + template + elsif first_render && template = self.view_paths["#{template_file_name}.#{first_render.extension}"] + template + elsif template_format == :js && template = self.view_paths["#{template_file_name}.html"] + @template_format = :html + template + else + Template.new(template_path, view_paths) end end @@ -292,6 +319,10 @@ module ActionView #:nodoc: # Renders the template present at template_path. The hash in local_assigns # is made available as local variables. def render_file(template_path, use_full_path = nil, local_assigns = {}) #:nodoc: + unless use_full_path == nil + ActiveSupport::Deprecation.warn("use_full_path option has been deprecated and has no affect.", caller) + end + if defined?(ActionMailer) && defined?(ActionMailer::Base) && controller.is_a?(ActionMailer::Base) && !template_path.include?("/") raise ActionViewError, <<-END_ERROR Due to changes in ActionMailer, you need to provide the mailer_name along with the template name. @@ -305,11 +336,12 @@ module ActionView #:nodoc: END_ERROR end - Template.new(self, template_path, use_full_path, local_assigns).render_template + template = pick_template(template_path) + template.render_template(self, local_assigns) end def render_inline(text, local_assigns = {}, type = nil) - InlineTemplate.new(self, text, local_assigns, type).render + InlineTemplate.new(text, type).render(self, local_assigns) end def wrap_content_for_layout(content) @@ -333,32 +365,9 @@ module ActionView #:nodoc: end def execute(template, local_assigns = {}) - send(template.method, local_assigns) do |*names| + send(template.method(local_assigns), local_assigns) do |*names| instance_variable_get "@content_for_#{names.first || 'layout'}" end end - - def template_file_from_name(template_name) - template_name = TemplateFile.from_path(template_name) - pick_template(template_name) unless template_name.extension - end - - def pick_template(file) - if f = self.view_paths.find_template_file_for_path(file.dup_with_extension(template_format)) || file_from_first_render(file) - f - elsif template_format == :js && f = self.view_paths.find_template_file_for_path(file.dup_with_extension(:html)) - @template_format = :html - f - else - nil - end - end - - # Determine the template extension from the @first_render filename - def file_from_first_render(file) - if extension = File.basename(@first_render.to_s)[/^[^.]+\.(.+)$/, 1] - file.dup_with_extension(extension) - end - end end end diff --git a/actionpack/lib/action_view/inline_template.rb b/actionpack/lib/action_view/inline_template.rb index 3e25f6902d..5e00cef13f 100644 --- a/actionpack/lib/action_view/inline_template.rb +++ b/actionpack/lib/action_view/inline_template.rb @@ -2,15 +2,18 @@ module ActionView #:nodoc: class InlineTemplate #:nodoc: include Renderable - def initialize(view, source, locals = {}, type = nil) - @view = view + attr_reader :source, :extension, :method_segment + def initialize(source, type = nil) @source = source @extension = type - @locals = locals || {} - @method_segment = "inline_#{@source.hash.abs}" - @handler = Template.handler_class_for_extension(@extension).new(@view) end + + private + # Always recompile inline templates + def recompile?(local_assigns) + true + end end end diff --git a/actionpack/lib/action_view/partial_template.rb b/actionpack/lib/action_view/partial_template.rb deleted file mode 100644 index 72f831e937..0000000000 --- a/actionpack/lib/action_view/partial_template.rb +++ /dev/null @@ -1,69 +0,0 @@ -module ActionView #:nodoc: - class PartialTemplate < Template #:nodoc: - attr_reader :variable_name, :object, :as - - def initialize(view, partial_path, object = nil, locals = {}, as = nil) - @view_controller = view.controller if view.respond_to?(:controller) - @as = as - set_path_and_variable_name!(partial_path) - super(view, @path, nil, locals) - add_object_to_local_assigns!(object) - - # This is needed here in order to compile template with knowledge of 'counter' - initialize_counter! - - # Prepare early. This is a performance optimization for partial collections - prepare! - end - - def render - ActionController::Base.benchmark("Rendered #{@path.path_without_format_and_extension}", Logger::DEBUG, false) do - super - end - end - - def render_member(object) - @locals[:object] = @locals[@variable_name] = object - @locals[as] = object if as - - template = render_template - @locals[@counter_name] += 1 - @locals.delete(as) - @locals.delete(@variable_name) - @locals.delete(:object) - - template - end - - def counter=(num) - @locals[@counter_name] = num - end - - private - def add_object_to_local_assigns!(object) - @locals[:object] ||= - @locals[@variable_name] ||= object || @view_controller.instance_variable_get("@#{variable_name}") - @locals[as] ||= @locals[:object] if as - end - - def set_path_and_variable_name!(partial_path) - if partial_path.include?('/') - @variable_name = File.basename(partial_path) - @path = "#{File.dirname(partial_path)}/_#{@variable_name}" - elsif @view_controller - @variable_name = partial_path - @path = "#{@view_controller.class.controller_path}/_#{@variable_name}" - else - @variable_name = partial_path - @path = "_#{@variable_name}" - end - - @variable_name = @variable_name.sub(/\..*$/, '').to_sym - end - - def initialize_counter! - @counter_name ||= "#{@variable_name}_counter".to_sym - @locals[@counter_name] = 0 - end - end -end diff --git a/actionpack/lib/action_view/partials.rb b/actionpack/lib/action_view/partials.rb index 7c6c98d611..116d61e13b 100644 --- a/actionpack/lib/action_view/partials.rb +++ b/actionpack/lib/action_view/partials.rb @@ -104,10 +104,12 @@ module ActionView module Partials private def render_partial(partial_path, object_assigns = nil, local_assigns = {}) #:nodoc: + local_assigns ||= {} + case partial_path when String, Symbol, NilClass - # Render the template - ActionView::PartialTemplate.new(self, partial_path, object_assigns, local_assigns).render_template + variable_name, path = partial_pieces(partial_path) + pick_template(path).render_partial(self, variable_name, object_assigns, local_assigns) when ActionView::Helpers::FormBuilder builder_partial_path = partial_path.class.to_s.demodulize.underscore.sub(/_builder$/, '') render_partial(builder_partial_path, object_assigns, (local_assigns || {}).merge(builder_partial_path.to_sym => partial_path)) @@ -128,31 +130,43 @@ module ActionView local_assigns = local_assigns ? local_assigns.clone : {} spacer = partial_spacer_template ? render(:partial => partial_spacer_template) : '' + _partial_pieces = {} + _templates = {} - if partial_path.nil? - render_partial_collection_with_unknown_partial_path(collection, local_assigns, as) - else - render_partial_collection_with_known_partial_path(collection, partial_path, local_assigns, as) + index = 0 + collection.map do |object| + _partial_path ||= partial_path || ActionController::RecordIdentifier.partial_path(object, controller.class.controller_path) + variable_name, path = _partial_pieces[_partial_path] ||= partial_pieces(_partial_path) + template = _templates[path] ||= pick_template(path) + + local_assigns["#{variable_name}_counter".to_sym] = index + local_assigns[:object] = local_assigns[variable_name] = object + local_assigns[as] = object if as + + result = template.render_partial(self, variable_name, object, local_assigns) + + local_assigns.delete(as) + local_assigns.delete(variable_name) + local_assigns.delete(:object) + index += 1 + + result end.join(spacer) end - def render_partial_collection_with_known_partial_path(collection, partial_path, local_assigns, as) - template = ActionView::PartialTemplate.new(self, partial_path, nil, local_assigns, as) - collection.map do |element| - template.render_member(element) - end - end - - def render_partial_collection_with_unknown_partial_path(collection, local_assigns, as) - templates = Hash.new - i = 0 - collection.map do |element| - partial_path = ActionController::RecordIdentifier.partial_path(element, controller.class.controller_path) - template = templates[partial_path] ||= ActionView::PartialTemplate.new(self, partial_path, nil, local_assigns, as) - template.counter = i - i += 1 - template.render_member(element) + def partial_pieces(partial_path) + if partial_path.include?('/') + variable_name = File.basename(partial_path) + path = "#{File.dirname(partial_path)}/_#{variable_name}" + elsif respond_to?(:controller) + variable_name = partial_path + path = "#{controller.class.controller_path}/_#{variable_name}" + else + variable_name = partial_path + path = "_#{variable_name}" end + variable_name = variable_name.sub(/\..*$/, '').to_sym + return variable_name, path end end end diff --git a/actionpack/lib/action_view/paths.rb b/actionpack/lib/action_view/paths.rb new file mode 100644 index 0000000000..0fa7d3dad3 --- /dev/null +++ b/actionpack/lib/action_view/paths.rb @@ -0,0 +1,85 @@ +module ActionView #:nodoc: + class PathSet < Array #:nodoc: + def self.type_cast(obj) + obj.is_a?(String) ? Path.new(obj) : obj + end + + class Path #:nodoc: + attr_reader :path, :paths + delegate :to_s, :to_str, :inspect, :to => :path + + def initialize(path) + @path = path.freeze + reload! + end + + def ==(path) + to_str == path.to_str + end + + def [](path) + @paths[path] + end + + # Rebuild load path directory cache + def reload! + @paths = {} + + templates_in_path do |template| + @paths[template.path] = template + @paths[template.path_without_extension] ||= template + end + + @paths.freeze + end + + private + def templates_in_path + (Dir.glob("#{@path}/**/*/**") | Dir.glob("#{@path}/**")).each do |file| + unless File.directory?(file) + template = Template.new(file.split("#{self}/").last, self) + # Eager load memoized methods and freeze cached template + template.freeze if Base.cache_template_loading + yield template + end + end + end + end + + def initialize(*args) + super(*args).map! { |obj| self.class.type_cast(obj) } + end + + def reload! + each { |path| path.reload! } + end + + def <<(obj) + super(self.class.type_cast(obj)) + end + + def push(*objs) + delete_paths!(objs) + super(*objs.map { |obj| self.class.type_cast(obj) }) + end + + def unshift(*objs) + delete_paths!(objs) + super(*objs.map { |obj| self.class.type_cast(obj) }) + end + + def [](template_path) + each do |path| + if template = path[template_path] + return template + end + end + nil + end + + private + def delete_paths!(paths) + paths.each { |p1| delete_if { |p2| p1.to_s == p2.to_s } } + end + end +end diff --git a/actionpack/lib/action_view/renderable.rb b/actionpack/lib/action_view/renderable.rb index 6a8a0e44fc..2c4302146f 100644 --- a/actionpack/lib/action_view/renderable.rb +++ b/actionpack/lib/action_view/renderable.rb @@ -1,37 +1,78 @@ module ActionView module Renderable - # TODO: Local assigns should not be tied to template instance - attr_accessor :locals + # NOTE: The template that this mixin is beening include into is frozen + # So you can not set or modify any instance variables - # TODO: These readers should be private - attr_reader :filename, :source, :handler - - def render - prepare! - @handler.render(self, @locals) + def self.included(base) + @@mutex = Mutex.new end - def method - ['_run', @extension, @method_segment, local_assigns_keys].compact.join('_').to_sym + # NOTE: Exception to earlier notice. Ensure this is called before freeze + def handler + @handler ||= Template.handler_class_for_extension(extension) + end + + # NOTE: Exception to earlier notice. Ensure this is called before freeze + def compiled_source + @compiled_source ||= handler.new(nil).compile(self) if handler.compilable? + end + + def render(view, local_assigns = {}) + view.first_render ||= self + view.send(:evaluate_assigns) + view.current_render_extension = extension + compile(local_assigns) if handler.compilable? + handler.new(view).render(self, local_assigns) + end + + def method(local_assigns) + if local_assigns && local_assigns.any? + local_assigns_keys = "locals_#{local_assigns.keys.map { |k| k.to_s }.sort.join('_')}" + end + ['_run', extension, method_segment, local_assigns_keys].compact.join('_').to_sym end private - def prepare! - unless @prepared - @view.send(:evaluate_assigns) - @view.current_render_extension = @extension + # Compile and evaluate the template's code + def compile(local_assigns) + render_symbol = method(local_assigns) - if @handler.compilable? - @handler.compile_template(self) # compile the given template, if necessary + @@mutex.synchronize do + return false unless recompile?(render_symbol) + + locals_code = local_assigns.keys.map { |key| "#{key} = local_assigns[:#{key}];" }.join + + source = <<-end_src + def #{render_symbol}(local_assigns) + old_output_buffer = output_buffer;#{locals_code};#{compiled_source} + ensure + self.output_buffer = old_output_buffer + end + end_src + + begin + file_name = respond_to?(:filename) ? filename : 'compiled-template' + ActionView::Base::CompiledTemplates.module_eval(source, file_name, 0) + rescue Exception => e # errors from template code + if logger = ActionController::Base.logger + logger.debug "ERROR: compiling #{render_symbol} RAISED #{e}" + logger.debug "Function body: #{source}" + logger.debug "Backtrace: #{e.backtrace.join("\n")}" + end + + raise ActionView::TemplateError.new(self, {}, e) end - - @prepared = true end end - def local_assigns_keys - if @locals && @locals.any? - "locals_#{@locals.keys.map { |k| k.to_s }.sort.join('_')}" + # Method to check whether template compilation is necessary. + # The template will be compiled if the file has not been compiled yet, or + # if local_assigns has a new key, which isn't supported by the compiled code yet. + def recompile?(symbol) + unless Base::CompiledTemplates.instance_methods.include?(symbol) && Base.cache_template_loading + true + else + false end end end diff --git a/actionpack/lib/action_view/renderable_partial.rb b/actionpack/lib/action_view/renderable_partial.rb new file mode 100644 index 0000000000..6a17b50a14 --- /dev/null +++ b/actionpack/lib/action_view/renderable_partial.rb @@ -0,0 +1,19 @@ +module ActionView + module RenderablePartial + # NOTE: The template that this mixin is beening include into is frozen + # So you can not set or modify any instance variables + + def render(view, local_assigns = {}) + ActionController::Base.benchmark("Rendered #{path_without_format_and_extension}", Logger::DEBUG, false) do + super + end + end + + def render_partial(view, variable_name, object = nil, local_assigns = {}, as = nil) + object ||= view.controller.instance_variable_get("@#{variable_name}") if view.respond_to?(:controller) + local_assigns[:object] ||= local_assigns[variable_name] ||= object + local_assigns[as] ||= local_assigns[:object] if as + render_template(view, local_assigns) + end + end +end diff --git a/actionpack/lib/action_view/template.rb b/actionpack/lib/action_view/template.rb index 5e5ea9b9b9..03f9234289 100644 --- a/actionpack/lib/action_view/template.rb +++ b/actionpack/lib/action_view/template.rb @@ -1,82 +1,112 @@ module ActionView #:nodoc: - class Template #:nodoc: + class Template extend TemplateHandlers include Renderable - attr_reader :path, :extension + attr_accessor :filename, :load_path, :base_path, :name, :format, :extension + delegate :to_s, :to => :path - def initialize(view, path, use_full_path = nil, locals = {}) - unless use_full_path == nil - ActiveSupport::Deprecation.warn("use_full_path option has been deprecated and has no affect.", caller) - end + def initialize(template_path, load_paths = []) + template_path = template_path.dup + @base_path, @name, @format, @extension = split(template_path) + @base_path.to_s.gsub!(/\/$/, '') # Push to split method + @load_path, @filename = find_full_path(template_path, load_paths) - @view = view - @paths = view.view_paths - - @original_path = path - @path = TemplateFile.from_path(path) - @view.first_render ||= @path.to_s - - set_extension_and_file_name - - @method_segment = compiled_method_name_file_path_segment - @locals = (locals && locals.dup) || {} - @handler = self.class.handler_class_for_extension(@extension).new(@view) + # Extend with partial super powers + extend RenderablePartial if @name =~ /^_/ end - def render_template - render - rescue Exception => e - raise e unless filename - if TemplateError === e - e.sub_template_of(filename) - raise e - else - raise TemplateError.new(self, @view.assigns, e) - end + def freeze + # Eager load memoized methods + format_and_extension + path + path_without_extension + path_without_format_and_extension + source + method_segment + + # Eager load memoized methods from Renderable + handler + compiled_source + + instance_variables.each { |ivar| ivar.freeze } + + super + end + + def format_and_extension + @format_and_extension ||= (extensions = [format, extension].compact.join(".")).blank? ? nil : extensions + end + + def path + @path ||= [base_path, [name, format, extension].compact.join('.')].compact.join('/') + end + + def path_without_extension + @path_without_extension ||= [base_path, [name, format].compact.join('.')].compact.join('/') + end + + def path_without_format_and_extension + @path_without_format_and_extension ||= [base_path, name].compact.join('/') end def source @source ||= File.read(@filename) end - def base_path_for_exception - (@paths.find_load_path_for_path(@path) || @paths.first).to_s + def method_segment + unless @method_segment + segment = File.expand_path(@filename) + segment.sub!(/^#{Regexp.escape(File.expand_path(RAILS_ROOT))}/, '') if defined?(RAILS_ROOT) + segment.gsub!(/([^a-zA-Z0-9_])/) { $1.ord } + @method_segment = segment + end + + @method_segment + end + + def render_template(view, local_assigns = {}) + render(view, local_assigns) + rescue Exception => e + raise e unless filename + if TemplateError === e + e.sub_template_of(filename) + raise e + else + raise TemplateError.new(self, view.assigns, e) + end end private - def set_extension_and_file_name - @extension = @path.extension - - unless @extension - @path = @view.send(:template_file_from_name, @path) - raise_missing_template_exception unless @path - @extension = @path.extension - end - - if p = @paths.find_template_file_for_path(path) - @path = p - @filename = @path.full_path - @extension = @path.extension - raise_missing_template_exception if @filename.blank? - else - @filename = @original_path - raise_missing_template_exception unless File.exist?(@filename) - end + def valid_extension?(extension) + Template.template_handler_extensions.include?(extension) end - def raise_missing_template_exception - full_template_path = @original_path.include?('.') ? @original_path : "#{@original_path}.#{@view.template_format}.erb" - display_paths = @paths.join(':') - template_type = (@original_path =~ /layouts/i) ? 'layout' : 'template' - raise MissingTemplate, "Missing #{template_type} #{full_template_path} in view path #{display_paths}" + def find_full_path(path, load_paths) + load_paths = Array(load_paths) + [nil] + load_paths.each do |load_path| + file = [load_path, path].compact.join('/') + return load_path, file if File.exist?(file) + end + raise MissingTemplate.new(load_paths, path) end - def compiled_method_name_file_path_segment - s = File.expand_path(@filename) - s.sub!(/^#{Regexp.escape(File.expand_path(RAILS_ROOT))}/, '') if defined?(RAILS_ROOT) - s.gsub!(/([^a-zA-Z0-9_])/) { $1.ord } - s + # Returns file split into an array + # [base_path, name, format, extension] + def split(file) + if m = file.match(/^(.*\/)?([^\.]+)\.?(\w+)?\.?(\w+)?\.?(\w+)?$/) + if m[5] # Mulipart formats + [m[1], m[2], "#{m[3]}.#{m[4]}", m[5]] + elsif m[4] # Single format + [m[1], m[2], m[3], m[4]] + else + if valid_extension?(m[3]) # No format + [m[1], m[2], nil, m[3]] + else # No extension + [m[1], m[2], m[3], nil] + end + end + end end end end diff --git a/actionpack/lib/action_view/template_error.rb b/actionpack/lib/action_view/template_error.rb index a1be1a8833..35fc07bdb2 100644 --- a/actionpack/lib/action_view/template_error.rb +++ b/actionpack/lib/action_view/template_error.rb @@ -7,7 +7,7 @@ module ActionView attr_reader :original_exception def initialize(template, assigns, original_exception) - @base_path = template.base_path_for_exception + @base_path = template.base_path @assigns, @source, @original_exception = assigns.dup, template.source, original_exception @file_path = template.filename @backtrace = compute_backtrace diff --git a/actionpack/lib/action_view/template_file.rb b/actionpack/lib/action_view/template_file.rb deleted file mode 100644 index 0aa16b5e70..0000000000 --- a/actionpack/lib/action_view/template_file.rb +++ /dev/null @@ -1,88 +0,0 @@ -module ActionView #:nodoc: - # TemplateFile abstracts the pattern of querying a file path for its - # path with or without its extension. The path is only the partial path - # from the load path root e.g. "hello/index.html.erb" not - # "app/views/hello/index.html.erb" - class TemplateFile - def self.from_path(path) - path.is_a?(self) ? path : new(path) - end - - def self.from_full_path(load_path, full_path) - file = new(full_path.split(load_path).last) - file.load_path = load_path - file.freeze - end - - attr_accessor :load_path, :base_path, :name, :format, :extension - delegate :to_s, :inspect, :to => :path - - def initialize(path) - path = path.dup - - # Clear the forward slash in the beginning - trim_forward_slash!(path) - - @base_path, @name, @format, @extension = split(path) - end - - def freeze - @load_path.freeze - @base_path.freeze - @name.freeze - @format.freeze - @extension.freeze - super - end - - def format_and_extension - extensions = [format, extension].compact.join(".") - extensions.blank? ? nil : extensions - end - - def full_path - if load_path - "#{load_path}/#{path}" - else - path - end - end - - def path - base_path.to_s + [name, format, extension].compact.join(".") - end - - def path_without_extension - base_path.to_s + [name, format].compact.join(".") - end - - def path_without_format_and_extension - "#{base_path}#{name}" - end - - def dup_with_extension(extension) - file = dup - file.extension = extension ? extension.to_s : nil - file - end - - private - def trim_forward_slash!(path) - path.sub!(/^\//, '') - end - - # Returns file split into an array - # [base_path, name, format, extension] - def split(file) - if m = file.match(/^(.*\/)?([^\.]+)\.?(\w+)?\.?(\w+)?\.?(\w+)?$/) - if m[5] # Mulipart formats - [m[1], m[2], "#{m[3]}.#{m[4]}", m[5]] - elsif m[4] # Single format - [m[1], m[2], m[3], m[4]] - else # No format - [m[1], m[2], nil, m[3]] - end - end - end - end -end diff --git a/actionpack/lib/action_view/template_handlers/compilable.rb b/actionpack/lib/action_view/template_handlers/compilable.rb index 95ae75ca68..a0ebaefeef 100644 --- a/actionpack/lib/action_view/template_handlers/compilable.rb +++ b/actionpack/lib/action_view/template_handlers/compilable.rb @@ -3,8 +3,6 @@ module ActionView module Compilable def self.included(base) base.extend ClassMethod - - @@mutex = Mutex.new end module ClassMethod @@ -17,54 +15,6 @@ module ActionView def render(template, local_assigns = {}) @view.send(:execute, template, local_assigns) end - - # Compile and evaluate the template's code - def compile_template(template) - return false unless recompile_template?(template) - - @@mutex.synchronize do - locals_code = template.locals.keys.map { |key| "#{key} = local_assigns[:#{key}];" }.join - - source = <<-end_src - def #{template.method}(local_assigns) - old_output_buffer = output_buffer;#{locals_code};#{compile(template)} - ensure - self.output_buffer = old_output_buffer - end - end_src - - begin - file_name = template.filename || 'compiled-template' - ActionView::Base::CompiledTemplates.module_eval(source, file_name, 0) - rescue Exception => e # errors from template code - if logger = ActionController::Base.logger - logger.debug "ERROR: compiling #{template.method} RAISED #{e}" - logger.debug "Function body: #{source}" - logger.debug "Backtrace: #{e.backtrace.join("\n")}" - end - - raise ActionView::TemplateError.new(template, @view.assigns, e) - end - end - end - - private - # Method to check whether template compilation is necessary. - # The template will be compiled if the inline template or file has not been compiled yet, - # if local_assigns has a new key, which isn't supported by the compiled code yet. - def recompile_template?(template) - # Unless the template has been complied yet, compile - return true unless Base::CompiledTemplates.instance_methods.include?(template.method.to_s) - - # If template caching is disabled, compile - return true unless Base.cache_template_loading - - # Always recompile inline templates - return true if template.is_a?(InlineTemplate) - - # Otherwise, use compiled method - return false - end end end end diff --git a/actionpack/lib/action_view/view_load_paths.rb b/actionpack/lib/action_view/view_load_paths.rb deleted file mode 100644 index 6e439a009c..0000000000 --- a/actionpack/lib/action_view/view_load_paths.rb +++ /dev/null @@ -1,103 +0,0 @@ -module ActionView #:nodoc: - class ViewLoadPaths < Array #:nodoc: - def self.type_cast(obj) - obj.is_a?(String) ? LoadPath.new(obj) : obj - end - - class LoadPath #:nodoc: - attr_reader :path, :paths - delegate :to_s, :to_str, :inspect, :to => :path - - def initialize(path) - @path = path.freeze - reload! - end - - def ==(path) - to_str == path.to_str - end - - # Rebuild load path directory cache - def reload! - @paths = {} - - files.each do |file| - @paths[file.path] = file - @paths[file.path_without_extension] ||= file - end - - @paths.freeze - end - - def find_template_file_for_partial_path(template_path, template_format) - @paths["#{template_path}.#{template_format}"] || - @paths[template_path] || - @paths[template_path.gsub(/\..*$/, '')] - end - - private - # Get all the files and directories in the path - def files_in_path - Dir.glob("#{@path}/**/*/**") | Dir.glob("#{@path}/**") - end - - # Create an array of all the files within the path - def files - files_in_path.map do |file| - TemplateFile.from_full_path(@path, file) unless File.directory?(file) - end.compact - end - end - - def initialize(*args) - super(*args).map! { |obj| self.class.type_cast(obj) } - end - - def reload! - each { |path| path.reload! } - end - - def <<(obj) - super(self.class.type_cast(obj)) - end - - def push(*objs) - delete_paths!(objs) - super(*objs.map { |obj| self.class.type_cast(obj) }) - end - - def unshift(*objs) - delete_paths!(objs) - super(*objs.map { |obj| self.class.type_cast(obj) }) - end - - def template_exists?(file) - find_load_path_for_path(file) ? true : false - end - - def find_load_path_for_path(file) - find { |path| path.paths[file.to_s] } - end - - def find_template_file_for_path(template_path) - template_path_without_extension, template_extension = path_and_extension(template_path.to_s) - each do |path| - if f = path.find_template_file_for_partial_path(template_path_without_extension, template_extension) - return f - end - end - nil - end - - private - def delete_paths!(paths) - paths.each { |p1| delete_if { |p2| p1.to_s == p2.to_s } } - end - - # Splits the path and extension from the given template_path and returns as an array. - def path_and_extension(template_path) - template_path_without_extension = template_path.sub(/\.(\w+)$/, '') - [template_path_without_extension, $1] - end - end -end diff --git a/actionpack/test/controller/layout_test.rb b/actionpack/test/controller/layout_test.rb index 32be7d90ff..92b6aa4f2f 100644 --- a/actionpack/test/controller/layout_test.rb +++ b/actionpack/test/controller/layout_test.rb @@ -63,6 +63,7 @@ class LayoutAutoDiscoveryTest < Test::Unit::TestCase end def test_third_party_template_library_auto_discovers_layout + ThirdPartyTemplateLibraryController.view_paths.reload! @controller = ThirdPartyTemplateLibraryController.new get :hello assert_equal 'layouts/third_party_template_library', @controller.active_layout diff --git a/railties/lib/initializer.rb b/railties/lib/initializer.rb index d80460d4bc..18bcf69d69 100644 --- a/railties/lib/initializer.rb +++ b/railties/lib/initializer.rb @@ -137,12 +137,12 @@ module Rails initialize_logger initialize_framework_logging - initialize_framework_views initialize_dependency_mechanism initialize_whiny_nils initialize_temporary_session_directory initialize_time_zone initialize_framework_settings + initialize_framework_views add_support_load_paths From 99cc85bc099a757cdd44e4f5f1be4972ab124e0d Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sat, 12 Jul 2008 15:31:50 -0500 Subject: [PATCH 37/74] Set config.action_view.warn_cache_misses = true to receive a warning if you perform an action that results in an expensive disk operation that could be cached --- actionpack/CHANGELOG | 2 ++ actionpack/lib/action_controller/base.rb | 4 ++-- actionpack/lib/action_view/base.rb | 22 ++++++++++++++++++++-- actionpack/lib/action_view/paths.rb | 13 ++++++++++++- 4 files changed, 36 insertions(+), 5 deletions(-) diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG index 507a2b69bf..5b7bfe9c30 100644 --- a/actionpack/CHANGELOG +++ b/actionpack/CHANGELOG @@ -1,5 +1,7 @@ *Edge* +* Set config.action_view.warn_cache_misses = true to receive a warning if you perform an action that results in an expensive disk operation that could be cached [Josh Peek] + * Refactor template preloading. New abstractions include Renderable mixins and a refactored Template class [Josh Peek] * Changed ActionView::TemplateHandler#render API method signature to render(template, local_assigns = {}) [Josh Peek] diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index 6926941396..df94f78f18 100755 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -431,7 +431,7 @@ module ActionController #:nodoc: end def view_paths=(value) - @view_paths = ActionView::PathSet.new(Array(value)) if value + @view_paths = ActionView::Base.process_view_paths(value) if value end # Adds a view_path to the front of the view_paths array. @@ -652,7 +652,7 @@ module ActionController #:nodoc: end def view_paths=(value) - @template.view_paths = PathSet.new(value) + @template.view_paths = ActionView::Base.process_view_paths(value) end # Adds a view_path to the front of the view_paths array. diff --git a/actionpack/lib/action_view/base.rb b/actionpack/lib/action_view/base.rb index 9d4d897dbc..989e92a890 100644 --- a/actionpack/lib/action_view/base.rb +++ b/actionpack/lib/action_view/base.rb @@ -185,6 +185,10 @@ module ActionView #:nodoc: @@debug_rjs = false cattr_accessor :debug_rjs + # A warning will be displayed whenever an action results in a cache miss on your view paths. + @@warn_cache_misses = false + cattr_accessor :warn_cache_misses + attr_internal :request delegate :request_forgery_protection_token, :template, :params, :session, :cookies, :response, :headers, @@ -212,6 +216,10 @@ module ActionView #:nodoc: return helpers end + def self.process_view_paths(value) + ActionView::PathSet.new(Array(value)) + end + def initialize(view_paths = [], assigns_for_first_render = {}, controller = nil)#:nodoc: @assigns = assigns_for_first_render @assigns_added = nil @@ -222,7 +230,7 @@ module ActionView #:nodoc: attr_reader :view_paths def view_paths=(paths) - @view_paths = PathSet.new(Array(paths)) + @view_paths = self.class.process_view_paths(paths) end # Renders the template present at template_path (relative to the view_paths array). @@ -311,7 +319,17 @@ module ActionView #:nodoc: @template_format = :html template else - Template.new(template_path, view_paths) + template = Template.new(template_path, view_paths) + + if self.class.warn_cache_misses && logger = ActionController::Base.logger + logger.debug "[PERFORMANCE] Rendering a template that was " + + "not found in view path. Templates outside the view path are " + + "not cached and result in expensive disk operations. Move this " + + "file into #{view_paths.join(':')} or add the folder to your " + + "view path list" + end + + template end end diff --git a/actionpack/lib/action_view/paths.rb b/actionpack/lib/action_view/paths.rb index 0fa7d3dad3..b0ab7d0c67 100644 --- a/actionpack/lib/action_view/paths.rb +++ b/actionpack/lib/action_view/paths.rb @@ -1,7 +1,18 @@ module ActionView #:nodoc: class PathSet < Array #:nodoc: def self.type_cast(obj) - obj.is_a?(String) ? Path.new(obj) : obj + if obj.is_a?(String) + if Base.warn_cache_misses && defined?(Rails) && Rails.initialized? + Rails.logger.debug "[PERFORMANCE] Processing view path during a " + + "request. This an expense disk operation that should be done at " + + "boot. You can manually process this view path with " + + "ActionView::Base.process_view_paths(#{obj.inspect}) and set it " + + "as your view path" + end + Path.new(obj) + else + obj + end end class Path #:nodoc: From e0fef66149092dd3d2988fff64f0ce8765735687 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sun, 13 Jul 2008 13:26:48 -0500 Subject: [PATCH 38/74] Made ActionView::Base#first_render a little more private. And added _last_render to track the most recent render. Will fix #609 as a side effect. [#609 state:resolved] --- .../assertions/response_assertions.rb | 4 ++-- actionpack/lib/action_controller/test_process.rb | 15 ++++----------- actionpack/lib/action_view/base.rb | 6 +++--- .../lib/action_view/helpers/cache_helper.rb | 3 +-- actionpack/lib/action_view/renderable.rb | 4 ++-- .../controller/action_pack_assertions_test.rb | 6 +++--- actionpack/test/controller/caching_test.rb | 8 ++++++++ .../inline_fragment_cached.html.erb | 2 ++ 8 files changed, 25 insertions(+), 23 deletions(-) create mode 100644 actionpack/test/fixtures/functional_caching/inline_fragment_cached.html.erb diff --git a/actionpack/lib/action_controller/assertions/response_assertions.rb b/actionpack/lib/action_controller/assertions/response_assertions.rb index cb36405700..a98c70d66f 100644 --- a/actionpack/lib/action_controller/assertions/response_assertions.rb +++ b/actionpack/lib/action_controller/assertions/response_assertions.rb @@ -87,11 +87,11 @@ module ActionController # def assert_template(expected = nil, message=nil) clean_backtrace do - rendered = expected ? @response.rendered_file(!expected.include?('/')) : @response.rendered_file + rendered = @response.rendered_template.to_s msg = build_message(message, "expecting but rendering with ", expected, rendered) assert_block(msg) do if expected.nil? - !@response.rendered_with_file? + @response.rendered_template ? true : false else rendered.match(expected) end diff --git a/actionpack/lib/action_controller/test_process.rb b/actionpack/lib/action_controller/test_process.rb index 52884a93f4..a6e0c98936 100644 --- a/actionpack/lib/action_controller/test_process.rb +++ b/actionpack/lib/action_controller/test_process.rb @@ -205,17 +205,10 @@ module ActionController #:nodoc: p.match(redirect_url) != nil end - # Returns the template path of the file which was used to - # render this response (or nil) - def rendered_file(with_controller = false) - if template.first_render - template.first_render.to_s - end - end - - # Was this template rendered by a file? - def rendered_with_file? - !rendered_file.nil? + # Returns the template of the file which was used to + # render this response (or nil) + def rendered_template + template._first_render end # A shortcut to the flash. Returns an empyt hash if no session flash exists. diff --git a/actionpack/lib/action_view/base.rb b/actionpack/lib/action_view/base.rb index 989e92a890..9f244d7250 100644 --- a/actionpack/lib/action_view/base.rb +++ b/actionpack/lib/action_view/base.rb @@ -159,11 +159,11 @@ module ActionView #:nodoc: class Base include ERB::Util - attr_accessor :base_path, :assigns, :template_extension, :first_render + attr_accessor :base_path, :assigns, :template_extension attr_accessor :controller + attr_accessor :_first_render, :_last_render attr_writer :template_format - attr_accessor :current_render_extension attr_accessor :output_buffer @@ -313,7 +313,7 @@ module ActionView #:nodoc: template elsif template = self.view_paths[template_file_name] template - elsif first_render && template = self.view_paths["#{template_file_name}.#{first_render.extension}"] + elsif _first_render && template = self.view_paths["#{template_file_name}.#{_first_render.extension}"] template elsif template_format == :js && template = self.view_paths["#{template_file_name}.html"] @template_format = :html diff --git a/actionpack/lib/action_view/helpers/cache_helper.rb b/actionpack/lib/action_view/helpers/cache_helper.rb index 930c397785..2cdbae6e40 100644 --- a/actionpack/lib/action_view/helpers/cache_helper.rb +++ b/actionpack/lib/action_view/helpers/cache_helper.rb @@ -32,8 +32,7 @@ module ActionView # Topics listed alphabetically # <% end %> def cache(name = {}, options = nil, &block) - handler = Template.handler_class_for_extension(current_render_extension.to_sym) - handler.new(@controller).cache_fragment(block, name, options) + _last_render.handler.new(@controller).cache_fragment(block, name, options) end end end diff --git a/actionpack/lib/action_view/renderable.rb b/actionpack/lib/action_view/renderable.rb index 2c4302146f..ebb0f1b674 100644 --- a/actionpack/lib/action_view/renderable.rb +++ b/actionpack/lib/action_view/renderable.rb @@ -18,9 +18,9 @@ module ActionView end def render(view, local_assigns = {}) - view.first_render ||= self + view._first_render ||= self + view._last_render = self view.send(:evaluate_assigns) - view.current_render_extension = extension compile(local_assigns) if handler.compilable? handler.new(view).render(self, local_assigns) end diff --git a/actionpack/test/controller/action_pack_assertions_test.rb b/actionpack/test/controller/action_pack_assertions_test.rb index 610e196362..56ba36cee5 100644 --- a/actionpack/test/controller/action_pack_assertions_test.rb +++ b/actionpack/test/controller/action_pack_assertions_test.rb @@ -328,11 +328,11 @@ class ActionPackAssertionsControllerTest < Test::Unit::TestCase # check if we were rendered by a file-based template? def test_rendered_action process :nothing - assert !@response.rendered_with_file? + assert_nil @response.rendered_template process :hello_world - assert @response.rendered_with_file? - assert 'hello_world', @response.rendered_file + assert @response.rendered_template + assert 'hello_world', @response.rendered_template.to_s end # check the redirection location diff --git a/actionpack/test/controller/caching_test.rb b/actionpack/test/controller/caching_test.rb index 2e98837a35..c30f7be700 100644 --- a/actionpack/test/controller/caching_test.rb +++ b/actionpack/test/controller/caching_test.rb @@ -664,6 +664,14 @@ CACHED assert_match "Fragment caching in a partial", @store.read('views/test.host/functional_caching/html_fragment_cached_with_partial') end + def test_render_inline_before_fragment_caching + get :inline_fragment_cached + assert_response :success + assert_match /Some inline content/, @response.body + assert_match /Some cached content/, @response.body + assert_match "Some cached content", @store.read('views/test.host/functional_caching/inline_fragment_cached') + end + def test_fragment_caching_in_rjs_partials xhr :get, :js_fragment_cached_with_partial assert_response :success diff --git a/actionpack/test/fixtures/functional_caching/inline_fragment_cached.html.erb b/actionpack/test/fixtures/functional_caching/inline_fragment_cached.html.erb new file mode 100644 index 0000000000..87309b8ccb --- /dev/null +++ b/actionpack/test/fixtures/functional_caching/inline_fragment_cached.html.erb @@ -0,0 +1,2 @@ +<%= render :inline => 'Some inline content' %> +<% cache do %>Some cached content<% end %> From 26bc867151a8f302b4c6122e6375c3ea2088a037 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sun, 13 Jul 2008 14:00:40 -0500 Subject: [PATCH 39/74] Small tweak to e0fef66 --- .../lib/action_controller/assertions/response_assertions.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/actionpack/lib/action_controller/assertions/response_assertions.rb b/actionpack/lib/action_controller/assertions/response_assertions.rb index a98c70d66f..765225ae24 100644 --- a/actionpack/lib/action_controller/assertions/response_assertions.rb +++ b/actionpack/lib/action_controller/assertions/response_assertions.rb @@ -87,13 +87,13 @@ module ActionController # def assert_template(expected = nil, message=nil) clean_backtrace do - rendered = @response.rendered_template.to_s + rendered = @response.rendered_template msg = build_message(message, "expecting but rendering with ", expected, rendered) assert_block(msg) do if expected.nil? - @response.rendered_template ? true : false + @response.rendered_template.nil? else - rendered.match(expected) + rendered.to_s.match(expected) end end end From 68fe898189a27e4e3c4c2fe005c99975d40e1dd7 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sun, 13 Jul 2008 14:05:21 -0500 Subject: [PATCH 40/74] Check first render format and extension. Fixes failing ActionMailer test. --- actionpack/lib/action_view/base.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionpack/lib/action_view/base.rb b/actionpack/lib/action_view/base.rb index 9f244d7250..04e8d3a358 100644 --- a/actionpack/lib/action_view/base.rb +++ b/actionpack/lib/action_view/base.rb @@ -313,7 +313,7 @@ module ActionView #:nodoc: template elsif template = self.view_paths[template_file_name] template - elsif _first_render && template = self.view_paths["#{template_file_name}.#{_first_render.extension}"] + elsif _first_render && template = self.view_paths["#{template_file_name}.#{_first_render.format_and_extension}"] template elsif template_format == :js && template = self.view_paths["#{template_file_name}.html"] @template_format = :html From 0d241f4434bafa2107cd6c3f3ab77c05f5d5ec71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tapaj=C3=B3s?= Date: Sun, 13 Jul 2008 14:19:03 -0500 Subject: [PATCH 41/74] Use full path in database tasks so commands will work outside of Rails root [#612 state:resolved] Signed-off-by: Joshua Peek --- railties/lib/tasks/databases.rake | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/railties/lib/tasks/databases.rake b/railties/lib/tasks/databases.rake index 75fba8b45a..22b8459ce4 100644 --- a/railties/lib/tasks/databases.rake +++ b/railties/lib/tasks/databases.rake @@ -215,14 +215,14 @@ namespace :db do desc "Create a db/schema.rb file that can be portably used against any DB supported by AR" task :dump => :environment do require 'active_record/schema_dumper' - File.open(ENV['SCHEMA'] || "db/schema.rb", "w") do |file| + File.open(ENV['SCHEMA'] || "#{RAILS_ROOT}/db/schema.rb", "w") do |file| ActiveRecord::SchemaDumper.dump(ActiveRecord::Base.connection, file) end end desc "Load a schema.rb file into the database" task :load => :environment do - file = ENV['SCHEMA'] || "db/schema.rb" + file = ENV['SCHEMA'] || "#{RAILS_ROOT}/db/schema.rb" load(file) end end @@ -234,7 +234,7 @@ namespace :db do case abcs[RAILS_ENV]["adapter"] when "mysql", "oci", "oracle" ActiveRecord::Base.establish_connection(abcs[RAILS_ENV]) - File.open("db/#{RAILS_ENV}_structure.sql", "w+") { |f| f << ActiveRecord::Base.connection.structure_dump } + File.open("#{RAILS_ROOT}/db/#{RAILS_ENV}_structure.sql", "w+") { |f| f << ActiveRecord::Base.connection.structure_dump } when "postgresql" ENV['PGHOST'] = abcs[RAILS_ENV]["host"] if abcs[RAILS_ENV]["host"] ENV['PGPORT'] = abcs[RAILS_ENV]["port"].to_s if abcs[RAILS_ENV]["port"] @@ -252,13 +252,13 @@ namespace :db do when "firebird" set_firebird_env(abcs[RAILS_ENV]) db_string = firebird_db_string(abcs[RAILS_ENV]) - sh "isql -a #{db_string} > db/#{RAILS_ENV}_structure.sql" + sh "isql -a #{db_string} > #{RAILS_ROOT}/db/#{RAILS_ENV}_structure.sql" else raise "Task not supported by '#{abcs["test"]["adapter"]}'" end if ActiveRecord::Base.connection.supports_migrations? - File.open("db/#{RAILS_ENV}_structure.sql", "a") { |f| f << ActiveRecord::Base.connection.dump_schema_information } + File.open("#{RAILS_ROOT}/db/#{RAILS_ENV}_structure.sql", "a") { |f| f << ActiveRecord::Base.connection.dump_schema_information } end end end @@ -281,28 +281,28 @@ namespace :db do when "mysql" ActiveRecord::Base.establish_connection(:test) ActiveRecord::Base.connection.execute('SET foreign_key_checks = 0') - IO.readlines("db/#{RAILS_ENV}_structure.sql").join.split("\n\n").each do |table| + IO.readlines("#{RAILS_ROOT}/db/#{RAILS_ENV}_structure.sql").join.split("\n\n").each do |table| ActiveRecord::Base.connection.execute(table) end when "postgresql" ENV['PGHOST'] = abcs["test"]["host"] if abcs["test"]["host"] ENV['PGPORT'] = abcs["test"]["port"].to_s if abcs["test"]["port"] ENV['PGPASSWORD'] = abcs["test"]["password"].to_s if abcs["test"]["password"] - `psql -U "#{abcs["test"]["username"]}" -f db/#{RAILS_ENV}_structure.sql #{abcs["test"]["database"]}` + `psql -U "#{abcs["test"]["username"]}" -f #{RAILS_ROOT}/db/#{RAILS_ENV}_structure.sql #{abcs["test"]["database"]}` when "sqlite", "sqlite3" dbfile = abcs["test"]["database"] || abcs["test"]["dbfile"] - `#{abcs["test"]["adapter"]} #{dbfile} < db/#{RAILS_ENV}_structure.sql` + `#{abcs["test"]["adapter"]} #{dbfile} < #{RAILS_ROOT}/db/#{RAILS_ENV}_structure.sql` when "sqlserver" `osql -E -S #{abcs["test"]["host"]} -d #{abcs["test"]["database"]} -i db\\#{RAILS_ENV}_structure.sql` when "oci", "oracle" ActiveRecord::Base.establish_connection(:test) - IO.readlines("db/#{RAILS_ENV}_structure.sql").join.split(";\n\n").each do |ddl| + IO.readlines("#{RAILS_ROOT}/db/#{RAILS_ENV}_structure.sql").join.split(";\n\n").each do |ddl| ActiveRecord::Base.connection.execute(ddl) end when "firebird" set_firebird_env(abcs["test"]) db_string = firebird_db_string(abcs["test"]) - sh "isql -i db/#{RAILS_ENV}_structure.sql #{db_string}" + sh "isql -i #{RAILS_ROOT}/db/#{RAILS_ENV}_structure.sql #{db_string}" else raise "Task not supported by '#{abcs["test"]["adapter"]}'" end From 95812d5eafc3b63ce5eeb0748a5d0132f5108b64 Mon Sep 17 00:00:00 2001 From: rsl Date: Mon, 14 Jul 2008 00:55:57 +0100 Subject: [PATCH 42/74] Ensure :index works with fields_for select methods. [#518 state:resolved] Signed-off-by: Pratik Naik --- .../helpers/form_options_helper.rb | 8 +- .../test/template/form_options_helper_test.rb | 885 ++++++++++++++++++ 2 files changed, 889 insertions(+), 4 deletions(-) diff --git a/actionpack/lib/action_view/helpers/form_options_helper.rb b/actionpack/lib/action_view/helpers/form_options_helper.rb index ab9e174621..87d49397c6 100644 --- a/actionpack/lib/action_view/helpers/form_options_helper.rb +++ b/actionpack/lib/action_view/helpers/form_options_helper.rb @@ -445,19 +445,19 @@ module ActionView class FormBuilder def select(method, choices, options = {}, html_options = {}) - @template.select(@object_name, method, choices, options.merge(:object => @object), html_options) + @template.select(@object_name, method, choices, objectify_options(options), @default_options.merge(html_options)) end def collection_select(method, collection, value_method, text_method, options = {}, html_options = {}) - @template.collection_select(@object_name, method, collection, value_method, text_method, options.merge(:object => @object), html_options) + @template.collection_select(@object_name, method, collection, value_method, text_method, objectify_options(options), @default_options.merge(html_options)) end def country_select(method, priority_countries = nil, options = {}, html_options = {}) - @template.country_select(@object_name, method, priority_countries, options.merge(:object => @object), html_options) + @template.country_select(@object_name, method, priority_countries, objectify_options(options), @default_options.merge(html_options)) end def time_zone_select(method, priority_zones = nil, options = {}, html_options = {}) - @template.time_zone_select(@object_name, method, priority_zones, options.merge(:object => @object), html_options) + @template.time_zone_select(@object_name, method, priority_zones, objectify_options(options), @default_options.merge(html_options)) end end end diff --git a/actionpack/test/template/form_options_helper_test.rb b/actionpack/test/template/form_options_helper_test.rb index 2496931f4b..9dd43d7b4f 100644 --- a/actionpack/test/template/form_options_helper_test.rb +++ b/actionpack/test/template/form_options_helper_test.rb @@ -231,6 +231,35 @@ uses_mocha "FormOptionsHelperTest" do ) end + def test_select_under_fields_for_with_index + @post = Post.new + @post.category = "" + + fields_for :post, @post, :index => 108 do |f| + concat f.select(:category, %w( abe hest)) + end + + assert_dom_equal( + "", + output_buffer + ) + end + + def test_select_under_fields_for_with_auto_index + @post = Post.new + @post.category = "" + def @post.to_param; 108; end + + fields_for "post[]", @post do |f| + concat f.select(:category, %w( abe hest)) + end + + assert_dom_equal( + "", + output_buffer + ) + end + def test_select_with_blank @post = Post.new @post.category = "" @@ -351,6 +380,47 @@ uses_mocha "FormOptionsHelperTest" do ) end + def test_collection_select_under_fields_for_with_index + @posts = [ + Post.new(" went home", "", "To a little house", "shh!"), + Post.new("Babe went home", "Babe", "To a little house", "shh!"), + Post.new("Cabe went home", "Cabe", "To a little house", "shh!") + ] + + @post = Post.new + @post.author_name = "Babe" + + fields_for :post, @post, :index => 815 do |f| + concat f.collection_select(:author_name, @posts, :author_name, :author_name) + end + + assert_dom_equal( + "", + output_buffer + ) + end + + def test_collection_select_under_fields_for_with_auto_index + @posts = [ + Post.new(" went home", "", "To a little house", "shh!"), + Post.new("Babe went home", "Babe", "To a little house", "shh!"), + Post.new("Cabe went home", "Cabe", "To a little house", "shh!") + ] + + @post = Post.new + @post.author_name = "Babe" + def @post.to_param; 815; end + + fields_for "post[]", @post do |f| + concat f.collection_select(:author_name, @posts, :author_name, :author_name) + end + + assert_dom_equal( + "", + output_buffer + ) + end + def test_collection_select_with_blank_and_style @posts = [ Post.new(" went home", "", "To a little house", "shh!"), @@ -1165,6 +1235,782 @@ uses_mocha "FormOptionsHelperTest" do assert_dom_equal(expected_select[0..-2], country_select("post", "origin", ["New Zealand", "Nicaragua"])) end + def test_country_select_under_fields_for + @post = Post.new + @post.origin = "Australia" + expected_select = <<-COUNTRIES + + COUNTRIES + + fields_for :post, @post do |f| + concat f.country_select("origin") + end + + assert_dom_equal(expected_select[0..-2], output_buffer) + end + + def test_country_select_under_fields_for_with_index + @post = Post.new + @post.origin = "United States" + expected_select = <<-COUNTRIES + + COUNTRIES + + fields_for :post, @post, :index => 325 do |f| + concat f.country_select("origin") + end + + assert_dom_equal(expected_select[0..-2], output_buffer) + end + + def test_country_select_under_fields_for_with_auto_index + @post = Post.new + @post.origin = "Iraq" + def @post.to_param; 325; end + + expected_select = <<-COUNTRIES + + COUNTRIES + + fields_for "post[]", @post do |f| + concat f.country_select("origin") + end + + assert_dom_equal(expected_select[0..-2], output_buffer) + end + def test_time_zone_select @firm = Firm.new("D") html = time_zone_select( "firm", "time_zone" ) @@ -1197,6 +2043,45 @@ uses_mocha "FormOptionsHelperTest" do ) end + def test_time_zone_select_under_fields_for_with_index + @firm = Firm.new("D") + + fields_for :firm, @firm, :index => 305 do |f| + concat f.time_zone_select(:time_zone) + end + + assert_dom_equal( + "", + output_buffer + ) + end + + def test_time_zone_select_under_fields_for_with_auto_index + @firm = Firm.new("D") + def @firm.to_param; 305; end + + fields_for "firm[]", @firm do |f| + concat f.time_zone_select(:time_zone) + end + + assert_dom_equal( + "", + output_buffer + ) + end + def test_time_zone_select_with_blank @firm = Firm.new("D") html = time_zone_select("firm", "time_zone", nil, :include_blank => true) From 9783e66cade4d145389cca18fab822f44d03161a Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Mon, 14 Jul 2008 01:02:07 +0100 Subject: [PATCH 43/74] Slightly faster DateTime#to_json. [#598 state:resolved] [Alex Zepeda] --- activesupport/lib/active_support/json/encoders/date_time.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activesupport/lib/active_support/json/encoders/date_time.rb b/activesupport/lib/active_support/json/encoders/date_time.rb index d41c3e9786..a4a5efbfb1 100644 --- a/activesupport/lib/active_support/json/encoders/date_time.rb +++ b/activesupport/lib/active_support/json/encoders/date_time.rb @@ -8,7 +8,7 @@ class DateTime if ActiveSupport.use_standard_json_time_format xmlschema.inspect else - %("#{strftime("%Y/%m/%d %H:%M:%S %z")}") + strftime('"%Y/%m/%d %H:%M:%S %z"') end end end From 697ee1a50dea7580a7240535d3ad89d2d090721a Mon Sep 17 00:00:00 2001 From: Jacek Becela Date: Wed, 9 Jul 2008 21:34:04 +0200 Subject: [PATCH 44/74] Enable loading fixtures from arbitrary locations. [#586 state:resolved] Signed-off-by: Pratik Naik --- railties/lib/tasks/databases.rake | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/railties/lib/tasks/databases.rake b/railties/lib/tasks/databases.rake index 22b8459ce4..87934c295e 100644 --- a/railties/lib/tasks/databases.rake +++ b/railties/lib/tasks/databases.rake @@ -179,12 +179,15 @@ namespace :db do end namespace :fixtures do - desc "Load fixtures into the current environment's database. Load specific fixtures using FIXTURES=x,y" + desc "Load fixtures into the current environment's database. Load specific fixtures using FIXTURES=x,y. Load from subdirectory in test/fixtures using FIXTURES_DIR=z." task :load => :environment do require 'active_record/fixtures' - ActiveRecord::Base.establish_connection(RAILS_ENV.to_sym) - (ENV['FIXTURES'] ? ENV['FIXTURES'].split(/,/) : Dir.glob(File.join(RAILS_ROOT, 'test', 'fixtures', '*.{yml,csv}'))).each do |fixture_file| - Fixtures.create_fixtures('test/fixtures', File.basename(fixture_file, '.*')) + ActiveRecord::Base.establish_connection(Rails.env) + base_dir = File.join(Rails.root, 'test', 'fixtures') + fixtures_dir = ENV['FIXTURES_DIR'] ? File.join(base_dir, ENV['FIXTURES_DIR']) : base_dir + + (ENV['FIXTURES'] ? ENV['FIXTURES'].split(/,/).map {|f| File.join(fixtures_dir, f) } : Dir.glob(File.join(fixtures_dir, '*.{yml,csv}'))).each do |fixture_file| + Fixtures.create_fixtures(File.dirname(fixture_file), File.basename(fixture_file, '.*')) end end From d72c66532f959846cdc2d7fb1dc1ef6ba87bdcb1 Mon Sep 17 00:00:00 2001 From: Rhett Sutphin Date: Mon, 14 Jul 2008 02:01:52 +0100 Subject: [PATCH 45/74] Make fixture accessors work when fixture name is not same as the table name. [#124 state:resolved] Signed-off-by: Pratik Naik --- activerecord/lib/active_record/fixtures.rb | 7 +++--- activerecord/test/cases/fixtures_test.rb | 29 ++++++++++++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/activerecord/lib/active_record/fixtures.rb b/activerecord/lib/active_record/fixtures.rb index e19614e31f..17fb9355c4 100755 --- a/activerecord/lib/active_record/fixtures.rb +++ b/activerecord/lib/active_record/fixtures.rb @@ -541,10 +541,11 @@ class Fixtures < (RUBY_VERSION < '1.9' ? YAML::Omap : Hash) label.to_s.hash.abs end - attr_reader :table_name + attr_reader :table_name, :name def initialize(connection, table_name, class_name, fixture_path, file_filter = DEFAULT_FILTER_RE) @connection, @table_name, @fixture_path, @file_filter = connection, table_name, fixture_path, file_filter + @name = table_name # preserve fixture base name @class_name = class_name || (ActiveRecord::Base.pluralize_table_names ? @table_name.singularize.camelize : @table_name.camelize) @table_name = "#{ActiveRecord::Base.table_name_prefix}#{@table_name}#{ActiveRecord::Base.table_name_suffix}" @@ -963,9 +964,9 @@ module Test #:nodoc: fixtures = Fixtures.create_fixtures(fixture_path, fixture_table_names, fixture_class_names) unless fixtures.nil? if fixtures.instance_of?(Fixtures) - @loaded_fixtures[fixtures.table_name] = fixtures + @loaded_fixtures[fixtures.name] = fixtures else - fixtures.each { |f| @loaded_fixtures[f.table_name] = f } + fixtures.each { |f| @loaded_fixtures[f.name] = f } end end end diff --git a/activerecord/test/cases/fixtures_test.rb b/activerecord/test/cases/fixtures_test.rb index aca7cfb367..0ea24868f1 100755 --- a/activerecord/test/cases/fixtures_test.rb +++ b/activerecord/test/cases/fixtures_test.rb @@ -15,6 +15,7 @@ require 'models/pirate' require 'models/treasure' require 'models/matey' require 'models/ship' +require 'models/book' class FixturesTest < ActiveRecord::TestCase self.use_instantiated_fixtures = true @@ -373,6 +374,34 @@ class CheckSetTableNameFixturesTest < ActiveRecord::TestCase end end +class FixtureNameIsNotTableNameFixturesTest < ActiveRecord::TestCase + set_fixture_class :items => Book + fixtures :items + # Set to false to blow away fixtures cache and ensure our fixtures are loaded + # and thus takes into account our set_fixture_class + self.use_transactional_fixtures = false + + def test_named_accessor + assert_kind_of Book, items(:dvd) + end +end + +class FixtureNameIsNotTableNameMultipleFixturesTest < ActiveRecord::TestCase + set_fixture_class :items => Book, :funny_jokes => Joke + fixtures :items, :funny_jokes + # Set to false to blow away fixtures cache and ensure our fixtures are loaded + # and thus takes into account our set_fixture_class + self.use_transactional_fixtures = false + + def test_named_accessor_of_differently_named_fixture + assert_kind_of Book, items(:dvd) + end + + def test_named_accessor_of_same_named_fixture + assert_kind_of Joke, funny_jokes(:a_joke) + end +end + class CustomConnectionFixturesTest < ActiveRecord::TestCase set_fixture_class :courses => Course fixtures :courses From c6f397c5cecf183680c191dd2128c0a96c5b9399 Mon Sep 17 00:00:00 2001 From: Jason Dew Date: Fri, 27 Jun 2008 12:25:26 -0400 Subject: [PATCH 46/74] Add block syntax to HasManyAssociation#build. [#502 state:resolve] Signed-off-by: Pratik Naik --- .../associations/association_collection.rb | 9 ++++-- .../has_many_associations_test.rb | 31 +++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index eb39714909..04be59e89d 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -78,11 +78,14 @@ module ActiveRecord @loaded = false end - def build(attributes = {}) + def build(attributes = {}, &block) if attributes.is_a?(Array) - attributes.collect { |attr| build(attr) } + attributes.collect { |attr| build(attr, &block) } else - build_record(attributes) { |record| set_belongs_to_association_for(record) } + build_record(attributes) do |record| + block.call(record) if block_given? + set_belongs_to_association_for(record) + end end end diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index e90edbb213..b9c7ec6377 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -425,6 +425,37 @@ class HasManyAssociationsTest < ActiveRecord::TestCase assert_equal 2, first_topic.replies.to_ary.size end + def test_build_via_block + company = companies(:first_firm) + new_client = assert_no_queries { company.clients_of_firm.build {|client| client.name = "Another Client" } } + assert !company.clients_of_firm.loaded? + + assert_equal "Another Client", new_client.name + assert new_client.new_record? + assert_equal new_client, company.clients_of_firm.last + company.name += '-changed' + assert_queries(2) { assert company.save } + assert !new_client.new_record? + assert_equal 2, company.clients_of_firm(true).size + end + + def test_build_many_via_block + company = companies(:first_firm) + new_clients = assert_no_queries do + company.clients_of_firm.build([{"name" => "Another Client"}, {"name" => "Another Client II"}]) do |client| + client.name = "changed" + end + end + + assert_equal 2, new_clients.size + assert_equal "changed", new_clients.first.name + assert_equal "changed", new_clients.last.name + + company.name += '-changed' + assert_queries(3) { assert company.save } + assert_equal 3, company.clients_of_firm(true).size + end + def test_create_without_loading_association first_firm = companies(:first_firm) Firm.column_names From e0750d6a5c7f621e4ca12205137c0b135cab444a Mon Sep 17 00:00:00 2001 From: David Dollar Date: Sun, 13 Jul 2008 21:13:50 -0400 Subject: [PATCH 47/74] Add :accessible option to Associations for allowing mass assignments using hash. [#474 state:resolved] Allows nested Hashes (i.e. from nested forms) to hydrate the appropriate ActiveRecord models. class Post < ActiveRecord::Base belongs_to :author, :accessible => true has_many :comments, :accessible => true end post = Post.create({ :title => 'Accessible Attributes', :author => { :name => 'David Dollar' }, :comments => [ { :body => 'First Post!' }, { :body => 'Nested Hashes are great!' } ] }) post.comments << { :body => 'Another Comment' } Signed-off-by: Pratik Naik --- activerecord/CHANGELOG | 18 +++ .../lib/active_record/associations.rb | 14 ++- .../associations/association_collection.rb | 6 + activerecord/test/cases/associations_test.rb | 108 ++++++++++++++++++ activerecord/test/models/author.rb | 2 +- activerecord/test/models/post.rb | 6 + 6 files changed, 149 insertions(+), 5 deletions(-) diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index d92b89cfe0..56b1781537 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -1,5 +1,23 @@ *Edge* +* Add :accessible option to associations for allowing (opt-in) mass assignment. #474. [David Dollar] Example : + + class Post < ActiveRecord::Base + belongs_to :author, :accessible => true + has_many :comments, :accessible => true + end + + post = Post.create({ + :title => 'Accessible Attributes', + :author => { :name => 'David Dollar' }, + :comments => [ + { :body => 'First Post!' }, + { :body => 'Nested Hashes are great!' } + ] + }) + + post.comments << { :body => 'Another Comment' } + * Add :tokenizer option to validates_length_of to specify how to split up the attribute string. #507. [David Lowenfels] Example : # Ensure essay contains at least 100 words. diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 6931744058..d43e07ab4e 100755 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -692,6 +692,7 @@ module ActiveRecord # * :uniq - If true, duplicates will be omitted from the collection. Useful in conjunction with :through. # * :readonly - If true, all the associated objects are readonly through the association. # * :validate - If false, don't validate the associated objects when saving the parent object. true by default. + # * :accessible - Mass assignment is allowed for this assocation (similar to ActiveRecord::Base#attr_accessible). # # Option examples: # has_many :comments, :order => "posted_on" @@ -774,6 +775,7 @@ module ActiveRecord # association is a polymorphic +belongs_to+. # * :readonly - If true, the associated object is readonly through the association. # * :validate - If false, don't validate the associated object when saving the parent object. +false+ by default. + # * :accessible - Mass assignment is allowed for this assocation (similar to ActiveRecord::Base#attr_accessible). # # Option examples: # has_one :credit_card, :dependent => :destroy # destroys the associated credit card @@ -863,6 +865,7 @@ module ActiveRecord # to the +attr_readonly+ list in the associated classes (e.g. class Post; attr_readonly :comments_count; end). # * :readonly - If true, the associated object is readonly through the association. # * :validate - If false, don't validate the associated objects when saving the parent object. +false+ by default. + # * :accessible - Mass assignment is allowed for this assocation (similar to ActiveRecord::Base#attr_accessible). # # Option examples: # belongs_to :firm, :foreign_key => "client_of" @@ -1034,6 +1037,7 @@ module ActiveRecord # but not include the joined columns. Do not forget to include the primary and foreign keys, otherwise it will raise an error. # * :readonly - If true, all the associated objects are readonly through the association. # * :validate - If false, don't validate the associated objects when saving the parent object. +true+ by default. + # * :accessible - Mass assignment is allowed for this assocation (similar to ActiveRecord::Base#attr_accessible). # # Option examples: # has_and_belongs_to_many :projects @@ -1109,6 +1113,8 @@ module ActiveRecord association = association_proxy_class.new(self, reflection) end + new_value = reflection.klass.new(new_value) if reflection.options[:accessible] && new_value.is_a?(Hash) + if association_proxy_class == HasOneThroughAssociation association.create_through_record(new_value) self.send(reflection.name, new_value) @@ -1357,7 +1363,7 @@ module ActiveRecord :finder_sql, :counter_sql, :before_add, :after_add, :before_remove, :after_remove, :extend, :readonly, - :validate + :validate, :accessible ) options[:extend] = create_extension_modules(association_id, extension, options[:extend]) @@ -1367,7 +1373,7 @@ module ActiveRecord def create_has_one_reflection(association_id, options) options.assert_valid_keys( - :class_name, :foreign_key, :remote, :select, :conditions, :order, :include, :dependent, :counter_cache, :extend, :as, :readonly, :validate, :primary_key + :class_name, :foreign_key, :remote, :select, :conditions, :order, :include, :dependent, :counter_cache, :extend, :as, :readonly, :validate, :primary_key, :accessible ) create_reflection(:has_one, association_id, options, self) @@ -1383,7 +1389,7 @@ module ActiveRecord def create_belongs_to_reflection(association_id, options) options.assert_valid_keys( :class_name, :foreign_key, :foreign_type, :remote, :select, :conditions, :include, :dependent, - :counter_cache, :extend, :polymorphic, :readonly, :validate + :counter_cache, :extend, :polymorphic, :readonly, :validate, :accessible ) reflection = create_reflection(:belongs_to, association_id, options, self) @@ -1403,7 +1409,7 @@ module ActiveRecord :finder_sql, :delete_sql, :insert_sql, :before_add, :after_add, :before_remove, :after_remove, :extend, :readonly, - :validate + :validate, :accessible ) options[:extend] = create_extension_modules(association_id, extension, options[:extend]) diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index 04be59e89d..a28be9eed1 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -97,6 +97,8 @@ module ActiveRecord @owner.transaction do flatten_deeper(records).each do |record| + record = @reflection.klass.new(record) if @reflection.options[:accessible] && record.is_a?(Hash) + raise_on_type_mismatch(record) add_record_to_target_with_callbacks(record) do |r| result &&= insert_record(record) unless @owner.new_record? @@ -229,6 +231,10 @@ module ActiveRecord # Replace this collection with +other_array+ # This will perform a diff and delete/add only records that have changed. def replace(other_array) + other_array.map! do |val| + val.is_a?(Hash) ? @reflection.klass.new(val) : val + end if @reflection.options[:accessible] + other_array.each { |val| raise_on_type_mismatch(val) } load_target diff --git a/activerecord/test/cases/associations_test.rb b/activerecord/test/cases/associations_test.rb index 59349dd7cf..4904feeb7d 100755 --- a/activerecord/test/cases/associations_test.rb +++ b/activerecord/test/cases/associations_test.rb @@ -189,6 +189,114 @@ class AssociationProxyTest < ActiveRecord::TestCase end end + def test_belongs_to_mass_assignment + post_attributes = { :title => 'Associations', :body => 'Are They Accessible?' } + author_attributes = { :name => 'David Dollar' } + + assert_no_difference 'Author.count' do + assert_raise(ActiveRecord::AssociationTypeMismatch) do + Post.create(post_attributes.merge({:author => author_attributes})) + end + end + + assert_difference 'Author.count' do + post = Post.create(post_attributes.merge({:creatable_author => author_attributes})) + assert_equal post.creatable_author.name, author_attributes[:name] + end + end + + def test_has_one_mass_assignment + post_attributes = { :title => 'Associations', :body => 'Are They Accessible?' } + comment_attributes = { :body => 'Setter Takes Hash' } + + assert_no_difference 'Comment.count' do + assert_raise(ActiveRecord::AssociationTypeMismatch) do + Post.create(post_attributes.merge({:uncreatable_comment => comment_attributes})) + end + end + + assert_difference 'Comment.count' do + post = Post.create(post_attributes.merge({:creatable_comment => comment_attributes})) + assert_equal post.creatable_comment.body, comment_attributes[:body] + end + end + + def test_has_many_mass_assignment + post = posts(:welcome) + post_attributes = { :title => 'Associations', :body => 'Are They Accessible?' } + comment_attributes = { :body => 'Setter Takes Hash' } + + assert_no_difference 'Comment.count' do + assert_raise(ActiveRecord::AssociationTypeMismatch) do + Post.create(post_attributes.merge({:comments => [comment_attributes]})) + end + assert_raise(ActiveRecord::AssociationTypeMismatch) do + post.comments << comment_attributes + end + end + + assert_difference 'Comment.count' do + post = Post.create(post_attributes.merge({:creatable_comments => [comment_attributes]})) + assert_equal post.creatable_comments.last.body, comment_attributes[:body] + end + + assert_difference 'Comment.count' do + post.creatable_comments << comment_attributes + assert_equal post.comments.last.body, comment_attributes[:body] + end + + post.creatable_comments = [comment_attributes, comment_attributes] + assert_equal post.creatable_comments.count, 2 + end + + def test_has_and_belongs_to_many_mass_assignment + post = posts(:welcome) + post_attributes = { :title => 'Associations', :body => 'Are They Accessible?' } + category_attributes = { :name => 'Accessible Association', :type => 'Category' } + + assert_no_difference 'Category.count' do + assert_raise(ActiveRecord::AssociationTypeMismatch) do + Post.create(post_attributes.merge({:categories => [category_attributes]})) + end + assert_raise(ActiveRecord::AssociationTypeMismatch) do + post.categories << category_attributes + end + end + + assert_difference 'Category.count' do + post = Post.create(post_attributes.merge({:creatable_categories => [category_attributes]})) + assert_equal post.creatable_categories.last.name, category_attributes[:name] + end + + assert_difference 'Category.count' do + post.creatable_categories << category_attributes + assert_equal post.creatable_categories.last.name, category_attributes[:name] + end + + post.creatable_categories = [category_attributes, category_attributes] + assert_equal post.creatable_categories.count, 2 + end + + def test_association_proxy_setter_can_take_hash + special_comment_attributes = { :body => 'Setter Takes Hash' } + + post = posts(:welcome) + post.creatable_comment = { :body => 'Setter Takes Hash' } + + assert_equal post.creatable_comment.body, special_comment_attributes[:body] + end + + def test_association_collection_can_take_hash + post_attributes = { :title => 'Setter Takes', :body => 'Hash' } + david = authors(:david) + + post = (david.posts << post_attributes).last + assert_equal post.title, post_attributes[:title] + + david.posts = [post_attributes, post_attributes] + assert_equal david.posts.count, 2 + end + def setup_dangling_association josh = Author.create(:name => "Josh") p = Post.create(:title => "New on Edge", :body => "More cool stuff!", :author => josh) diff --git a/activerecord/test/models/author.rb b/activerecord/test/models/author.rb index 82e069005a..136dc39cf3 100644 --- a/activerecord/test/models/author.rb +++ b/activerecord/test/models/author.rb @@ -1,5 +1,5 @@ class Author < ActiveRecord::Base - has_many :posts + has_many :posts, :accessible => true has_many :posts_with_comments, :include => :comments, :class_name => "Post" has_many :posts_with_comments_sorted_by_comment_id, :include => :comments, :class_name => "Post", :order => 'comments.id' has_many :posts_with_categories, :include => :categories, :class_name => "Post" diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb index 3adbc0ce1f..e23818eb33 100644 --- a/activerecord/test/models/post.rb +++ b/activerecord/test/models/post.rb @@ -33,6 +33,12 @@ class Post < ActiveRecord::Base has_and_belongs_to_many :categories has_and_belongs_to_many :special_categories, :join_table => "categories_posts", :association_foreign_key => 'category_id' + belongs_to :creatable_author, :class_name => 'Author', :accessible => true + has_one :uncreatable_comment, :class_name => 'Comment', :accessible => false, :order => 'id desc' + has_one :creatable_comment, :class_name => 'Comment', :accessible => true, :order => 'id desc' + has_many :creatable_comments, :class_name => 'Comment', :accessible => true, :dependent => :destroy + has_and_belongs_to_many :creatable_categories, :class_name => 'Category', :accessible => true + has_many :taggings, :as => :taggable has_many :tags, :through => :taggings do def add_joins_and_select From 5c086070824bf7dd2bc4c9ce97956d82ac3fa206 Mon Sep 17 00:00:00 2001 From: Tim Pope Date: Sun, 13 Jul 2008 22:24:16 -0400 Subject: [PATCH 48/74] Make script/plugin install -r option work with git based plugins. [#257 state:resolved] Signed-off-by: Pratik Naik --- railties/CHANGELOG | 5 ++++ railties/lib/commands/plugin.rb | 47 ++++++++++++++++++++++----------- 2 files changed, 37 insertions(+), 15 deletions(-) diff --git a/railties/CHANGELOG b/railties/CHANGELOG index 1b9bdb57f2..b5c5aba460 100644 --- a/railties/CHANGELOG +++ b/railties/CHANGELOG @@ -1,5 +1,10 @@ *Edge* +* Make script/plugin install -r option work with git based plugins. #257. [Tim Pope Jakub Kuźma]. Example: + + script/plugin install git://github.com/mislav/will_paginate.git -r agnostic # Installs 'agnostic' branch + script/plugin install git://github.com/dchelimsky/rspec.git -r 'tag 1.1.4' + * Added Rails.initialized? flag [Josh Peek] * Make rake test:uncommitted work with Git. [Tim Pope] diff --git a/railties/lib/commands/plugin.rb b/railties/lib/commands/plugin.rb index ce4b0d051d..0256090d16 100644 --- a/railties/lib/commands/plugin.rb +++ b/railties/lib/commands/plugin.rb @@ -43,6 +43,16 @@ # plugin is pulled via `svn checkout` or `svn export` but looks # exactly the same. # +# Specifying revisions: +# +# * Subversion revision is a single integer. +# +# * Git revision format: +# - full - 'refs/tags/1.8.0' or 'refs/heads/experimental' +# - short: 'experimental' (equivalent to 'refs/heads/experimental') +# 'tag 1.8.0' (equivalent to 'refs/tags/1.8.0') +# +# # This is Free Software, copyright 2005 by Ryan Tomayko (rtomayko@gmail.com) # and is licensed MIT: (http://www.opensource.org/licenses/mit-license.php) @@ -175,7 +185,7 @@ class Plugin method ||= rails_env.best_install_method? if :http == method method = :export if svn_url? - method = :clone if git_url? + method = :git if git_url? end uninstall if installed? and options[:force] @@ -255,8 +265,25 @@ class Plugin end end - def install_using_clone(options = {}) - git_command :clone, options + def install_using_git(options = {}) + root = rails_env.root + install_path = mkdir_p "#{root}/vendor/plugins/#{name}" + Dir.chdir install_path do + init_cmd = "git init" + init_cmd += " -q" if options[:quiet] and not $verbose + puts init_cmd if $verbose + system(init_cmd) + base_cmd = "git pull --depth 1 #{uri}" + base_cmd += " -q" if options[:quiet] and not $verbose + base_cmd += " #{options[:revision]}" if options[:revision] + puts base_cmd if $verbose + if system(base_cmd) + puts "removing: .git" if $verbose + rm_rf ".git" + else + rm_rf install_path + end + end end def svn_command(cmd, options = {}) @@ -268,16 +295,6 @@ class Plugin puts base_cmd if $verbose system(base_cmd) end - - def git_command(cmd, options = {}) - root = rails_env.root - mkdir_p "#{root}/vendor/plugins" - base_cmd = "git #{cmd} --depth 1 #{uri} \"#{root}/vendor/plugins/#{name}\"" - puts base_cmd if $verbose - puts "removing: #{root}/vendor/plugins/#{name}/.git" - system(base_cmd) - rm_rf "#{root}/vendor/plugins/#{name}/.git" - end def guess_name(url) @name = File.basename(url) @@ -756,8 +773,8 @@ module Commands "Suppresses the output from installation.", "Ignored if -v is passed (./script/plugin -v install ...)") { |v| @options[:quiet] = true } o.on( "-r REVISION", "--revision REVISION", - "Checks out the given revision from subversion.", - "Ignored if subversion is not used.") { |v| @options[:revision] = v } + "Checks out the given revision from subversion or git.", + "Ignored if subversion/git is not used.") { |v| @options[:revision] = v } o.on( "-f", "--force", "Reinstalls a plugin if it's already installed.") { |v| @options[:force] = true } o.separator "" From 0176e6adb388998414083e99523de318d3b8ca49 Mon Sep 17 00:00:00 2001 From: "Sebastian A. Espindola" Date: Tue, 8 Jul 2008 01:14:11 -0300 Subject: [PATCH 49/74] Added db:charset support to PostgreSQL. [#556 state:resolved] Signed-off-by: Pratik Naik --- .../connection_adapters/postgresql_adapter.rb | 13 +++++++++++++ activerecord/test/cases/adapter_test.rb | 6 ++++++ cleanlogs.sh | 1 - railties/lib/tasks/databases.rake | 3 +++ 4 files changed, 22 insertions(+), 1 deletion(-) delete mode 100755 cleanlogs.sh diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index 2e2d50ccf4..29ecd83ba0 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -623,6 +623,19 @@ module ActiveRecord end end + # Returns the current database name. + def current_database + query('select current_database()')[0][0] + end + + # Returns the current database encoding format. + def encoding + query(<<-end_sql)[0][0] + SELECT pg_encoding_to_char(pg_database.encoding) FROM pg_database + WHERE pg_database.datname LIKE '#{current_database}' + end_sql + end + # Sets the schema search path to a string of comma-separated schema names. # Names beginning with $ have to be quoted (e.g. $user => '$user'). # See: http://www.postgresql.org/docs/current/static/ddl-schemas.html diff --git a/activerecord/test/cases/adapter_test.rb b/activerecord/test/cases/adapter_test.rb index 11f9870534..04770646b2 100644 --- a/activerecord/test/cases/adapter_test.rb +++ b/activerecord/test/cases/adapter_test.rb @@ -65,6 +65,12 @@ class AdapterTest < ActiveRecord::TestCase end end + if current_adapter?(:PostgreSQLAdapter) + def test_encoding + assert_not_nil @connection.encoding + end + end + def test_table_alias def @connection.test_table_alias_length() 10; end class << @connection diff --git a/cleanlogs.sh b/cleanlogs.sh deleted file mode 100755 index a4e6baf0df..0000000000 --- a/cleanlogs.sh +++ /dev/null @@ -1 +0,0 @@ -rm activerecord/debug.log activerecord/test/debug.log actionpack/debug.log activeresource/test/debug.log diff --git a/railties/lib/tasks/databases.rake b/railties/lib/tasks/databases.rake index 87934c295e..5ec712a02d 100644 --- a/railties/lib/tasks/databases.rake +++ b/railties/lib/tasks/databases.rake @@ -141,6 +141,9 @@ namespace :db do when 'mysql' ActiveRecord::Base.establish_connection(config) puts ActiveRecord::Base.connection.charset + when 'postgresql' + ActiveRecord::Base.establish_connection(config) + puts ActiveRecord::Base.connection.encoding else puts 'sorry, your database adapter is not supported yet, feel free to submit a patch' end From 30370227890dc950f1544b7b1040aa75e505f877 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Mon, 14 Jul 2008 10:12:54 -0500 Subject: [PATCH 50/74] Fixed that create database statements would always include "DEFAULT NULL" (Nick Sieger) [#334 status:committed] --- activerecord/CHANGELOG | 2 ++ .../abstract/schema_definitions.rb | 5 ++- .../test/cases/column_definition_test.rb | 36 +++++++++++++++++++ 3 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 activerecord/test/cases/column_definition_test.rb diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index 56b1781537..17430ba0b7 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -1,5 +1,7 @@ *Edge* +* Fixed that create database statements would always include "DEFAULT NULL" (Nick Sieger) [#334] + * Add :accessible option to associations for allowing (opt-in) mass assignment. #474. [David Dollar] Example : class Post < ActiveRecord::Base diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb index d4c8a80448..13ccfea0b8 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb @@ -257,7 +257,10 @@ module ActiveRecord def to_sql column_sql = "#{base.quote_column_name(name)} #{sql_type}" - add_column_options!(column_sql, :null => null, :default => default) unless type.to_sym == :primary_key + column_options = {} + column_options[:null] = null unless null.nil? + column_options[:default] = default unless default.nil? + add_column_options!(column_sql, column_options) unless type.to_sym == :primary_key column_sql end alias to_s :to_sql diff --git a/activerecord/test/cases/column_definition_test.rb b/activerecord/test/cases/column_definition_test.rb new file mode 100644 index 0000000000..540f42f4b6 --- /dev/null +++ b/activerecord/test/cases/column_definition_test.rb @@ -0,0 +1,36 @@ +require "cases/helper" + +class ColumnDefinitionTest < ActiveRecord::TestCase + def setup + @adapter = ActiveRecord::ConnectionAdapters::AbstractAdapter.new(nil) + def @adapter.native_database_types + {:string => "varchar"} + end + end + + # Avoid column definitions in create table statements like: + # `title` varchar(255) DEFAULT NULL NULL + def test_should_not_include_default_clause_when_default_is_null + column = ActiveRecord::ConnectionAdapters::Column.new("title", nil, "varchar(20)") + column_def = ActiveRecord::ConnectionAdapters::ColumnDefinition.new( + @adapter, column.name, "string", + column.limit, column.precision, column.scale, column.default, column.null) + assert_equal "title varchar(20) NULL", column_def.to_sql + end + + def test_should_include_default_clause_when_default_is_present + column = ActiveRecord::ConnectionAdapters::Column.new("title", "Hello", "varchar(20)") + column_def = ActiveRecord::ConnectionAdapters::ColumnDefinition.new( + @adapter, column.name, "string", + column.limit, column.precision, column.scale, column.default, column.null) + assert_equal %Q{title varchar(20) DEFAULT 'Hello' NULL}, column_def.to_sql + end + + def test_should_specify_not_null_if_null_option_is_false + column = ActiveRecord::ConnectionAdapters::Column.new("title", "Hello", "varchar(20)", false) + column_def = ActiveRecord::ConnectionAdapters::ColumnDefinition.new( + @adapter, column.name, "string", + column.limit, column.precision, column.scale, column.default, column.null) + assert_equal %Q{title varchar(20) DEFAULT 'Hello' NOT NULL}, column_def.to_sql + end +end \ No newline at end of file From 2167f95d857224b88901c5fb4cda63c2e0756676 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Wed, 9 Jul 2008 12:38:59 -0700 Subject: [PATCH 51/74] Restore the more readable before_ and after_filters methods since they aren't called frequently --- actionpack/lib/action_controller/filters.rb | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/actionpack/lib/action_controller/filters.rb b/actionpack/lib/action_controller/filters.rb index fc63890d13..10dc0cc45b 100644 --- a/actionpack/lib/action_controller/filters.rb +++ b/actionpack/lib/action_controller/filters.rb @@ -569,21 +569,13 @@ module ActionController #:nodoc: # Returns all the before filters for this class and all its ancestors. # This method returns the actual filter that was assigned in the controller to maintain existing functionality. def before_filters #:nodoc: - filters = [] - filter_chain.each do |filter| - filters << filter.method if filter.before? - end - filters + filter_chain.select(&:before?).map(&:method) end # Returns all the after filters for this class and all its ancestors. # This method returns the actual filter that was assigned in the controller to maintain existing functionality. def after_filters #:nodoc: - filters = [] - filter_chain.each do |filter| - filters << filter.method if filter.after? - end - filters + filter_chain.select(&:after?).map(&:method) end end From 425de8db6adf13f21242bdd9adcb931bb836ad8b Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Wed, 9 Jul 2008 13:30:53 -0700 Subject: [PATCH 52/74] Use instance_method(...) to check whether the method exists --- .../template_handlers/compilable.rb | 46 ++++++++++++++++++- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/actionpack/lib/action_view/template_handlers/compilable.rb b/actionpack/lib/action_view/template_handlers/compilable.rb index a0ebaefeef..2a19fc7628 100644 --- a/actionpack/lib/action_view/template_handlers/compilable.rb +++ b/actionpack/lib/action_view/template_handlers/compilable.rb @@ -12,9 +12,51 @@ module ActionView end end - def render(template, local_assigns = {}) - @view.send(:execute, template, local_assigns) + def render(template) + @view.send(:execute, template) end + + # Compile and evaluate the template's code + def compile_template(template) + return false unless recompile_template?(template) + + @@mutex.synchronize do + locals_code = template.locals.keys.map { |key| "#{key} = local_assigns[:#{key}];" }.join + + source = <<-end_src + def #{template.method}(local_assigns) + old_output_buffer = output_buffer;#{locals_code};#{compile(template)} + ensure + self.output_buffer = old_output_buffer + end + end_src + + begin + file_name = template.filename || 'compiled-template' + ActionView::Base::CompiledTemplates.module_eval(source, file_name, 0) + rescue Exception => e # errors from template code + if logger = ActionController::Base.logger + logger.debug "ERROR: compiling #{template.method} RAISED #{e}" + logger.debug "Function body: #{source}" + logger.debug "Backtrace: #{e.backtrace.join("\n")}" + end + + raise ActionView::TemplateError.new(template, @view.assigns, e) + end + end + end + + private + # Method to check whether template compilation is necessary. + # The template will be compiled if the inline template or file has not been compiled yet, + # if local_assigns has a new key, which isn't supported by the compiled code yet. + def recompile_template?(template) + # Unless the template has been compiled yet, compile + # If template caching is disabled, compile + # Always recompile inline templates + meth = Base::CompiledTemplates.instance_method(template.method) rescue nil + !meth || !Base.cache_template_loading || template.is_a?(InlineTemplate) + end end end end From ae9356ae9eff4072efae7433204578c30c6ddbf7 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Wed, 9 Jul 2008 19:41:49 -0700 Subject: [PATCH 53/74] Cache template loading for performance tests --- railties/helpers/performance_test_helper.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/railties/helpers/performance_test_helper.rb b/railties/helpers/performance_test_helper.rb index 3c4c7fb740..f1662fa30a 100644 --- a/railties/helpers/performance_test_helper.rb +++ b/railties/helpers/performance_test_helper.rb @@ -2,5 +2,6 @@ require 'test_helper' require 'action_controller/performance_test' ActionController::Base.perform_caching = true +ActionView::Base.cache_template_loading = true ActiveSupport::Dependencies.mechanism = :require Rails.logger.level = ActiveSupport::BufferedLogger::INFO From 269c6c6bcfec342cf830e0b20c44bb2c888e8fdf Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Wed, 9 Jul 2008 19:43:38 -0700 Subject: [PATCH 54/74] Cache Module#parent_name --- .../core_ext/module/introspection.rb | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/activesupport/lib/active_support/core_ext/module/introspection.rb b/activesupport/lib/active_support/core_ext/module/introspection.rb index bb894ec080..45f3e4bf5c 100644 --- a/activesupport/lib/active_support/core_ext/module/introspection.rb +++ b/activesupport/lib/active_support/core_ext/module/introspection.rb @@ -1,4 +1,14 @@ class Module + # Returns the name of the module containing this one. + # + # p M::N.parent_name # => "M" + def parent_name + unless defined? @parent_name + @parent_name = name =~ /::[^:]+\Z/ ? $`.freeze : nil + end + @parent_name + end + # Returns the module which contains this one according to its name. # # module M @@ -16,8 +26,7 @@ class Module # p Module.new.parent # => Object # def parent - parent_name = name.split('::')[0..-2] * '::' - parent_name.empty? ? Object : parent_name.constantize + parent_name ? parent_name.constantize : Object end # Returns all the parents of this module according to its name, ordered from @@ -35,10 +44,12 @@ class Module # def parents parents = [] - parts = name.split('::')[0..-2] - until parts.empty? - parents << (parts * '::').constantize - parts.pop + if parent_name + parts = parent_name.split('::') + until parts.empty? + parents << (parts * '::').constantize + parts.pop + end end parents << Object unless parents.include? Object parents From cdf0f1aa2ee4ba73bafc9b9217ee35b7f7eb3273 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Sat, 12 Jul 2008 11:42:17 -0700 Subject: [PATCH 55/74] Faster and clearer value_to_boolean --- .../connection_adapters/abstract/schema_definitions.rb | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb index 13ccfea0b8..31d6c7942c 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb @@ -1,4 +1,5 @@ require 'date' +require 'set' require 'bigdecimal' require 'bigdecimal/util' @@ -6,6 +7,8 @@ module ActiveRecord module ConnectionAdapters #:nodoc: # An abstract definition of a column in a table. class Column + TRUE_VALUES = [true, 1, '1', 't', 'T', 'true', 'TRUE'].to_set + module Format ISO_DATE = /\A(\d{4})-(\d\d)-(\d\d)\z/ ISO_DATETIME = /\A(\d{4})-(\d\d)-(\d\d) (\d\d):(\d\d):(\d\d)(\.\d+)?\z/ @@ -135,11 +138,7 @@ module ActiveRecord # convert something to a boolean def value_to_boolean(value) - if value == true || value == false - value - else - !(value.to_s !~ /\A(?:1|t|true)\Z/i) - end + TRUE_VALUES.include?(value) end # convert something to a BigDecimal From 4e323f6ef09fed146a9302d7b7e3f09934de6e37 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Mon, 14 Jul 2008 11:50:07 -0700 Subject: [PATCH 56/74] Fix bad merge --- actionpack/lib/action_view/template_handlers/compilable.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/actionpack/lib/action_view/template_handlers/compilable.rb b/actionpack/lib/action_view/template_handlers/compilable.rb index 2a19fc7628..2b96e7f133 100644 --- a/actionpack/lib/action_view/template_handlers/compilable.rb +++ b/actionpack/lib/action_view/template_handlers/compilable.rb @@ -12,8 +12,8 @@ module ActionView end end - def render(template) - @view.send(:execute, template) + def render(template, local_assigns = {}) + @view.send(:execute, template, local_assigns) end # Compile and evaluate the template's code From c760dbfd3117562c6f27170a213f586e3ba2b794 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Mon, 14 Jul 2008 11:59:46 -0700 Subject: [PATCH 57/74] PostgreSQL: don't dump :limit => 4 for integers --- .../lib/active_record/connection_adapters/postgresql_adapter.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index 29ecd83ba0..6d16d72dea 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -49,7 +49,6 @@ module ActiveRecord private def extract_limit(sql_type) case sql_type - when /^integer/i; 4 when /^bigint/i; 8 when /^smallint/i; 2 else super From 8f72bc92e20b1242272714f253e23b256761ec1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tarmo=20T=C3=A4nav?= Date: Mon, 14 Jul 2008 09:40:05 +0300 Subject: [PATCH 58/74] Fixed test_rename_nonexistent_column for PostgreSQL Also fixed ability to run migration_test.rb alone [#616 state:resolved] --- activerecord/test/cases/migration_test.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index 4482b487dd..f9a2d9c47c 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -3,6 +3,7 @@ require 'bigdecimal/util' require 'models/person' require 'models/topic' +require 'models/developer' require MIGRATIONS_ROOT + "/valid/1_people_have_last_names" require MIGRATIONS_ROOT + "/valid/2_we_need_reminders" @@ -511,7 +512,12 @@ if ActiveRecord::Base.connection.supports_migrations? ActiveRecord::Base.connection.create_table(:hats) do |table| table.column :hat_name, :string, :default => nil end - assert_raises(ActiveRecord::ActiveRecordError) do + exception = if current_adapter?(:PostgreSQLAdapter) + ActiveRecord::StatementInvalid + else + ActiveRecord::ActiveRecordError + end + assert_raises(exception) do Person.connection.rename_column "hats", "nonexistent", "should_fail" end ensure From 07578ac85585d3c64d4d38d4892fd31582c7c473 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tarmo=20T=C3=A4nav?= Date: Mon, 14 Jul 2008 09:42:20 +0300 Subject: [PATCH 59/74] Fixed mysql change_column_default to not make the column always nullable. Also added change_column_null to both mysql and sqlite to keep the api features closer to postgresql. [#617 state:resolved] --- activerecord/CHANGELOG | 2 + .../connection_adapters/mysql_adapter.rb | 33 ++++++++++--- .../connection_adapters/sqlite_adapter.rb | 9 ++++ activerecord/test/cases/migration_test.rb | 49 +++++++++++++++++++ 4 files changed, 86 insertions(+), 7 deletions(-) diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index 17430ba0b7..95fdd93f65 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -1,5 +1,7 @@ *Edge* +* change_column_default preserves the not-null constraint. #617 [Tarmo Tänav] + * Fixed that create database statements would always include "DEFAULT NULL" (Nick Sieger) [#334] * Add :accessible option to associations for allowing (opt-in) mass assignment. #474. [David Dollar] Example : diff --git a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb index 4b13ac8be0..35b9ed4746 100755 --- a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb @@ -437,18 +437,29 @@ module ActiveRecord end def change_column_default(table_name, column_name, default) #:nodoc: - current_type = select_one("SHOW COLUMNS FROM #{quote_table_name(table_name)} LIKE '#{column_name}'")["Type"] + column = column_for(table_name, column_name) + change_column table_name, column_name, column.sql_type, :default => default + end - execute("ALTER TABLE #{quote_table_name(table_name)} CHANGE #{quote_column_name(column_name)} #{quote_column_name(column_name)} #{current_type} DEFAULT #{quote(default)}") + def change_column_null(table_name, column_name, null, default = nil) + column = column_for(table_name, column_name) + + unless null || default.nil? + execute("UPDATE #{quote_table_name(table_name)} SET #{quote_column_name(column_name)}=#{quote(default)} WHERE #{quote_column_name(column_name)} IS NULL") + end + + change_column table_name, column_name, column.sql_type, :null => null end def change_column(table_name, column_name, type, options = {}) #:nodoc: + column = column_for(table_name, column_name) + unless options_include_default?(options) - if column = columns(table_name).find { |c| c.name == column_name.to_s } - options[:default] = column.default - else - raise "No such column: #{table_name}.#{column_name}" - end + options[:default] = column.default + end + + unless options.has_key?(:null) + options[:null] = column.null end change_column_sql = "ALTER TABLE #{quote_table_name(table_name)} CHANGE #{quote_column_name(column_name)} #{quote_column_name(column_name)} #{type_to_sql(type, options[:limit], options[:precision], options[:scale])}" @@ -460,6 +471,7 @@ module ActiveRecord options = {} if column = columns(table_name).find { |c| c.name == column_name.to_s } options[:default] = column.default + options[:null] = column.null else raise ActiveRecordError, "No such column: #{table_name}.#{column_name}" end @@ -536,6 +548,13 @@ module ActiveRecord def version @version ||= @connection.server_info.scan(/^(\d+)\.(\d+)\.(\d+)/).flatten.map { |v| v.to_i } end + + def column_for(table_name, column_name) + unless column = columns(table_name).find { |c| c.name == column_name.to_s } + raise "No such column: #{table_name}.#{column_name}" + end + column + end end end end diff --git a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb index 51cfd10e5c..f4d387cfb4 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb @@ -238,6 +238,15 @@ module ActiveRecord end end + def change_column_null(table_name, column_name, null, default = nil) + unless null || default.nil? + execute("UPDATE #{quote_table_name(table_name)} SET #{quote_column_name(column_name)}=#{quote(default)} WHERE #{quote_column_name(column_name)} IS NULL") + end + alter_table(table_name) do |definition| + definition[column_name].null = null + end + end + def change_column(table_name, column_name, type, options = {}) #:nodoc: alter_table(table_name) do |definition| include_default = options_include_default?(options) diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index f9a2d9c47c..7ecf755ef8 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -703,6 +703,55 @@ if ActiveRecord::Base.connection.supports_migrations? Person.connection.drop_table :testings rescue nil end + def test_keeping_default_and_notnull_constaint_on_change + Person.connection.create_table :testings do |t| + t.column :title, :string + end + person_klass = Class.new(Person) + person_klass.set_table_name 'testings' + + person_klass.connection.add_column "testings", "wealth", :integer, :null => false, :default => 99 + person_klass.reset_column_information + assert_equal 99, person_klass.columns_hash["wealth"].default + assert_equal false, person_klass.columns_hash["wealth"].null + assert_nothing_raised {person_klass.connection.execute("insert into testings (title) values ('tester')")} + + # change column default to see that column doesn't lose its not null definition + person_klass.connection.change_column_default "testings", "wealth", 100 + person_klass.reset_column_information + assert_equal 100, person_klass.columns_hash["wealth"].default + assert_equal false, person_klass.columns_hash["wealth"].null + + # rename column to see that column doesn't lose its not null and/or default definition + person_klass.connection.rename_column "testings", "wealth", "money" + person_klass.reset_column_information + assert_nil person_klass.columns_hash["wealth"] + assert_equal 100, person_klass.columns_hash["money"].default + assert_equal false, person_klass.columns_hash["money"].null + + # change column + person_klass.connection.change_column "testings", "money", :integer, :null => false, :default => 1000 + person_klass.reset_column_information + assert_equal 1000, person_klass.columns_hash["money"].default + assert_equal false, person_klass.columns_hash["money"].null + + # change column, make it nullable and clear default + person_klass.connection.change_column "testings", "money", :integer, :null => true, :default => nil + person_klass.reset_column_information + assert_nil person_klass.columns_hash["money"].default + assert_equal true, person_klass.columns_hash["money"].null + + # change_column_null, make it not nullable and set null values to a default value + person_klass.connection.execute('UPDATE testings SET money = NULL') + person_klass.connection.change_column_null "testings", "money", false, 2000 + person_klass.reset_column_information + assert_nil person_klass.columns_hash["money"].default + assert_equal false, person_klass.columns_hash["money"].null + assert_equal [2000], Person.connection.select_values("SELECT money FROM testings").map { |s| s.to_i }.sort + ensure + Person.connection.drop_table :testings rescue nil + end + def test_change_column_default_to_null Person.connection.change_column_default "people", "first_name", nil Person.reset_column_information From 3fbefecc9cdc0d1a903d6ab76170223166cc69ab Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Mon, 14 Jul 2008 13:47:51 -0700 Subject: [PATCH 60/74] Remove dead code from merge --- .../template_handlers/compilable.rb | 30 ------------------- 1 file changed, 30 deletions(-) diff --git a/actionpack/lib/action_view/template_handlers/compilable.rb b/actionpack/lib/action_view/template_handlers/compilable.rb index 2b96e7f133..7e45761481 100644 --- a/actionpack/lib/action_view/template_handlers/compilable.rb +++ b/actionpack/lib/action_view/template_handlers/compilable.rb @@ -16,36 +16,6 @@ module ActionView @view.send(:execute, template, local_assigns) end - # Compile and evaluate the template's code - def compile_template(template) - return false unless recompile_template?(template) - - @@mutex.synchronize do - locals_code = template.locals.keys.map { |key| "#{key} = local_assigns[:#{key}];" }.join - - source = <<-end_src - def #{template.method}(local_assigns) - old_output_buffer = output_buffer;#{locals_code};#{compile(template)} - ensure - self.output_buffer = old_output_buffer - end - end_src - - begin - file_name = template.filename || 'compiled-template' - ActionView::Base::CompiledTemplates.module_eval(source, file_name, 0) - rescue Exception => e # errors from template code - if logger = ActionController::Base.logger - logger.debug "ERROR: compiling #{template.method} RAISED #{e}" - logger.debug "Function body: #{source}" - logger.debug "Backtrace: #{e.backtrace.join("\n")}" - end - - raise ActionView::TemplateError.new(template, @view.assigns, e) - end - end - end - private # Method to check whether template compilation is necessary. # The template will be compiled if the inline template or file has not been compiled yet, From 2d6562d51b96af518c1eb2947d6d34d5dd5bad12 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Mon, 14 Jul 2008 13:51:59 -0700 Subject: [PATCH 61/74] Move dead recompile_template? also --- actionpack/lib/action_view/renderable.rb | 7 ++----- .../lib/action_view/template_handlers/compilable.rb | 12 ------------ 2 files changed, 2 insertions(+), 17 deletions(-) diff --git a/actionpack/lib/action_view/renderable.rb b/actionpack/lib/action_view/renderable.rb index ebb0f1b674..ffcffd1667 100644 --- a/actionpack/lib/action_view/renderable.rb +++ b/actionpack/lib/action_view/renderable.rb @@ -69,11 +69,8 @@ module ActionView # The template will be compiled if the file has not been compiled yet, or # if local_assigns has a new key, which isn't supported by the compiled code yet. def recompile?(symbol) - unless Base::CompiledTemplates.instance_methods.include?(symbol) && Base.cache_template_loading - true - else - false - end + meth = Base::CompiledTemplates.instance_method(template.method) rescue nil + !(meth && Base.cache_template_loading) end end end diff --git a/actionpack/lib/action_view/template_handlers/compilable.rb b/actionpack/lib/action_view/template_handlers/compilable.rb index 7e45761481..a0ebaefeef 100644 --- a/actionpack/lib/action_view/template_handlers/compilable.rb +++ b/actionpack/lib/action_view/template_handlers/compilable.rb @@ -15,18 +15,6 @@ module ActionView def render(template, local_assigns = {}) @view.send(:execute, template, local_assigns) end - - private - # Method to check whether template compilation is necessary. - # The template will be compiled if the inline template or file has not been compiled yet, - # if local_assigns has a new key, which isn't supported by the compiled code yet. - def recompile_template?(template) - # Unless the template has been compiled yet, compile - # If template caching is disabled, compile - # Always recompile inline templates - meth = Base::CompiledTemplates.instance_method(template.method) rescue nil - !meth || !Base.cache_template_loading || template.is_a?(InlineTemplate) - end end end end From cd9b24286a90111a08002e0da753198c5fb2432a Mon Sep 17 00:00:00 2001 From: Gabe da Silveira Date: Tue, 3 Jun 2008 17:50:42 -0300 Subject: [PATCH 62/74] Add assert_sql helper method to check for specific SQL output in Active Record test suite. [#325 state:resolved] Signed-off-by: Pratik Naik --- activerecord/lib/active_record/test_case.rb | 17 ++++++++++++++--- activerecord/test/cases/helper.rb | 10 +++++----- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/activerecord/lib/active_record/test_case.rb b/activerecord/lib/active_record/test_case.rb index 7dee962c8a..ca5591ae35 100644 --- a/activerecord/lib/active_record/test_case.rb +++ b/activerecord/lib/active_record/test_case.rb @@ -22,11 +22,22 @@ module ActiveRecord end end - def assert_queries(num = 1) - $query_count = 0 + def assert_sql(*patterns_to_match) + $queries_executed = [] yield ensure - assert_equal num, $query_count, "#{$query_count} instead of #{num} queries were executed." + failed_patterns = [] + patterns_to_match.each do |pattern| + failed_patterns << pattern unless $queries_executed.any?{ |sql| pattern === sql } + end + assert failed_patterns.empty?, "Query pattern(s) #{failed_patterns.map(&:inspect).join(', ')} not found." + end + + def assert_queries(num = 1) + $queries_executed = [] + yield + ensure + assert_equal num, $queries_executed.size, "#{$queries_executed.size} instead of #{num} queries were executed." end def assert_no_queries(&block) diff --git a/activerecord/test/cases/helper.rb b/activerecord/test/cases/helper.rb index dc83300efa..0530ba9bd9 100644 --- a/activerecord/test/cases/helper.rb +++ b/activerecord/test/cases/helper.rb @@ -32,13 +32,13 @@ end ActiveRecord::Base.connection.class.class_eval do IGNORED_SQL = [/^PRAGMA/, /^SELECT currval/, /^SELECT CAST/, /^SELECT @@IDENTITY/, /^SELECT @@ROWCOUNT/] - def execute_with_counting(sql, name = nil, &block) - $query_count ||= 0 - $query_count += 1 unless IGNORED_SQL.any? { |r| sql =~ r } - execute_without_counting(sql, name, &block) + def execute_with_query_record(sql, name = nil, &block) + $queries_executed ||= [] + $queries_executed << sql unless IGNORED_SQL.any? { |r| sql =~ r } + execute_without_query_record(sql, name, &block) end - alias_method_chain :execute, :counting + alias_method_chain :execute, :query_record end # Make with_scope public for tests From 76df9fa0680d62ce41fa6f3b743c605101d101d2 Mon Sep 17 00:00:00 2001 From: Tiago Macedo Date: Fri, 11 Jul 2008 04:18:41 +0100 Subject: [PATCH 63/74] Fix integer quoting issues in association preload. [#602 state:resolved] Signed-off-by: Pratik Naik --- .../lib/active_record/association_preload.rb | 15 ++++++++++++--- activerecord/test/cases/inheritance_test.rb | 7 +++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/activerecord/lib/active_record/association_preload.rb b/activerecord/lib/active_record/association_preload.rb index 49f5270396..174ec95de2 100644 --- a/activerecord/lib/active_record/association_preload.rb +++ b/activerecord/lib/active_record/association_preload.rb @@ -188,7 +188,6 @@ module ActiveRecord through_records end - # FIXME: quoting def preload_belongs_to_association(records, reflection, preload_options={}) options = reflection.options primary_key_name = reflection.primary_key_name @@ -227,9 +226,19 @@ module ActiveRecord table_name = klass.quoted_table_name primary_key = klass.primary_key - conditions = "#{table_name}.#{primary_key} IN (?)" + conditions = "#{table_name}.#{connection.quote_column_name(primary_key)} IN (?)" conditions << append_conditions(options, preload_options) - associated_records = klass.find(:all, :conditions => [conditions, id_map.keys.uniq], + column_type = klass.columns.detect{|c| c.name == primary_key}.type + ids = id_map.keys.uniq.map do |id| + if column_type == :integer + id.to_i + elsif column_type == :float + id.to_f + else + id + end + end + associated_records = klass.find(:all, :conditions => [conditions, ids], :include => options[:include], :select => options[:select], :joins => options[:joins], diff --git a/activerecord/test/cases/inheritance_test.rb b/activerecord/test/cases/inheritance_test.rb index 47fb5c608f..4fd38bfbc9 100755 --- a/activerecord/test/cases/inheritance_test.rb +++ b/activerecord/test/cases/inheritance_test.rb @@ -191,6 +191,13 @@ class InheritanceTest < ActiveRecord::TestCase assert_not_nil account.instance_variable_get("@firm"), "nil proves eager load failed" end + def test_eager_load_belongs_to_primary_key_quoting + con = Account.connection + assert_sql(/\(#{con.quote_table_name('companies')}.#{con.quote_column_name('id')} IN \(1\)\)/) do + Account.find(1, :include => :firm) + end + end + def test_alt_eager_loading switch_to_alt_inheritance_column test_eager_load_belongs_to_something_inherited From 9dc258d6147c8dab772d1f041098a38389cd3e73 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Mon, 14 Jul 2008 17:40:03 -0500 Subject: [PATCH 64/74] Eager load Partial variable_name and counter_name. Tidy up render_partial_collection. --- actionpack/lib/action_view/partials.rb | 33 +++++-------------- actionpack/lib/action_view/renderable.rb | 9 +++-- .../lib/action_view/renderable_partial.rb | 28 +++++++++++++--- actionpack/lib/action_view/template.rb | 24 +++++--------- 4 files changed, 49 insertions(+), 45 deletions(-) diff --git a/actionpack/lib/action_view/partials.rb b/actionpack/lib/action_view/partials.rb index 116d61e13b..5aa4c83009 100644 --- a/actionpack/lib/action_view/partials.rb +++ b/actionpack/lib/action_view/partials.rb @@ -108,8 +108,7 @@ module ActionView case partial_path when String, Symbol, NilClass - variable_name, path = partial_pieces(partial_path) - pick_template(path).render_partial(self, variable_name, object_assigns, local_assigns) + pick_template(find_partial_path(partial_path)).render_partial(self, object_assigns, local_assigns) when ActionView::Helpers::FormBuilder builder_partial_path = partial_path.class.to_s.demodulize.underscore.sub(/_builder$/, '') render_partial(builder_partial_path, object_assigns, (local_assigns || {}).merge(builder_partial_path.to_sym => partial_path)) @@ -130,43 +129,29 @@ module ActionView local_assigns = local_assigns ? local_assigns.clone : {} spacer = partial_spacer_template ? render(:partial => partial_spacer_template) : '' - _partial_pieces = {} + _paths = {} _templates = {} index = 0 collection.map do |object| _partial_path ||= partial_path || ActionController::RecordIdentifier.partial_path(object, controller.class.controller_path) - variable_name, path = _partial_pieces[_partial_path] ||= partial_pieces(_partial_path) + path = _paths[_partial_path] ||= find_partial_path(_partial_path) template = _templates[path] ||= pick_template(path) - - local_assigns["#{variable_name}_counter".to_sym] = index - local_assigns[:object] = local_assigns[variable_name] = object - local_assigns[as] = object if as - - result = template.render_partial(self, variable_name, object, local_assigns) - - local_assigns.delete(as) - local_assigns.delete(variable_name) - local_assigns.delete(:object) + local_assigns[template.counter_name] = index + result = template.render_partial(self, object, local_assigns, as) index += 1 - result end.join(spacer) end - def partial_pieces(partial_path) + def find_partial_path(partial_path) if partial_path.include?('/') - variable_name = File.basename(partial_path) - path = "#{File.dirname(partial_path)}/_#{variable_name}" + "#{File.dirname(partial_path)}/_#{File.basename(partial_path)}" elsif respond_to?(:controller) - variable_name = partial_path - path = "#{controller.class.controller_path}/_#{variable_name}" + "#{controller.class.controller_path}/_#{partial_path}" else - variable_name = partial_path - path = "_#{variable_name}" + "_#{partial_path}" end - variable_name = variable_name.sub(/\..*$/, '').to_sym - return variable_name, path end end end diff --git a/actionpack/lib/action_view/renderable.rb b/actionpack/lib/action_view/renderable.rb index ffcffd1667..4fda408367 100644 --- a/actionpack/lib/action_view/renderable.rb +++ b/actionpack/lib/action_view/renderable.rb @@ -7,16 +7,21 @@ module ActionView @@mutex = Mutex.new end - # NOTE: Exception to earlier notice. Ensure this is called before freeze def handler @handler ||= Template.handler_class_for_extension(extension) end - # NOTE: Exception to earlier notice. Ensure this is called before freeze def compiled_source @compiled_source ||= handler.new(nil).compile(self) if handler.compilable? end + def freeze + # Eager load and freeze memoized methods + handler.freeze + compiled_source.freeze + super + end + def render(view, local_assigns = {}) view._first_render ||= self view._last_render = self diff --git a/actionpack/lib/action_view/renderable_partial.rb b/actionpack/lib/action_view/renderable_partial.rb index 6a17b50a14..250b4e1624 100644 --- a/actionpack/lib/action_view/renderable_partial.rb +++ b/actionpack/lib/action_view/renderable_partial.rb @@ -3,16 +3,36 @@ module ActionView # NOTE: The template that this mixin is beening include into is frozen # So you can not set or modify any instance variables + def variable_name + @variable_name ||= name.gsub(/^_/, '').to_sym + end + + def counter_name + @counter_name ||= "#{variable_name}_counter".to_sym + end + + def freeze + # Eager load and freeze memoized methods + variable_name.freeze + counter_name.freeze + super + end + def render(view, local_assigns = {}) ActionController::Base.benchmark("Rendered #{path_without_format_and_extension}", Logger::DEBUG, false) do super end end - def render_partial(view, variable_name, object = nil, local_assigns = {}, as = nil) - object ||= view.controller.instance_variable_get("@#{variable_name}") if view.respond_to?(:controller) - local_assigns[:object] ||= local_assigns[variable_name] ||= object - local_assigns[as] ||= local_assigns[:object] if as + def render_partial(view, object = nil, local_assigns = {}, as = nil) + object ||= local_assigns[:object] || + local_assigns[variable_name] || + view.controller.instance_variable_get("@#{variable_name}") if view.respond_to?(:controller) + + # Ensure correct object is reassigned to other accessors + local_assigns[:object] = local_assigns[variable_name] = object + local_assigns[as] = object if as + render_template(view, local_assigns) end end diff --git a/actionpack/lib/action_view/template.rb b/actionpack/lib/action_view/template.rb index 03f9234289..42659efbd4 100644 --- a/actionpack/lib/action_view/template.rb +++ b/actionpack/lib/action_view/template.rb @@ -17,19 +17,13 @@ module ActionView #:nodoc: end def freeze - # Eager load memoized methods - format_and_extension - path - path_without_extension - path_without_format_and_extension - source - method_segment - - # Eager load memoized methods from Renderable - handler - compiled_source - - instance_variables.each { |ivar| ivar.freeze } + # Eager load and freeze memoized methods + format_and_extension.freeze + path.freeze + path_without_extension.freeze + path_without_format_and_extension.freeze + source.freeze + method_segment.freeze super end @@ -51,12 +45,12 @@ module ActionView #:nodoc: end def source - @source ||= File.read(@filename) + @source ||= File.read(filename) end def method_segment unless @method_segment - segment = File.expand_path(@filename) + segment = File.expand_path(filename) segment.sub!(/^#{Regexp.escape(File.expand_path(RAILS_ROOT))}/, '') if defined?(RAILS_ROOT) segment.gsub!(/([^a-zA-Z0-9_])/) { $1.ord } @method_segment = segment From d27dd860c7f4f9b9e5aebe7d0c6e9b6108d8717c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tarmo=20T=C3=A4nav?= Date: Mon, 14 Jul 2008 18:02:59 -0500 Subject: [PATCH 65/74] Use sub instead of gsub Signed-off-by: Joshua Peek --- actionpack/lib/action_view/renderable_partial.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionpack/lib/action_view/renderable_partial.rb b/actionpack/lib/action_view/renderable_partial.rb index 250b4e1624..7b51eccc27 100644 --- a/actionpack/lib/action_view/renderable_partial.rb +++ b/actionpack/lib/action_view/renderable_partial.rb @@ -4,7 +4,7 @@ module ActionView # So you can not set or modify any instance variables def variable_name - @variable_name ||= name.gsub(/^_/, '').to_sym + @variable_name ||= name.sub(/\A_/, '').to_sym end def counter_name From 8a9934a9d9fc98b56c4566ae2e3fd4d83e505d3e Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Mon, 14 Jul 2008 19:50:32 -0500 Subject: [PATCH 66/74] Added Memoizable mixin for caching simple lazy loaded attributes --- activesupport/CHANGELOG | 2 + activesupport/lib/active_support.rb | 1 + .../lib/active_support/memoizable.rb | 32 +++++++++++++ activesupport/test/memoizable_test.rb | 45 +++++++++++++++++++ 4 files changed, 80 insertions(+) create mode 100644 activesupport/lib/active_support/memoizable.rb create mode 100644 activesupport/test/memoizable_test.rb diff --git a/activesupport/CHANGELOG b/activesupport/CHANGELOG index 983e7d0dac..0c308a1cfe 100644 --- a/activesupport/CHANGELOG +++ b/activesupport/CHANGELOG @@ -1,5 +1,7 @@ *Edge* +* Added Memoizable mixin for caching simple lazy loaded attributes [Josh Peek] + * Move the test related core_ext stuff out of core_ext so it's only loaded by the test helpers. [Michael Koziarski] * Add Inflection rules for String#humanize. #535 [dcmanges] diff --git a/activesupport/lib/active_support.rb b/activesupport/lib/active_support.rb index 1a8603e892..0526057b15 100644 --- a/activesupport/lib/active_support.rb +++ b/activesupport/lib/active_support.rb @@ -43,6 +43,7 @@ require 'active_support/ordered_hash' require 'active_support/ordered_options' require 'active_support/option_merger' +require 'active_support/memoizable' require 'active_support/string_inquirer' require 'active_support/values/time_zone' diff --git a/activesupport/lib/active_support/memoizable.rb b/activesupport/lib/active_support/memoizable.rb new file mode 100644 index 0000000000..c78fb0a793 --- /dev/null +++ b/activesupport/lib/active_support/memoizable.rb @@ -0,0 +1,32 @@ +module ActiveSupport + module Memoizable + def self.included(base) #:nodoc: + base.extend(ClassMethods) + end + + module ClassMethods + def memorize(symbol) + original_method = "_unmemorized_#{symbol}" + alias_method original_method, symbol + class_eval <<-EOS, __FILE__, __LINE__ + def #{symbol} + if instance_variable_defined?(:@#{symbol}) + @#{symbol} + else + @#{symbol} = #{original_method} + end + end + EOS + end + end + + def freeze + methods.each do |method| + if m = method.to_s.match(/^_unmemorized_(.*)/) + send(m[1]).freeze + end + end + super + end + end +end diff --git a/activesupport/test/memoizable_test.rb b/activesupport/test/memoizable_test.rb new file mode 100644 index 0000000000..40a02cf253 --- /dev/null +++ b/activesupport/test/memoizable_test.rb @@ -0,0 +1,45 @@ +require 'abstract_unit' + +uses_mocha 'Memoizable' do + class MemoizableTest < Test::Unit::TestCase + class Person + include ActiveSupport::Memoizable + + def name + fetch_name_from_floppy + end + memorize :name + + def age + nil + end + memorize :age + + private + def fetch_name_from_floppy + "Josh" + end + end + + def test_memoization + person = Person.new + assert_equal "Josh", person.name + + person.expects(:fetch_name_from_floppy).never + 2.times { assert_equal "Josh", person.name } + end + + def test_memoized_methods_are_frozen + person = Person.new + person.freeze + assert_equal "Josh", person.name + assert_equal true, person.name.frozen? + end + + def test_memoization_frozen_with_nil_value + person = Person.new + person.freeze + assert_equal nil, person.age + end + end +end From dd41f66af577947ad420fbd2a44184344ad5c983 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Mon, 14 Jul 2008 19:51:43 -0500 Subject: [PATCH 67/74] Include Memoizable in ActionView::Template --- actionpack/lib/action_view/renderable.rb | 15 +++---- .../lib/action_view/renderable_partial.rb | 15 +++---- actionpack/lib/action_view/template.rb | 40 +++++++------------ 3 files changed, 27 insertions(+), 43 deletions(-) diff --git a/actionpack/lib/action_view/renderable.rb b/actionpack/lib/action_view/renderable.rb index 4fda408367..9185045adf 100644 --- a/actionpack/lib/action_view/renderable.rb +++ b/actionpack/lib/action_view/renderable.rb @@ -7,20 +7,17 @@ module ActionView @@mutex = Mutex.new end + include ActiveSupport::Memoizable + def handler - @handler ||= Template.handler_class_for_extension(extension) + Template.handler_class_for_extension(extension) end + memorize :handler def compiled_source - @compiled_source ||= handler.new(nil).compile(self) if handler.compilable? - end - - def freeze - # Eager load and freeze memoized methods - handler.freeze - compiled_source.freeze - super + handler.new(nil).compile(self) if handler.compilable? end + memorize :compiled_source def render(view, local_assigns = {}) view._first_render ||= self diff --git a/actionpack/lib/action_view/renderable_partial.rb b/actionpack/lib/action_view/renderable_partial.rb index 7b51eccc27..d2fe635b9c 100644 --- a/actionpack/lib/action_view/renderable_partial.rb +++ b/actionpack/lib/action_view/renderable_partial.rb @@ -3,20 +3,17 @@ module ActionView # NOTE: The template that this mixin is beening include into is frozen # So you can not set or modify any instance variables + include ActiveSupport::Memoizable + def variable_name - @variable_name ||= name.sub(/\A_/, '').to_sym + name.sub(/\A_/, '').to_sym end + memorize :variable_name def counter_name - @counter_name ||= "#{variable_name}_counter".to_sym - end - - def freeze - # Eager load and freeze memoized methods - variable_name.freeze - counter_name.freeze - super + "#{variable_name}_counter".to_sym end + memorize :counter_name def render(view, local_assigns = {}) ActionController::Base.benchmark("Rendered #{path_without_format_and_extension}", Logger::DEBUG, false) do diff --git a/actionpack/lib/action_view/template.rb b/actionpack/lib/action_view/template.rb index 42659efbd4..b39ba0c48b 100644 --- a/actionpack/lib/action_view/template.rb +++ b/actionpack/lib/action_view/template.rb @@ -1,6 +1,7 @@ module ActionView #:nodoc: class Template extend TemplateHandlers + include ActiveSupport::Memoizable include Renderable attr_accessor :filename, :load_path, :base_path, :name, :format, :extension @@ -16,48 +17,37 @@ module ActionView #:nodoc: extend RenderablePartial if @name =~ /^_/ end - def freeze - # Eager load and freeze memoized methods - format_and_extension.freeze - path.freeze - path_without_extension.freeze - path_without_format_and_extension.freeze - source.freeze - method_segment.freeze - - super - end - def format_and_extension - @format_and_extension ||= (extensions = [format, extension].compact.join(".")).blank? ? nil : extensions + (extensions = [format, extension].compact.join(".")).blank? ? nil : extensions end + memorize :format_and_extension def path - @path ||= [base_path, [name, format, extension].compact.join('.')].compact.join('/') + [base_path, [name, format, extension].compact.join('.')].compact.join('/') end + memorize :path def path_without_extension - @path_without_extension ||= [base_path, [name, format].compact.join('.')].compact.join('/') + [base_path, [name, format].compact.join('.')].compact.join('/') end + memorize :path_without_extension def path_without_format_and_extension - @path_without_format_and_extension ||= [base_path, name].compact.join('/') + [base_path, name].compact.join('/') end + memorize :path_without_format_and_extension def source - @source ||= File.read(filename) + File.read(filename) end + memorize :source def method_segment - unless @method_segment - segment = File.expand_path(filename) - segment.sub!(/^#{Regexp.escape(File.expand_path(RAILS_ROOT))}/, '') if defined?(RAILS_ROOT) - segment.gsub!(/([^a-zA-Z0-9_])/) { $1.ord } - @method_segment = segment - end - - @method_segment + segment = File.expand_path(filename) + segment.sub!(/^#{Regexp.escape(File.expand_path(RAILS_ROOT))}/, '') if defined?(RAILS_ROOT) + segment.gsub!(/([^a-zA-Z0-9_])/) { $1.ord } end + memorize :method_segment def render_template(view, local_assigns = {}) render(view, local_assigns) From 001c8beb4d0999a858a8b52ad511ee1251cc3517 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Mon, 14 Jul 2008 20:02:59 -0500 Subject: [PATCH 68/74] memorize typo --- actionpack/lib/action_view/renderable.rb | 4 ++-- actionpack/lib/action_view/renderable_partial.rb | 4 ++-- actionpack/lib/action_view/template.rb | 12 ++++++------ activesupport/lib/active_support/memoizable.rb | 6 +++--- activesupport/test/memoizable_test.rb | 4 ++-- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/actionpack/lib/action_view/renderable.rb b/actionpack/lib/action_view/renderable.rb index 9185045adf..f66356c939 100644 --- a/actionpack/lib/action_view/renderable.rb +++ b/actionpack/lib/action_view/renderable.rb @@ -12,12 +12,12 @@ module ActionView def handler Template.handler_class_for_extension(extension) end - memorize :handler + memoize :handler def compiled_source handler.new(nil).compile(self) if handler.compilable? end - memorize :compiled_source + memoize :compiled_source def render(view, local_assigns = {}) view._first_render ||= self diff --git a/actionpack/lib/action_view/renderable_partial.rb b/actionpack/lib/action_view/renderable_partial.rb index d2fe635b9c..fdb1a5e6a7 100644 --- a/actionpack/lib/action_view/renderable_partial.rb +++ b/actionpack/lib/action_view/renderable_partial.rb @@ -8,12 +8,12 @@ module ActionView def variable_name name.sub(/\A_/, '').to_sym end - memorize :variable_name + memoize :variable_name def counter_name "#{variable_name}_counter".to_sym end - memorize :counter_name + memoize :counter_name def render(view, local_assigns = {}) ActionController::Base.benchmark("Rendered #{path_without_format_and_extension}", Logger::DEBUG, false) do diff --git a/actionpack/lib/action_view/template.rb b/actionpack/lib/action_view/template.rb index b39ba0c48b..304aec3a4c 100644 --- a/actionpack/lib/action_view/template.rb +++ b/actionpack/lib/action_view/template.rb @@ -20,34 +20,34 @@ module ActionView #:nodoc: def format_and_extension (extensions = [format, extension].compact.join(".")).blank? ? nil : extensions end - memorize :format_and_extension + memoize :format_and_extension def path [base_path, [name, format, extension].compact.join('.')].compact.join('/') end - memorize :path + memoize :path def path_without_extension [base_path, [name, format].compact.join('.')].compact.join('/') end - memorize :path_without_extension + memoize :path_without_extension def path_without_format_and_extension [base_path, name].compact.join('/') end - memorize :path_without_format_and_extension + memoize :path_without_format_and_extension def source File.read(filename) end - memorize :source + memoize :source def method_segment segment = File.expand_path(filename) segment.sub!(/^#{Regexp.escape(File.expand_path(RAILS_ROOT))}/, '') if defined?(RAILS_ROOT) segment.gsub!(/([^a-zA-Z0-9_])/) { $1.ord } end - memorize :method_segment + memoize :method_segment def render_template(view, local_assigns = {}) render(view, local_assigns) diff --git a/activesupport/lib/active_support/memoizable.rb b/activesupport/lib/active_support/memoizable.rb index c78fb0a793..65feca363a 100644 --- a/activesupport/lib/active_support/memoizable.rb +++ b/activesupport/lib/active_support/memoizable.rb @@ -5,8 +5,8 @@ module ActiveSupport end module ClassMethods - def memorize(symbol) - original_method = "_unmemorized_#{symbol}" + def memoize(symbol) + original_method = "_unmemoized_#{symbol}" alias_method original_method, symbol class_eval <<-EOS, __FILE__, __LINE__ def #{symbol} @@ -22,7 +22,7 @@ module ActiveSupport def freeze methods.each do |method| - if m = method.to_s.match(/^_unmemorized_(.*)/) + if m = method.to_s.match(/^_unmemoized_(.*)/) send(m[1]).freeze end end diff --git a/activesupport/test/memoizable_test.rb b/activesupport/test/memoizable_test.rb index 40a02cf253..1b6cec2b2f 100644 --- a/activesupport/test/memoizable_test.rb +++ b/activesupport/test/memoizable_test.rb @@ -8,12 +8,12 @@ uses_mocha 'Memoizable' do def name fetch_name_from_floppy end - memorize :name + memoize :name def age nil end - memorize :age + memoize :age private def fetch_name_from_floppy From 911c2c381347ffb04615896ee6afe45277eeb103 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tarmo=20T=C3=A4nav?= Date: Mon, 14 Jul 2008 20:23:23 -0500 Subject: [PATCH 69/74] Some performance tweaks to ActiveSupport::Memoizable Signed-off-by: Joshua Peek --- activesupport/lib/active_support/memoizable.rb | 6 ++++-- activesupport/test/memoizable_test.rb | 4 ++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/activesupport/lib/active_support/memoizable.rb b/activesupport/lib/active_support/memoizable.rb index 65feca363a..5af50df023 100644 --- a/activesupport/lib/active_support/memoizable.rb +++ b/activesupport/lib/active_support/memoizable.rb @@ -7,10 +7,12 @@ module ActiveSupport module ClassMethods def memoize(symbol) original_method = "_unmemoized_#{symbol}" + raise "Already memoized #{symbol}" if instance_methods.map(&:to_s).include?(original_method) + alias_method original_method, symbol class_eval <<-EOS, __FILE__, __LINE__ def #{symbol} - if instance_variable_defined?(:@#{symbol}) + if defined? @#{symbol} @#{symbol} else @#{symbol} = #{original_method} @@ -22,7 +24,7 @@ module ActiveSupport def freeze methods.each do |method| - if m = method.to_s.match(/^_unmemoized_(.*)/) + if m = method.to_s.match(/\A_unmemoized_(.*)/) send(m[1]).freeze end end diff --git a/activesupport/test/memoizable_test.rb b/activesupport/test/memoizable_test.rb index 1b6cec2b2f..fc24a2942d 100644 --- a/activesupport/test/memoizable_test.rb +++ b/activesupport/test/memoizable_test.rb @@ -41,5 +41,9 @@ uses_mocha 'Memoizable' do person.freeze assert_equal nil, person.age end + + def test_double_memoization + assert_raise(RuntimeError) { Person.memoize :name } + end end end From 7f0346237e30e55d6cd16a8b4a9dfe4193f61804 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Mon, 14 Jul 2008 20:25:09 -0500 Subject: [PATCH 70/74] Append a "_" to memoized instance variables --- activesupport/lib/active_support/memoizable.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/activesupport/lib/active_support/memoizable.rb b/activesupport/lib/active_support/memoizable.rb index 5af50df023..d06250171a 100644 --- a/activesupport/lib/active_support/memoizable.rb +++ b/activesupport/lib/active_support/memoizable.rb @@ -7,15 +7,16 @@ module ActiveSupport module ClassMethods def memoize(symbol) original_method = "_unmemoized_#{symbol}" + memoized_ivar = "@_memoized_#{symbol}" raise "Already memoized #{symbol}" if instance_methods.map(&:to_s).include?(original_method) alias_method original_method, symbol class_eval <<-EOS, __FILE__, __LINE__ def #{symbol} - if defined? @#{symbol} - @#{symbol} + if defined? #{memoized_ivar} + #{memoized_ivar} else - @#{symbol} = #{original_method} + #{memoized_ivar} = #{original_method} end end EOS From 34510456585216004e483b79beeea3ddc3eb4de6 Mon Sep 17 00:00:00 2001 From: gbuesing Date: Mon, 14 Jul 2008 23:16:39 -0500 Subject: [PATCH 71/74] Fix TimeWithZone unmarshaling: coerce unmarshaled Time instances to utc, because Ruby's marshaling of Time instances doesn't respect the zone --- activesupport/CHANGELOG | 2 ++ activesupport/lib/active_support/time_with_zone.rb | 2 +- activesupport/test/core_ext/time_with_zone_test.rb | 6 ++++++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/activesupport/CHANGELOG b/activesupport/CHANGELOG index 0c308a1cfe..a7b33c09ad 100644 --- a/activesupport/CHANGELOG +++ b/activesupport/CHANGELOG @@ -1,5 +1,7 @@ *Edge* +* Fix TimeWithZone unmarshaling: coerce unmarshaled Time instances to utc, because Ruby's marshaling of Time instances doesn't respect the zone [Geoff Buesing] + * Added Memoizable mixin for caching simple lazy loaded attributes [Josh Peek] * Move the test related core_ext stuff out of core_ext so it's only loaded by the test helpers. [Michael Koziarski] diff --git a/activesupport/lib/active_support/time_with_zone.rb b/activesupport/lib/active_support/time_with_zone.rb index 88593eb92d..e85bfe9b2e 100644 --- a/activesupport/lib/active_support/time_with_zone.rb +++ b/activesupport/lib/active_support/time_with_zone.rb @@ -263,7 +263,7 @@ module ActiveSupport end def marshal_load(variables) - initialize(variables[0], ::Time.send!(:get_zone, variables[1]), variables[2]) + initialize(variables[0].utc, ::Time.send!(:get_zone, variables[1]), variables[2].utc) end # Ensure proxy class responds to all methods that underlying time instance responds to. diff --git a/activesupport/test/core_ext/time_with_zone_test.rb b/activesupport/test/core_ext/time_with_zone_test.rb index ac52a1be0b..dfe04485be 100644 --- a/activesupport/test/core_ext/time_with_zone_test.rb +++ b/activesupport/test/core_ext/time_with_zone_test.rb @@ -320,8 +320,11 @@ class TimeWithZoneTest < Test::Unit::TestCase marshal_str = Marshal.dump(@twz) mtime = Marshal.load(marshal_str) assert_equal Time.utc(2000, 1, 1, 0), mtime.utc + assert mtime.utc.utc? assert_equal ActiveSupport::TimeZone['Eastern Time (US & Canada)'], mtime.time_zone assert_equal Time.utc(1999, 12, 31, 19), mtime.time + assert mtime.time.utc? + assert_equal @twz.inspect, mtime.inspect end end @@ -331,8 +334,11 @@ class TimeWithZoneTest < Test::Unit::TestCase marshal_str = Marshal.dump(twz) mtime = Marshal.load(marshal_str) assert_equal Time.utc(2000, 1, 1, 0), mtime.utc + assert mtime.utc.utc? assert_equal 'America/New_York', mtime.time_zone.name assert_equal Time.utc(1999, 12, 31, 19), mtime.time + assert mtime.time.utc? + assert_equal @twz.inspect, mtime.inspect end end From 8c91b767c0f36e9d767eb230ec42b111e57d90da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tarmo=20T=C3=A4nav?= Date: Tue, 15 Jul 2008 05:17:06 +0300 Subject: [PATCH 72/74] Fixed postgresql limited eager loading for the case where scoped :order was present --- activerecord/lib/active_record/associations.rb | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index d43e07ab4e..7ad7802cbc 100755 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1486,10 +1486,15 @@ module ActiveRecord join_dependency.joins_for_table_name(table) }.flatten.compact.uniq + order = options[:order] + if scoped_order = (scope && scope[:order]) + order = order ? "#{order}, #{scoped_order}" : scoped_order + end + is_distinct = !options[:joins].blank? || include_eager_conditions?(options, tables_from_conditions) || include_eager_order?(options, tables_from_order) sql = "SELECT " if is_distinct - sql << connection.distinct("#{connection.quote_table_name table_name}.#{primary_key}", options[:order]) + sql << connection.distinct("#{connection.quote_table_name table_name}.#{primary_key}", order) else sql << primary_key end @@ -1503,8 +1508,8 @@ module ActiveRecord add_conditions!(sql, options[:conditions], scope) add_group!(sql, options[:group], scope) - if options[:order] && is_distinct - connection.add_order_by_for_association_limiting!(sql, options) + if order && is_distinct + connection.add_order_by_for_association_limiting!(sql, :order => order) else add_order!(sql, options[:order], scope) end From c1531ae00dbd3ac804bce02733e050ec43400607 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tarmo=20T=C3=A4nav?= Date: Tue, 15 Jul 2008 05:24:24 +0300 Subject: [PATCH 73/74] SQLite: rename_column raises if the column doesn't exist. [#622 state:resolved] --- .../lib/active_record/connection_adapters/sqlite_adapter.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb index f4d387cfb4..84f8c0284e 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb @@ -260,6 +260,9 @@ module ActiveRecord end def rename_column(table_name, column_name, new_column_name) #:nodoc: + unless columns(table_name).detect{|c| c.name == column_name.to_s } + raise ActiveRecord::ActiveRecordError, "Missing column #{table_name}.#{column_name}" + end alter_table(table_name, :rename => {column_name.to_s => new_column_name.to_s}) end From fc89a951933638b051bb1f9e1339ee6ae7c94cda Mon Sep 17 00:00:00 2001 From: Adrian Mugnolo Date: Tue, 15 Jul 2008 01:17:03 -0300 Subject: [PATCH 74/74] Add in_groups to ActiveSupport::CoreExtensions::Array::Grouping. [#579 state:resolved] Signed-off-by: Pratik Naik --- activesupport/CHANGELOG | 6 +++ .../active_support/core_ext/array/grouping.rb | 47 ++++++++++++++++- activesupport/test/core_ext/array_ext_test.rb | 50 +++++++++++++++++-- 3 files changed, 97 insertions(+), 6 deletions(-) diff --git a/activesupport/CHANGELOG b/activesupport/CHANGELOG index a7b33c09ad..8d3b136d80 100644 --- a/activesupport/CHANGELOG +++ b/activesupport/CHANGELOG @@ -1,5 +1,11 @@ *Edge* +* Add Array#in_groups which splits or iterates over the array in specified number of groups. #579. [Adrian Mugnolo] Example: + + a = (1..10).to_a + a.in_groups(3) #=> [[1, 2, 3, 4], [5, 6, 7, nil], [8, 9, 10, nil]] + a.in_groups(3, false) #=> [[1, 2, 3, 4], [5, 6, 7], [8, 9, 10]] + * Fix TimeWithZone unmarshaling: coerce unmarshaled Time instances to utc, because Ruby's marshaling of Time instances doesn't respect the zone [Geoff Buesing] * Added Memoizable mixin for caching simple lazy loaded attributes [Josh Peek] diff --git a/activesupport/lib/active_support/core_ext/array/grouping.rb b/activesupport/lib/active_support/core_ext/array/grouping.rb index 767acc4e07..df37afb053 100644 --- a/activesupport/lib/active_support/core_ext/array/grouping.rb +++ b/activesupport/lib/active_support/core_ext/array/grouping.rb @@ -4,8 +4,8 @@ module ActiveSupport #:nodoc: module CoreExtensions #:nodoc: module Array #:nodoc: module Grouping - # Iterates over the array in groups of size +number+, padding any remaining - # slots with +fill_with+ unless it is +false+. + # Splits or iterates over the array in groups of size +number+, + # padding any remaining slots with +fill_with+ unless it is +false+. # # %w(1 2 3 4 5 6 7).in_groups_of(3) {|g| p g} # ["1", "2", "3"] @@ -39,6 +39,49 @@ module ActiveSupport #:nodoc: end end + # Splits or iterates over the array in +number+ of groups, padding any + # remaining slots with +fill_with+ unless it is +false+. + # + # %w(1 2 3 4 5 6 7 8 9 10).in_groups(3) {|g| p g} + # ["1", "2", "3", "4"] + # ["5", "6", "7", nil] + # ["8", "9", "10", nil] + # + # %w(1 2 3 4 5 6 7).in_groups(3, ' ') {|g| p g} + # ["1", "2", "3"] + # ["4", "5", " "] + # ["6", "7", " "] + # + # %w(1 2 3 4 5 6 7).in_groups(3, false) {|g| p g} + # ["1", "2", "3"] + # ["4", "5"] + # ["6", "7"] + def in_groups(number, fill_with = nil) + # size / number gives minor group size; + # size % number gives how many objects need extra accomodation; + # each group hold either division or division + 1 items. + division = size / number + modulo = size % number + + # create a new array avoiding dup + groups = [] + start = 0 + + number.times do |index| + length = division + (modulo > 0 && modulo > index ? 1 : 0) + padding = fill_with != false && + modulo > 0 && length == division ? 1 : 0 + groups << slice(start, length).concat([fill_with] * padding) + start += length + end + + if block_given? + groups.each{|g| yield(g) } + else + groups + end + end + # Divides the array into one or more subarrays based on a delimiting +value+ # or the result of an optional block. # diff --git a/activesupport/test/core_ext/array_ext_test.rb b/activesupport/test/core_ext/array_ext_test.rb index 7563be44f8..62a1f61d53 100644 --- a/activesupport/test/core_ext/array_ext_test.rb +++ b/activesupport/test/core_ext/array_ext_test.rb @@ -99,7 +99,7 @@ class ArrayExtToSTests < Test::Unit::TestCase end class ArrayExtGroupingTests < Test::Unit::TestCase - def test_group_by_with_perfect_fit + def test_in_groups_of_with_perfect_fit groups = [] ('a'..'i').to_a.in_groups_of(3) do |group| groups << group @@ -109,7 +109,7 @@ class ArrayExtGroupingTests < Test::Unit::TestCase assert_equal [%w(a b c), %w(d e f), %w(g h i)], ('a'..'i').to_a.in_groups_of(3) end - def test_group_by_with_padding + def test_in_groups_of_with_padding groups = [] ('a'..'g').to_a.in_groups_of(3) do |group| groups << group @@ -118,7 +118,7 @@ class ArrayExtGroupingTests < Test::Unit::TestCase assert_equal [%w(a b c), %w(d e f), ['g', nil, nil]], groups end - def test_group_by_pads_with_specified_values + def test_in_groups_of_pads_with_specified_values groups = [] ('a'..'g').to_a.in_groups_of(3, 'foo') do |group| @@ -128,7 +128,7 @@ class ArrayExtGroupingTests < Test::Unit::TestCase assert_equal [%w(a b c), %w(d e f), ['g', 'foo', 'foo']], groups end - def test_group_without_padding + def test_in_groups_of_without_padding groups = [] ('a'..'g').to_a.in_groups_of(3, false) do |group| @@ -137,6 +137,48 @@ class ArrayExtGroupingTests < Test::Unit::TestCase assert_equal [%w(a b c), %w(d e f), ['g']], groups end + + def test_in_groups_returned_array_size + array = (1..7).to_a + + 1.upto(array.size + 1) do |number| + assert_equal number, array.in_groups(number).size + end + end + + def test_in_groups_with_empty_array + assert_equal [[], [], []], [].in_groups(3) + end + + def test_in_groups_with_block + array = (1..9).to_a + groups = [] + + array.in_groups(3) do |group| + groups << group + end + + assert_equal array.in_groups(3), groups + end + + def test_in_groups_with_perfect_fit + assert_equal [[1, 2, 3], [4, 5, 6], [7, 8, 9]], + (1..9).to_a.in_groups(3) + end + + def test_in_groups_with_padding + array = (1..7).to_a + + assert_equal [[1, 2, 3], [4, 5, nil], [6, 7, nil]], + array.in_groups(3) + assert_equal [[1, 2, 3], [4, 5, 'foo'], [6, 7, 'foo']], + array.in_groups(3, 'foo') + end + + def test_in_groups_without_padding + assert_equal [[1, 2, 3], [4, 5], [6, 7]], + (1..7).to_a.in_groups(3, false) + end end class ArraySplitTests < Test::Unit::TestCase