@@ -1,7 +1,7 @@ module ActionPack #:nodoc: module VERSION #:nodoc: MAJOR = 2 - MINOR = 1 + MINOR = 2 TINY = 0 STRING = [MAJOR, MINOR, TINY].join('.') actionpack/lib/action_pack/version.rb @@ -151,7 +151,7 @@ module ActionView # javascript_path "http://www.railsapplication.com/js/xmlhr" # => http://www.railsapplication.com/js/xmlhr.js # javascript_path "http://www.railsapplication.com/js/xmlhr.js" # => http://www.railsapplication.com/js/xmlhr.js def javascript_path(source) - compute_public_path(source, 'javascripts', 'js') + JavaScriptTag.create(self, @controller, source).public_path end alias_method :path_to_javascript, :javascript_path # aliased to avoid conflicts with a javascript_path named route @@ -249,15 +249,17 @@ module ActionView 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, recursive)) unless File.exists?(joined_javascript_path) + unless File.exists?(joined_javascript_path) + JavaScriptSources.create(self, @controller, sources, recursive).write_asset_file_contents(joined_javascript_path) + end javascript_src_tag(joined_javascript_name, options) else - expand_javascript_sources(sources, recursive).collect { |source| javascript_src_tag(source, options) }.join("\n") + JavaScriptSources.create(self, @controller, sources, recursive).expand_sources.collect { |source| + javascript_src_tag(source, options) + }.join("\n") end end - @@javascript_expansions = { :defaults => JAVASCRIPT_DEFAULT_SOURCES.dup } - # Register one or more javascript files to be included when <tt>symbol</tt> # is passed to <tt>javascript_include_tag</tt>. This method is typically intended # to be called from plugin initialization to register javascript files @@ -270,11 +272,9 @@ module ActionView # <script type="text/javascript" src="/javascripts/body.js"></script> # <script type="text/javascript" src="/javascripts/tail.js"></script> def self.register_javascript_expansion(expansions) - @@javascript_expansions.merge!(expansions) + JavaScriptSources.expansions.merge!(expansions) end - @@stylesheet_expansions = {} - # Register one or more stylesheet files to be included when <tt>symbol</tt> # is passed to <tt>stylesheet_link_tag</tt>. This method is typically intended # to be called from plugin initialization to register stylesheet files @@ -287,7 +287,7 @@ module ActionView # <link href="/stylesheets/body.css" media="screen" rel="stylesheet" type="text/css" /> # <link href="/stylesheets/tail.css" media="screen" rel="stylesheet" type="text/css" /> def self.register_stylesheet_expansion(expansions) - @@stylesheet_expansions.merge!(expansions) + StylesheetSources.expansions.merge!(expansions) end # Register one or more additional JavaScript files to be included when @@ -295,11 +295,11 @@ module ActionView # typically intended to be called from plugin initialization to register additional # .js files that the plugin installed in <tt>public/javascripts</tt>. def self.register_javascript_include_default(*sources) - @@javascript_expansions[:defaults].concat(sources) + JavaScriptSources.expansions[:defaults].concat(sources) end def self.reset_javascript_include_default #:nodoc: - @@javascript_expansions[:defaults] = JAVASCRIPT_DEFAULT_SOURCES.dup + JavaScriptSources.expansions[:defaults] = JAVASCRIPT_DEFAULT_SOURCES.dup end # Computes the path to a stylesheet asset in the public stylesheets directory. @@ -314,7 +314,7 @@ module ActionView # stylesheet_path "http://www.railsapplication.com/css/style" # => http://www.railsapplication.com/css/style.css # stylesheet_path "http://www.railsapplication.com/css/style.js" # => http://www.railsapplication.com/css/style.css def stylesheet_path(source) - compute_public_path(source, 'stylesheets', 'css') + StylesheetTag.create(self, @controller, source).public_path end alias_method :path_to_stylesheet, :stylesheet_path # aliased to avoid conflicts with a stylesheet_path named route @@ -389,10 +389,14 @@ module ActionView 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, recursive)) unless File.exists?(joined_stylesheet_path) + unless File.exists?(joined_stylesheet_path) + StylesheetSources.create(self, @controller, sources, recursive).write_asset_file_contents(joined_stylesheet_path) + end stylesheet_tag(joined_stylesheet_name, options) else - expand_stylesheet_sources(sources, recursive).collect { |source| stylesheet_tag(source, options) }.join("\n") + StylesheetSources.create(self, @controller, sources, recursive).expand_sources.collect { |source| + stylesheet_tag(source, options) + }.join("\n") end end @@ -407,7 +411,7 @@ module ActionView # image_path("/icons/edit.png") # => /icons/edit.png # image_path("http://www.railsapplication.com/img/edit.png") # => http://www.railsapplication.com/img/edit.png def image_path(source) - compute_public_path(source, 'images') + ImageTag.create(self, @controller, source).public_path end alias_method :path_to_image, :image_path # aliased to avoid conflicts with an image_path named route @@ -463,180 +467,344 @@ module ActionView end private - COMPUTED_PUBLIC_PATHS = {} - COMPUTED_PUBLIC_PATHS_GUARD = Mutex.new - - # Add the the extension +ext+ if not present. Return full URLs otherwise untouched. - # Prefix with <tt>/dir/</tt> if lacking a leading +/+. Account for relative URL - # roots. Rewrite the asset path for cache-busting asset ids. Include - # asset host, if configured, with the correct request protocol. - def compute_public_path(source, dir, ext = nil, include_host = true) - has_request = @controller.respond_to?(:request) - - cache_key = - if has_request - [ @controller.request.protocol, - ActionController::Base.asset_host.to_s, - ActionController::Base.relative_url_root, - dir, source, ext, include_host ].join - else - [ ActionController::Base.asset_host.to_s, - dir, source, ext, include_host ].join - end + def javascript_src_tag(source, options) + content_tag("script", "", { "type" => Mime::JS, "src" => path_to_javascript(source) }.merge(options)) + end - COMPUTED_PUBLIC_PATHS_GUARD.synchronize do - source = COMPUTED_PUBLIC_PATHS[cache_key] ||= - begin - source += ".#{ext}" if ext && File.extname(source).blank? || File.exist?(File.join(ASSETS_DIR, dir, "#{source}.#{ext}")) + def stylesheet_tag(source, options) + tag("link", { "rel" => "stylesheet", "type" => Mime::CSS, "media" => "screen", "href" => html_escape(path_to_stylesheet(source)) }.merge(options), false, false) + end - if source =~ %r{^[-a-z]+://} - source - else - source = "/#{dir}/#{source}" unless source[0] == ?/ - if has_request - unless source =~ %r{^#{ActionController::Base.relative_url_root}/} - source = "#{ActionController::Base.relative_url_root}#{source}" - end - end + module ImageAsset + DIRECTORY = 'images'.freeze - rewrite_asset_path(source) - end - end + def directory + DIRECTORY end - if include_host && source !~ %r{^[-a-z]+://} - host = compute_asset_host(source) + def extension + nil + end + end - if has_request && !host.blank? && host !~ %r{^[-a-z]+://} - host = "#{@controller.request.protocol}#{host}" - end + module JavaScriptAsset + DIRECTORY = 'javascripts'.freeze + EXTENSION = 'js'.freeze + + def public_directory + JAVASCRIPTS_DIR + end + + def directory + DIRECTORY + end + + def extension + EXTENSION + end + end + + module StylesheetAsset + DIRECTORY = 'stylesheets'.freeze + EXTENSION = 'css'.freeze + + def public_directory + STYLESHEETS_DIR + end + + def directory + DIRECTORY + end - "#{host}#{source}" - else - source + def extension + EXTENSION end end - # Pick an asset host for this source. Returns +nil+ if no host is set, - # the host if no wildcard is set, the host interpolated with the - # numbers 0-3 if it contains <tt>%d</tt> (the number is the source hash mod 4), - # or the value returned from invoking the proc if it's a proc. - def compute_asset_host(source) - if host = ActionController::Base.asset_host - if host.is_a?(Proc) - case host.arity - when 2 - host.call(source, @controller.request) + class AssetTag + extend ActiveSupport::Memoizable + + Cache = {} + CacheGuard = Mutex.new + + def self.create(template, controller, source, include_host = true) + CacheGuard.synchronize do + key = if controller.respond_to?(:request) + [self, controller.request.protocol, + ActionController::Base.asset_host, + ActionController::Base.relative_url_root, + source, include_host] else - host.call(source) + [self, ActionController::Base.asset_host, source, include_host] end - else - (host =~ /%d/) ? host % (source.hash % 4) : host + Cache[key] ||= new(template, controller, source, include_host).freeze end end - end - # Use the RAILS_ASSET_ID environment variable or the source's - # modification time as its cache-busting asset id. - def rails_asset_id(source) - if asset_id = ENV["RAILS_ASSET_ID"] - asset_id - else - path = File.join(ASSETS_DIR, source) + ProtocolRegexp = %r{^[-a-z]+://}.freeze - if File.exist?(path) - File.mtime(path).to_i.to_s - else - '' - end + def initialize(template, controller, source, include_host = true) + # NOTE: The template arg is temporarily needed for a legacy plugin + # hook that is expected to call rewrite_asset_path on the + # template. This should eventually be removed. + @template = template + @controller = controller + @source = source + @include_host = include_host end - end - # Break out the asset path rewrite in case plugins wish to put the asset id - # someplace other than the query string. - def rewrite_asset_path(source) - asset_id = rails_asset_id(source) - if asset_id.blank? - source - else - source + "?#{asset_id}" + def public_path + compute_public_path(@source) end - end + memoize :public_path - def javascript_src_tag(source, options) - content_tag("script", "", { "type" => Mime::JS, "src" => path_to_javascript(source) }.merge(options)) + def asset_file_path + File.join(ASSETS_DIR, public_path.split('?').first) + end + memoize :asset_file_path + + def contents + File.read(asset_file_path) + end + + def mtime + File.mtime(asset_file_path) + end + + private + def request + @controller.request + end + + def request? + @controller.respond_to?(:request) + end + + # Add the the extension +ext+ if not present. Return full URLs otherwise untouched. + # Prefix with <tt>/dir/</tt> if lacking a leading +/+. Account for relative URL + # roots. Rewrite the asset path for cache-busting asset ids. Include + # asset host, if configured, with the correct request protocol. + def compute_public_path(source) + source += ".#{extension}" if missing_extension?(source) + unless source =~ ProtocolRegexp + source = "/#{directory}/#{source}" unless source[0] == ?/ + source = prepend_relative_url_root(source) + source = rewrite_asset_path(source) + end + source = prepend_asset_host(source) + source + end + + def missing_extension?(source) + extension && File.extname(source).blank? || File.exist?(File.join(ASSETS_DIR, directory, "#{source}.#{extension}")) + end + + def prepend_relative_url_root(source) + relative_url_root = ActionController::Base.relative_url_root + if request? && @include_host && source !~ %r{^#{relative_url_root}/} + "#{relative_url_root}#{source}" + else + source + end + end + + def prepend_asset_host(source) + if @include_host && source !~ ProtocolRegexp + host = compute_asset_host(source) + if request? && !host.blank? && host !~ ProtocolRegexp + host = "#{request.protocol}#{host}" + end + "#{host}#{source}" + else + source + end + end + + # Pick an asset host for this source. Returns +nil+ if no host is set, + # the host if no wildcard is set, the host interpolated with the + # numbers 0-3 if it contains <tt>%d</tt> (the number is the source hash mod 4), + # or the value returned from invoking the proc if it's a proc. + def compute_asset_host(source) + if host = ActionController::Base.asset_host + if host.is_a?(Proc) + case host.arity + when 2 + host.call(source, request) + else + host.call(source) + end + else + (host =~ /%d/) ? host % (source.hash % 4) : host + end + end + end + + # Use the RAILS_ASSET_ID environment variable or the source's + # modification time as its cache-busting asset id. + def rails_asset_id(source) + if asset_id = ENV["RAILS_ASSET_ID"] + asset_id + else + path = File.join(ASSETS_DIR, source) + + if File.exist?(path) + File.mtime(path).to_i.to_s + else + '' + end + end + end + + # Break out the asset path rewrite in case plugins wish to put the asset id + # someplace other than the query string. + def rewrite_asset_path(source) + if @template.respond_to?(:rewrite_asset_path) + # DEPRECATE: This way to override rewrite_asset_path + @template.send(:rewrite_asset_path, source) + else + asset_id = rails_asset_id(source) + if asset_id.blank? + source + else + "#{source}?#{asset_id}" + end + end + end end - def stylesheet_tag(source, options) - tag("link", { "rel" => "stylesheet", "type" => Mime::CSS, "media" => "screen", "href" => html_escape(path_to_stylesheet(source)) }.merge(options), false, false) + class ImageTag < AssetTag + include ImageAsset end - def compute_javascript_paths(*args) - expand_javascript_sources(*args).collect { |source| compute_public_path(source, 'javascripts', 'js', false) } + class JavaScriptTag < AssetTag + include JavaScriptAsset end - def compute_stylesheet_paths(*args) - expand_stylesheet_sources(*args).collect { |source| compute_public_path(source, 'stylesheets', 'css', false) } + class StylesheetTag < AssetTag + include StylesheetAsset end - def expand_javascript_sources(sources, recursive = false) - if sources.include?(:all) - 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) - end.flatten - expanded_sources << "application" if sources.include?(:defaults) && File.exist?(File.join(JAVASCRIPTS_DIR, "application.js")) - expanded_sources + class AssetCollection + extend ActiveSupport::Memoizable + + Cache = {} + CacheGuard = Mutex.new + + def self.create(template, controller, sources, recursive) + CacheGuard.synchronize do + key = [self, sources, recursive] + Cache[key] ||= new(template, controller, sources, recursive).freeze + end end - end - def expand_stylesheet_sources(sources, recursive) - if sources.first == :all - @@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) - end.flatten + def initialize(template, controller, sources, recursive) + # NOTE: The template arg is temporarily needed for a legacy plugin + # hook. See NOTE under AssetTag#initialize for more details + @template = template + @controller = controller + @sources = sources + @recursive = recursive end - end - def determine_source(source, collection) - case source - when Symbol - collection[source] || raise(ArgumentError, "No expansion found for #{source.inspect}") - else - source + def write_asset_file_contents(joined_asset_path) + FileUtils.mkdir_p(File.dirname(joined_asset_path)) + File.open(joined_asset_path, "w+") { |cache| cache.write(joined_contents) } + mt = latest_mtime + File.utime(mt, mt, joined_asset_path) end - end - def join_asset_file_contents(paths) - paths.collect { |path| File.read(asset_file_path(path)) }.join("\n\n") - end + private + def determine_source(source, collection) + case source + when Symbol + collection[source] || raise(ArgumentError, "No expansion found for #{source.inspect}") + else + source + end + end + + def validate_sources! + @sources.collect { |source| determine_source(source, self.class.expansions) }.flatten + end + + def all_asset_files + path = [public_directory, ('**' if @recursive), "*.#{extension}"].compact + Dir[File.join(*path)].collect { |file| + file[-(file.size - public_directory.size - 1)..-1].sub(/\.\w+$/, '') + }.sort + end + + def tag_sources + expand_sources.collect { |source| tag_class.create(@template, @controller, source, false) } + end - def write_asset_file_contents(joined_asset_path, asset_paths) - FileUtils.mkdir_p(File.dirname(joined_asset_path)) - File.open(joined_asset_path, "w+") { |cache| cache.write(join_asset_file_contents(asset_paths)) } + def joined_contents + tag_sources.collect { |source| source.contents }.join("\n\n") + end - # Set mtime to the latest of the combined files to allow for - # consistent ETag without a shared filesystem. - mt = asset_paths.map { |p| File.mtime(asset_file_path(p)) }.max - File.utime(mt, mt, joined_asset_path) + # Set mtime to the latest of the combined files to allow for + # consistent ETag without a shared filesystem. + def latest_mtime + tag_sources.map { |source| source.mtime }.max + end end - def asset_file_path(path) - File.join(ASSETS_DIR, path.split('?').first) + class JavaScriptSources < AssetCollection + include JavaScriptAsset + + EXPANSIONS = { :defaults => JAVASCRIPT_DEFAULT_SOURCES.dup } + + def self.expansions + EXPANSIONS + end + + APPLICATION_JS = "application".freeze + APPLICATION_FILE = "application.js".freeze + + def expand_sources + if @sources.include?(:all) + assets = all_asset_files + ((defaults.dup & assets) + assets).uniq! + else + expanded_sources = validate_sources! + expanded_sources << APPLICATION_JS if include_application? + expanded_sources + end + end + memoize :expand_sources + + private + def tag_class + JavaScriptTag + end + + def defaults + determine_source(:defaults, self.class.expansions) + end + + def include_application? + @sources.include?(:defaults) && File.exist?(File.join(JAVASCRIPTS_DIR, APPLICATION_FILE)) + end end - def collect_asset_files(*path) - dir = path.first + class StylesheetSources < AssetCollection + include StylesheetAsset + + EXPANSIONS = {} + + def self.expansions + EXPANSIONS + end - Dir[File.join(*path.compact)].collect do |file| - file[-(file.size - dir.size - 1)..-1].sub(/\.\w+$/, '') - end.sort + def expand_sources + @sources.first == :all ? all_asset_files : validate_sources! + end + memoize :expand_sources + + private + def tag_class + StylesheetTag + end end end end actionpack/lib/action_view/helpers/asset_tag_helper.rb @@ -38,7 +38,8 @@ class AssetTagHelperTest < ActionView::TestCase @controller.request = @request ActionView::Helpers::AssetTagHelper::reset_javascript_include_default - COMPUTED_PUBLIC_PATHS.clear + AssetTag::Cache.clear + AssetCollection::Cache.clear end def teardown @@ -155,12 +156,12 @@ class AssetTagHelperTest < ActionView::TestCase PathToJavascriptToTag.each { |method, tag| assert_dom_equal(tag, eval(method)) } end - def test_javascript_include_tag + def test_javascript_include_tag_with_blank_asset_id ENV["RAILS_ASSET_ID"] = "" JavascriptIncludeToTag.each { |method, tag| assert_dom_equal(tag, eval(method)) } + end - COMPUTED_PUBLIC_PATHS.clear - + def test_javascript_include_tag_with_given_asset_id ENV["RAILS_ASSET_ID"] = "1" assert_dom_equal(%(<script src="/javascripts/prototype.js?1" type="text/javascript"></script>\n<script src="/javascripts/effects.js?1" type="text/javascript"></script>\n<script src="/javascripts/dragdrop.js?1" type="text/javascript"></script>\n<script src="/javascripts/controls.js?1" type="text/javascript"></script>\n<script src="/javascripts/application.js?1" type="text/javascript"></script>), javascript_include_tag(:defaults)) end @@ -169,6 +170,11 @@ class AssetTagHelperTest < ActionView::TestCase ENV["RAILS_ASSET_ID"] = "" ActionView::Helpers::AssetTagHelper::register_javascript_include_default 'slider' assert_dom_equal %(<script src="/javascripts/prototype.js" type="text/javascript"></script>\n<script src="/javascripts/effects.js" type="text/javascript"></script>\n<script src="/javascripts/dragdrop.js" type="text/javascript"></script>\n<script src="/javascripts/controls.js" type="text/javascript"></script>\n<script src="/javascripts/slider.js" type="text/javascript"></script>\n<script src="/javascripts/application.js" type="text/javascript"></script>), javascript_include_tag(:defaults) + end + + def test_register_javascript_include_default_mixed_defaults + ENV["RAILS_ASSET_ID"] = "" + ActionView::Helpers::AssetTagHelper::register_javascript_include_default 'slider' ActionView::Helpers::AssetTagHelper::register_javascript_include_default 'lib1', '/elsewhere/blub/lib2' assert_dom_equal %(<script src="/javascripts/prototype.js" type="text/javascript"></script>\n<script src="/javascripts/effects.js" type="text/javascript"></script>\n<script src="/javascripts/dragdrop.js" type="text/javascript"></script>\n<script src="/javascripts/controls.js" type="text/javascript"></script>\n<script src="/javascripts/slider.js" type="text/javascript"></script>\n<script src="/javascripts/lib1.js" type="text/javascript"></script>\n<script src="/elsewhere/blub/lib2.js" type="text/javascript"></script>\n<script src="/javascripts/application.js" type="text/javascript"></script>), javascript_include_tag(:defaults) end @@ -386,6 +392,31 @@ class AssetTagHelperTest < ActionView::TestCase FileUtils.rm_f(File.join(ActionView::Helpers::AssetTagHelper::JAVASCRIPTS_DIR, 'combined.js')) end + def test_caching_javascript_include_tag_with_relative_url_root + ENV["RAILS_ASSET_ID"] = "" + ActionController::Base.relative_url_root = "/collaboration/hieraki" + ActionController::Base.perform_caching = true + + assert_dom_equal( + %(<script src="/collaboration/hieraki/javascripts/all.js" type="text/javascript"></script>), + javascript_include_tag(:all, :cache => true) + ) + + assert File.exist?(File.join(ActionView::Helpers::AssetTagHelper::JAVASCRIPTS_DIR, 'all.js')) + + assert_dom_equal( + %(<script src="/collaboration/hieraki/javascripts/money.js" type="text/javascript"></script>), + javascript_include_tag(:all, :cache => "money") + ) + + assert File.exist?(File.join(ActionView::Helpers::AssetTagHelper::JAVASCRIPTS_DIR, 'money.js')) + + ensure + ActionController::Base.relative_url_root = nil + FileUtils.rm_f(File.join(ActionView::Helpers::AssetTagHelper::JAVASCRIPTS_DIR, 'all.js')) + FileUtils.rm_f(File.join(ActionView::Helpers::AssetTagHelper::JAVASCRIPTS_DIR, 'money.js')) + end + def test_caching_javascript_include_tag_when_caching_off ENV["RAILS_ASSET_ID"] = "" ActionController::Base.perform_caching = false @@ -456,6 +487,31 @@ class AssetTagHelperTest < ActionView::TestCase FileUtils.rm_f(File.join(ActionView::Helpers::AssetTagHelper::STYLESHEETS_DIR, 'styles.css')) end + def test_caching_stylesheet_link_tag_with_relative_url_root + ENV["RAILS_ASSET_ID"] = "" + ActionController::Base.relative_url_root = "/collaboration/hieraki" + ActionController::Base.perform_caching = true + + assert_dom_equal( + %(<link href="/collaboration/hieraki/stylesheets/all.css" media="screen" rel="stylesheet" type="text/css" />), + stylesheet_link_tag(:all, :cache => true) + ) + + expected = Dir["#{ActionView::Helpers::AssetTagHelper::STYLESHEETS_DIR}/*.css"].map { |p| File.mtime(p) }.max + assert_equal expected, File.mtime(File.join(ActionView::Helpers::AssetTagHelper::STYLESHEETS_DIR, 'all.css')) + + assert_dom_equal( + %(<link href="/collaboration/hieraki/stylesheets/money.css" media="screen" rel="stylesheet" type="text/css" />), + stylesheet_link_tag(:all, :cache => "money") + ) + + assert File.exist?(File.join(ActionView::Helpers::AssetTagHelper::STYLESHEETS_DIR, 'money.css')) + ensure + ActionController::Base.relative_url_root = nil + FileUtils.rm_f(File.join(ActionView::Helpers::AssetTagHelper::STYLESHEETS_DIR, 'all.css')) + FileUtils.rm_f(File.join(ActionView::Helpers::AssetTagHelper::STYLESHEETS_DIR, 'money.css')) + end + def test_caching_stylesheet_include_tag_when_caching_off ENV["RAILS_ASSET_ID"] = "" ActionController::Base.perform_caching = false actionpack/test/template/asset_tag_helper_test.rb @@ -1,5 +1,7 @@ *Edge* +* Add Model#delete instance method, similar to Model.delete class method. #1086 [Hongli Lai] + * MySQL: cope with quirky default values for not-null text columns. #1043 [Frederick Cheung] * Multiparameter attributes skip time zone conversion for time-only columns [#1030 state:resolved] [Geoff Buesing] activerecord/CHANGELOG @@ -1470,7 +1470,7 @@ module ActiveRecord method_name = "has_one_dependent_delete_for_#{reflection.name}".to_sym define_method(method_name) do association = send(reflection.name) - association.class.delete(association.id) unless association.nil? + association.delete unless association.nil? end before_destroy method_name when :nullify @@ -1500,7 +1500,7 @@ module ActiveRecord method_name = "belongs_to_dependent_delete_for_#{reflection.name}".to_sym define_method(method_name) do association = send(reflection.name) - association.class.delete(association.id) unless association.nil? + association.delete unless association.nil? end before_destroy method_name else activerecord/lib/active_record/associations.rb @@ -63,7 +63,7 @@ module ActiveRecord # Fetches the first one using SQL if possible. def first(*args) - if fetch_first_or_last_using_find? args + if fetch_first_or_last_using_find?(args) find(:first, *args) else load_target unless loaded? @@ -73,7 +73,7 @@ module ActiveRecord # Fetches the last one using SQL if possible. def last(*args) - if fetch_first_or_last_using_find? args + if fetch_first_or_last_using_find?(args) find(:last, *args) else load_target unless loaded? @@ -420,7 +420,8 @@ module ActiveRecord end def fetch_first_or_last_using_find?(args) - args.first.kind_of?(Hash) || !(loaded? || @owner.new_record? || @reflection.options[:finder_sql] || !@target.blank? || args.first.kind_of?(Integer)) + args.first.kind_of?(Hash) || !(loaded? || @owner.new_record? || @reflection.options[:finder_sql] || + @target.any? { |record| record.new_record? } || args.first.kind_of?(Integer)) end end end activerecord/lib/active_record/associations/association_collection.rb @@ -2306,6 +2306,16 @@ module ActiveRecord #:nodoc: # Deletes the record in the database and freezes this instance to reflect that no changes should # be made (since they can't be persisted). + # + # Unlike #destroy, this method doesn't run any +before_delete+ and +after_delete+ + # callbacks, nor will it enforce any association +:dependent+ rules. + def delete + self.class.delete(id) unless new_record? + freeze + end + + # Deletes the record in the database and freezes this instance to reflect that no changes should + # be made (since they can't be persisted). def destroy unless new_record? connection.delete <<-end_sql, "#{self.class.name} Destroy" activerecord/lib/active_record/base.rb @@ -1,7 +1,7 @@ module ActiveRecord module VERSION #:nodoc: MAJOR = 2 - MINOR = 1 + MINOR = 2 TINY = 0 STRING = [MAJOR, MINOR, TINY].join('.') activerecord/lib/active_record/version.rb @@ -253,7 +253,7 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase assert !devel.projects.loaded? assert_equal devel.projects.last, proj - assert devel.projects.loaded? + assert !devel.projects.loaded? assert !proj.new_record? assert_equal Developer.find(1).projects.sort_by(&:id).last, proj # prove join table is updated activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb @@ -1009,6 +1009,19 @@ class HasManyAssociationsTest < ActiveRecord::TestCase assert firm.clients.loaded? end + def test_calling_first_or_last_on_existing_record_with_create_should_not_load_association + firm = companies(:first_firm) + firm.clients.create(:name => 'Foo') + assert !firm.clients.loaded? + + assert_queries 2 do + firm.clients.first + firm.clients.last + end + + assert !firm.clients.loaded? + end + def test_calling_first_or_last_on_new_record_should_not_run_queries firm = Firm.new activerecord/test/cases/associations/has_many_associations_test.rb @@ -472,6 +472,18 @@ class BasicsTest < ActiveRecord::TestCase assert topic.instance_variable_get("@custom_approved") end + def test_delete + topic = Topic.find(1) + assert_equal topic, topic.delete, 'topic.delete did not return self' + assert topic.frozen?, 'topic not frozen after delete' + assert_raise(ActiveRecord::RecordNotFound) { Topic.find(topic.id) } + end + + def test_delete_doesnt_run_callbacks + Topic.find(1).delete + assert_not_nil Topic.find(2) + end + def test_destroy topic = Topic.find(1) assert_equal topic, topic.destroy, 'topic.destroy did not return self' @@ -820,6 +832,20 @@ class BasicsTest < ActiveRecord::TestCase assert_equal [ Topic.find(1) ], [ Topic.find(2).topic ] & [ Topic.find(1) ] end + def test_delete_new_record + client = Client.new + client.delete + assert client.frozen? + end + + def test_delete_record_with_associations + client = Client.find(3) + client.delete + assert client.frozen? + assert_kind_of Firm, client.firm + assert_raises(ActiveSupport::FrozenObjectError) { client.name = "something else" } + end + def test_destroy_new_record client = Client.new client.destroy activerecord/test/cases/base_test.rb @@ -1,7 +1,7 @@ module ActiveSupport module VERSION #:nodoc: MAJOR = 2 - MINOR = 1 + MINOR = 2 TINY = 0 STRING = [MAJOR, MINOR, TINY].join('.') activesupport/lib/active_support/version.rb @@ -1,7 +1,7 @@ module Rails module VERSION #:nodoc: MAJOR = 2 - MINOR = 1 + MINOR = 2 TINY = 0 STRING = [MAJOR, MINOR, TINY].join('.') railties/lib/rails/version.rb 5795c509a7c0ab9c6d3d707f34526430e58e535c 5f86451a4c5d0beca5a746c4708be48b13f665be Michael Koziarski michael@koziarski.com http://github.com/rails/rails/commit/638bd19c7fdf4a4c09bfa5b4ada23c6f37724f9b 638bd19c7fdf4a4c09bfa5b4ada23c6f37724f9b 2008-09-22T12:35:35-07:00 2008-09-22T12:35:35-07:00 Merge branch 'patches' into multibyte c27414d94e86b396fe716e031a6fafb0fc8ca575 Michael Koziarski michael@koziarski.com