diff options
31 files changed, 443 insertions, 143 deletions
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 0545d36a0a..6d62d5eeec 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,5 +1,17 @@ ## Rails 3.2.0 (unreleased) ## +* form_for with +:as+ option uses "#{action}_#{as}" as css class and id: + + Before: + + form_for(@user, :as => 'client') # => "<form class="client_new">..." + + Now: + + form_for(@user, :as => 'client') # => "<form class="new_client">..." + + *Vasiliy Ermolovich* + * Allow rescue responses to be configured through a railtie as in `config.action_dispatch.rescue_responses`. Please look at ActiveRecord::Railtie for an example *José Valim* * Allow fresh_when/stale? to take a record instead of an options hash *DHH* @@ -84,6 +96,14 @@ persistent between requests so if you need to manipulate the environment for your test you need to do it before the cookie jar is created. +## Rails 3.1.4 (unreleased) ## + +* Allow to use asset_path on named_routes aliasing RailsHelper's + asset_path to path_to_asset *Adrian Pike* + +* Assets should use the request protocol by default or default to + relative if no request is available *Jonathan del Strother* + ## Rails 3.1.3 (unreleased) ## * Fix using `tranlate` helper with a html translation which uses the `:count` option for diff --git a/actionpack/lib/action_controller/caching/actions.rb b/actionpack/lib/action_controller/caching/actions.rb index f988de39dd..3b86a9a93a 100644 --- a/actionpack/lib/action_controller/caching/actions.rb +++ b/actionpack/lib/action_controller/caching/actions.rb @@ -116,9 +116,8 @@ module ActionController #:nodoc: def expire_action(options = {}) return unless cache_configured? - actions = options[:action] - if actions.is_a?(Array) - actions.each {|action| expire_action(options.merge(:action => action)) } + if options.is_a?(Hash) && options[:action].is_a?(Array) + options[:action].each {|action| expire_action(options.merge(:action => action)) } else expire_fragment(ActionCachePath.new(self, options, false).path) end diff --git a/actionpack/lib/action_controller/metal/params_wrapper.rb b/actionpack/lib/action_controller/metal/params_wrapper.rb index e0d8e1c992..9dcea86253 100644 --- a/actionpack/lib/action_controller/metal/params_wrapper.rb +++ b/actionpack/lib/action_controller/metal/params_wrapper.rb @@ -141,7 +141,7 @@ module ActionController # try to find Foo::Bar::User, Foo::User and finally User. def _default_wrap_model #:nodoc: return nil if self.anonymous? - model_name = self.name.sub(/Controller$/, '').singularize + model_name = self.name.sub(/Controller$/, '').classify begin if model_klass = model_name.safe_constantize diff --git a/actionpack/lib/action_controller/metal/redirecting.rb b/actionpack/lib/action_controller/metal/redirecting.rb index 0355c9f458..b07742e0e1 100644 --- a/actionpack/lib/action_controller/metal/redirecting.rb +++ b/actionpack/lib/action_controller/metal/redirecting.rb @@ -18,7 +18,7 @@ module ActionController # # * <tt>Hash</tt> - The URL will be generated by calling url_for with the +options+. # * <tt>Record</tt> - The URL will be generated by calling url_for with the +options+, which will reference a named URL for that record. - # * <tt>String</tt> starting with <tt>protocol://</tt> (like <tt>http://</tt>) - Is passed straight through as the target for redirection. + # * <tt>String</tt> starting with <tt>protocol://</tt> (like <tt>http://</tt>) or a protocol relative reference (like <tt>//</tt>) - Is passed straight through as the target for redirection. # * <tt>String</tt> not containing a protocol - The current protocol and host is prepended to the string. # * <tt>Proc</tt> - A block that will be executed in the controller's context. Should return any option accepted by +redirect_to+. # * <tt>:back</tt> - Back to the page that issued the request. Useful for forms that are triggered from multiple places. @@ -81,7 +81,8 @@ module ActionController # The scheme name consist of a letter followed by any combination of # letters, digits, and the plus ("+"), period ("."), or hyphen ("-") # characters; and is terminated by a colon (":"). - when %r{^\w[\w+.-]*:.*} + # The protocol relative scheme starts with a double slash "//" + when %r{^(\w[\w+.-]*:|//).*} options when String request.protocol + request.host_with_port + options diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 2bcde16110..c64214431a 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -356,7 +356,7 @@ module ActionDispatch conditions = build_conditions(conditions, valid_conditions, path.names.map { |x| x.to_sym }) route = @set.add_route(app, path, conditions, defaults, name) - named_routes[name] = route if name + named_routes[name] = route if name && !named_routes[name] route end diff --git a/actionpack/lib/action_view/helpers/form_helper.rb b/actionpack/lib/action_view/helpers/form_helper.rb index e674e12509..1a64b12aa6 100644 --- a/actionpack/lib/action_view/helpers/form_helper.rb +++ b/actionpack/lib/action_view/helpers/form_helper.rb @@ -387,8 +387,8 @@ module ActionView as = options[:as] action, method = object.respond_to?(:persisted?) && object.persisted? ? [:edit, :put] : [:new, :post] options[:html].reverse_merge!( - :class => as ? "#{as}_#{action}" : dom_class(object, action), - :id => as ? "#{as}_#{action}" : [options[:namespace], dom_id(object, action)].compact.join("_").presence, + :class => as ? "#{action}_#{as}" : dom_class(object, action), + :id => as ? "#{action}_#{as}" : [options[:namespace], dom_id(object, action)].compact.join("_").presence, :method => method ) diff --git a/actionpack/lib/sprockets/helpers/rails_helper.rb b/actionpack/lib/sprockets/helpers/rails_helper.rb index 1ebe7f68f7..75ba1e66c2 100644 --- a/actionpack/lib/sprockets/helpers/rails_helper.rb +++ b/actionpack/lib/sprockets/helpers/rails_helper.rb @@ -26,10 +26,10 @@ module Sprockets sources.collect do |source| if debug && asset = asset_paths.asset_for(source, 'js') asset.to_a.map { |dep| - super(dep.to_s, { :src => asset_path(dep, :ext => 'js', :body => true, :digest => digest) }.merge!(options)) + super(dep.to_s, { :src => path_to_asset(dep, :ext => 'js', :body => true, :digest => digest) }.merge!(options)) } else - super(source.to_s, { :src => asset_path(source, :ext => 'js', :body => body, :digest => digest) }.merge!(options)) + super(source.to_s, { :src => path_to_asset(source, :ext => 'js', :body => body, :digest => digest) }.merge!(options)) end end.join("\n").html_safe end @@ -43,10 +43,10 @@ module Sprockets sources.collect do |source| if debug && asset = asset_paths.asset_for(source, 'css') asset.to_a.map { |dep| - super(dep.to_s, { :href => asset_path(dep, :ext => 'css', :body => true, :protocol => :request, :digest => digest) }.merge!(options)) + super(dep.to_s, { :href => path_to_asset(dep, :ext => 'css', :body => true, :protocol => :request, :digest => digest) }.merge!(options)) } else - super(source.to_s, { :href => asset_path(source, :ext => 'css', :body => body, :protocol => :request, :digest => digest) }.merge!(options)) + super(source.to_s, { :href => path_to_asset(source, :ext => 'css', :body => body, :protocol => :request, :digest => digest) }.merge!(options)) end end.join("\n").html_safe end @@ -56,19 +56,20 @@ module Sprockets path = asset_paths.compute_public_path(source, asset_prefix, options.merge(:body => true)) options[:body] ? "#{path}?body=1" : path end - + alias_method :path_to_asset, :asset_path # aliased to avoid conflicts with an asset_path named route + def image_path(source) - asset_path(source) + path_to_asset(source) end alias_method :path_to_image, :image_path # aliased to avoid conflicts with an image_path named route def javascript_path(source) - asset_path(source) + path_to_asset(source) end alias_method :path_to_javascript, :javascript_path # aliased to avoid conflicts with an javascript_path named route def stylesheet_path(source) - asset_path(source) + path_to_asset(source) end alias_method :path_to_stylesheet, :stylesheet_path # aliased to avoid conflicts with an stylesheet_path named route diff --git a/actionpack/test/controller/caching_test.rb b/actionpack/test/controller/caching_test.rb index 2364bbf3a3..015e6b9955 100644 --- a/actionpack/test/controller/caching_test.rb +++ b/actionpack/test/controller/caching_test.rb @@ -251,6 +251,11 @@ class ActionCachingTestController < CachingController expire_action :controller => 'action_caching_test', :action => 'index', :format => 'xml' render :nothing => true end + + def expire_with_url_string + expire_action url_for(:controller => 'action_caching_test', :action => 'index') + render :nothing => true + end end class MockTime < Time @@ -445,6 +450,21 @@ class ActionCacheTest < ActionController::TestCase assert_not_equal cached_time, @response.body end + def test_cache_expiration_with_url_string + get :index + cached_time = content_to_cache + reset! + + @request.request_uri = "/action_caching_test/expire_with_url_string" + get :expire_with_url_string + assert_response :success + reset! + + get :index + assert_response :success + assert_not_equal cached_time, @response.body + end + def test_cache_is_scoped_by_subdomain @request.host = 'jamis.hostname.com' get :index diff --git a/actionpack/test/controller/params_wrapper_test.rb b/actionpack/test/controller/params_wrapper_test.rb index 7bef1e8d5d..0102f66dfe 100644 --- a/actionpack/test/controller/params_wrapper_test.rb +++ b/actionpack/test/controller/params_wrapper_test.rb @@ -285,3 +285,38 @@ class AnonymousControllerParamsWrapperTest < ActionController::TestCase end end end + +class IrregularInflectionParamsWrapperTest < ActionController::TestCase + include ParamsWrapperTestHelp + + class ParamswrappernewsItem + def self.attribute_names + ['test_attr'] + end + end + + class ParamswrappernewsController < ActionController::Base + class << self + attr_accessor :last_parameters + end + + def parse + self.class.last_parameters = request.params.except(:controller, :action) + head :ok + end + end + + tests ParamswrappernewsController + + def test_uses_model_attribute_names_with_irregular_inflection + ActiveSupport::Inflector.inflections do |inflect| + inflect.irregular 'paramswrappernews_item', 'paramswrappernews' + end + + with_default_wrapper_options do + @request.env['CONTENT_TYPE'] = 'application/json' + post :parse, { 'username' => 'sikachu', 'test_attr' => 'test_value' } + assert_parameters({ 'username' => 'sikachu', 'test_attr' => 'test_value', 'paramswrappernews_item' => { 'test_attr' => 'test_value' }}) + end + end +end diff --git a/actionpack/test/controller/redirect_test.rb b/actionpack/test/controller/redirect_test.rb index 79041055bd..5b739e49ac 100644 --- a/actionpack/test/controller/redirect_test.rb +++ b/actionpack/test/controller/redirect_test.rb @@ -70,6 +70,10 @@ class RedirectController < ActionController::Base redirect_to "x-test+scheme.complex:redirect" end + def redirect_to_url_with_network_path_reference + redirect_to "//www.rubyonrails.org/" + end + def redirect_to_back redirect_to :back end @@ -216,6 +220,12 @@ class RedirectTest < ActionController::TestCase assert_equal "x-test+scheme.complex:redirect", redirect_to_url end + def test_redirect_to_url_with_network_path_reference + get :redirect_to_url_with_network_path_reference + assert_response :redirect + assert_equal "//www.rubyonrails.org/", redirect_to_url + end + def test_redirect_to_back @request.env["HTTP_REFERER"] = "http://www.example.com/coming/from" get :redirect_to_back diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb index 4a67380f59..062fc1f94e 100644 --- a/actionpack/test/controller/routing_test.rb +++ b/actionpack/test/controller/routing_test.rb @@ -713,12 +713,12 @@ class RouteSetTest < ActiveSupport::TestCase assert_equal set.routes.first, set.named_routes[:hello] end - def test_later_named_routes_take_precedence + def test_earlier_named_routes_take_precedence set.draw do match '/hello/world' => 'a#b', :as => 'hello' match '/hello' => 'a#b', :as => 'hello' end - assert_equal set.routes.last, set.named_routes[:hello] + assert_equal set.routes.first, set.named_routes[:hello] end def setup_named_route_test diff --git a/actionpack/test/template/form_helper_test.rb b/actionpack/test/template/form_helper_test.rb index 3fee366804..41e925b000 100644 --- a/actionpack/test/template/form_helper_test.rb +++ b/actionpack/test/template/form_helper_test.rb @@ -769,7 +769,7 @@ class FormHelperTest < ActionView::TestCase concat f.submit('Create post') end - expected = whole_form("/posts/123", "create-post", "other_name_edit", :method => "put") do + expected = whole_form("/posts/123", "create-post", "edit_other_name", :method => "put") do "<label for='other_name_title' class='post_title'>Title</label>" + "<input name='other_name[title]' size='30' id='other_name_title' value='Hello World' type='text' />" + "<textarea name='other_name[body]' id='other_name_body' rows='20' cols='40'>Back to the hill and over it again!</textarea>" + @@ -906,7 +906,7 @@ class FormHelperTest < ActionView::TestCase concat f.check_box(:secret) end - expected = whole_form('/posts/123', 'post[]_edit', 'post[]_edit', 'put') do + expected = whole_form('/posts/123', 'edit_post[]', 'edit_post[]', 'put') do "<label for='post_123_title'>Title</label>" + "<input name='post[123][title]' size='30' type='text' id='post_123_title' value='Hello World' />" + "<textarea name='post[123][body]' id='post_123_body' rows='20' cols='40'>Back to the hill and over it again!</textarea>" + @@ -924,7 +924,7 @@ class FormHelperTest < ActionView::TestCase concat f.check_box(:secret) end - expected = whole_form('/posts/123', 'post[]_edit', 'post[]_edit', 'put') do + expected = whole_form('/posts/123', 'edit_post[]', 'edit_post[]', 'put') do "<input name='post[][title]' size='30' type='text' id='post__title' value='Hello World' />" + "<textarea name='post[][body]' id='post__body' rows='20' cols='40'>Back to the hill and over it again!</textarea>" + "<input name='post[][secret]' type='hidden' value='0' />" + @@ -1066,7 +1066,7 @@ class FormHelperTest < ActionView::TestCase concat f.submit end - expected = whole_form('/posts/123', 'another_post_edit', 'another_post_edit', :method => 'put') do + expected = whole_form('/posts/123', 'edit_another_post', 'edit_another_post', :method => 'put') do "<input name='commit' type='submit' value='Update your Post' />" end @@ -1098,7 +1098,7 @@ class FormHelperTest < ActionView::TestCase } end - expected = whole_form('/posts/123', 'post[]_edit', 'post[]_edit', 'put') do + expected = whole_form('/posts/123', 'edit_post[]', 'edit_post[]', 'put') do "<input name='post[123][title]' size='30' type='text' id='post_123_title' value='Hello World' />" + "<input name='post[123][comment][][name]' size='30' type='text' id='post_123_comment__name' value='new comment' />" end @@ -1157,7 +1157,7 @@ class FormHelperTest < ActionView::TestCase } end - expected = whole_form('/posts/123', 'post[]_edit', 'post[]_edit', 'put') do + expected = whole_form('/posts/123', 'edit_post[]', 'edit_post[]', 'put') do "<input name='post[123][comment][title]' size='30' type='text' id='post_123_comment_title' value='Hello World' />" end @@ -1185,7 +1185,7 @@ class FormHelperTest < ActionView::TestCase } end - expected = whole_form('/posts/123', 'post[]_edit', 'post[]_edit', 'put') do + expected = whole_form('/posts/123', 'edit_post[]', 'edit_post[]', 'put') do "<input name='post[123][comment][123][title]' size='30' type='text' id='post_123_comment_123_title' value='Hello World' />" end @@ -1205,9 +1205,9 @@ class FormHelperTest < ActionView::TestCase } end - expected = whole_form('/posts/123', 'post[]_edit', 'post[]_edit', 'put') do + expected = whole_form('/posts/123', 'edit_post[]', 'edit_post[]', 'put') do "<input name='post[123][comment][5][title]' size='30' type='text' id='post_123_comment_5_title' value='Hello World' />" - end + whole_form('/posts/123', 'post_edit', 'post_edit', 'put') do + end + whole_form('/posts/123', 'edit_post', 'edit_post', 'put') do "<input name='post[1][comment][123][title]' size='30' type='text' id='post_1_comment_123_title' value='Hello World' />" end @@ -1860,7 +1860,7 @@ class FormHelperTest < ActionView::TestCase } end - expected = whole_form('/posts/123', 'create-post', 'post_edit', :method => 'put') do + expected = whole_form('/posts/123', 'create-post', 'edit_post', :method => 'put') do "<input name='post[title]' size='30' type='text' id='post_title' value='Hello World' />" + "<textarea name='post[body]' id='post_body' rows='20' cols='40'>Back to the hill and over it again!</textarea>" + "<input name='parent_post[secret]' type='hidden' value='0' />" + @@ -1880,7 +1880,7 @@ class FormHelperTest < ActionView::TestCase } end - expected = whole_form('/posts/123', 'create-post', 'post_edit', :method => 'put') do + expected = whole_form('/posts/123', 'create-post', 'edit_post', :method => 'put') do "<input name='post[title]' size='30' type='text' id='post_title' value='Hello World' />" + "<textarea name='post[body]' id='post_body' rows='20' cols='40'>Back to the hill and over it again!</textarea>" + "<input name='post[comment][name]' type='text' id='post_comment_name' value='new comment' size='30' />" diff --git a/actionpack/test/template/sprockets_helper_with_routes_test.rb b/actionpack/test/template/sprockets_helper_with_routes_test.rb new file mode 100644 index 0000000000..bcbd81a7dd --- /dev/null +++ b/actionpack/test/template/sprockets_helper_with_routes_test.rb @@ -0,0 +1,57 @@ +require 'abstract_unit' +require 'sprockets' +require 'sprockets/helpers/rails_helper' +require 'mocha' + +class SprocketsHelperWithRoutesTest < ActionView::TestCase + include Sprockets::Helpers::RailsHelper + + # Let's bring in some named routes to test namespace conflicts with potential *_paths. + # We have to do this after we bring in the Sprockets RailsHelper so if there are conflicts, + # they'll fail in the way we expect in a real live Rails app. + routes = ActionDispatch::Routing::RouteSet.new + routes.draw do + resources :assets + end + include routes.url_helpers + + def setup + super + @controller = BasicController.new + + @assets = Sprockets::Environment.new + @assets.append_path(FIXTURES.join("sprockets/app/javascripts")) + @assets.append_path(FIXTURES.join("sprockets/app/stylesheets")) + @assets.append_path(FIXTURES.join("sprockets/app/images")) + + application = Struct.new(:config, :assets).new(config, @assets) + Rails.stubs(:application).returns(application) + @config = config + @config.perform_caching = true + @config.assets.digest = true + @config.assets.compile = true + end + + test "namespace conflicts on a named route called asset_path" do + # Testing this for sanity - asset_path is now a named route! + assert_match asset_path('test_asset'), '/assets/test_asset' + + assert_match %r{/assets/logo-[0-9a-f]+.png}, + path_to_asset("logo.png") + assert_match %r{/assets/logo-[0-9a-f]+.png}, + path_to_asset("logo.png", :digest => true) + assert_match %r{/assets/logo.png}, + path_to_asset("logo.png", :digest => false) + end + + test "javascript_include_tag with a named_route named asset_path" do + assert_match %r{<script src="/assets/application-[0-9a-f]+.js" type="text/javascript"></script>}, + javascript_include_tag(:application) + end + + test "stylesheet_link_tag with a named_route named asset_path" do + assert_match %r{<link href="/assets/application-[0-9a-f]+.css" media="screen" rel="stylesheet" type="text/css" />}, + stylesheet_link_tag(:application) + end + +end
\ No newline at end of file diff --git a/activemodel/lib/active_model/naming.rb b/activemodel/lib/active_model/naming.rb index 953d24a3b2..3caa61e41b 100644 --- a/activemodel/lib/active_model/naming.rb +++ b/activemodel/lib/active_model/naming.rb @@ -71,8 +71,8 @@ module ActiveModel # BookCover.model_name # => "BookCover" # BookCover.model_name.human # => "Book cover" # - # BookCover.model_name.i18n_key # => "book_cover" - # BookModule::BookCover.model_name.i18n_key # => "book_module.book_cover" + # BookCover.model_name.i18n_key # => :book_cover + # BookModule::BookCover.model_name.i18n_key # => :"book_module/book_cover" # # Providing the functionality that ActiveModel::Naming provides in your object # is required to pass the Active Model Lint test. So either extending the provided diff --git a/activemodel/lib/active_model/translation.rb b/activemodel/lib/active_model/translation.rb index 6d64c81b5f..02b7c54d61 100644 --- a/activemodel/lib/active_model/translation.rb +++ b/activemodel/lib/active_model/translation.rb @@ -43,13 +43,25 @@ module ActiveModel # # Specify +options+ with additional translating options. def human_attribute_name(attribute, options = {}) - defaults = lookup_ancestors.map do |klass| - :"#{self.i18n_scope}.attributes.#{klass.model_name.i18n_key}.#{attribute}" + defaults = [] + parts = attribute.to_s.split(".", 2) + attribute = parts.pop + namespace = parts.pop + + if namespace + lookup_ancestors.each do |klass| + defaults << :"#{self.i18n_scope}.attributes.#{klass.model_name.i18n_key}/#{namespace}.#{attribute}" + end + defaults << :"#{self.i18n_scope}.attributes.#{namespace}.#{attribute}" + else + lookup_ancestors.each do |klass| + defaults << :"#{self.i18n_scope}.attributes.#{klass.model_name.i18n_key}.#{attribute}" + end end defaults << :"attributes.#{attribute}" defaults << options.delete(:default) if options[:default] - defaults << attribute.to_s.humanize + defaults << attribute.humanize options.reverse_merge! :count => 1, :default => defaults I18n.translate(defaults.shift, options) diff --git a/activemodel/lib/active_model/validations/length.rb b/activemodel/lib/active_model/validations/length.rb index 6bc928bab7..a38de27b3c 100644 --- a/activemodel/lib/active_model/validations/length.rb +++ b/activemodel/lib/active_model/validations/length.rb @@ -1,3 +1,5 @@ +require "active_support/core_ext/string/encoding" + module ActiveModel # == Active Model Length Validator @@ -6,7 +8,6 @@ module ActiveModel MESSAGES = { :is => :wrong_length, :minimum => :too_short, :maximum => :too_long }.freeze CHECKS = { :is => :==, :minimum => :>=, :maximum => :<= }.freeze - DEFAULT_TOKENIZER = lambda { |value| value.split(//) } RESERVED_OPTIONS = [:minimum, :maximum, :within, :is, :tokenizer, :too_short, :too_long] def initialize(options) @@ -36,14 +37,11 @@ module ActiveModel end def validate_each(record, attribute, value) - value = (options[:tokenizer] || DEFAULT_TOKENIZER).call(value) if value.kind_of?(String) + value = tokenize(value) + value_length = value.respond_to?(:length) ? value.length : value.to_s.length CHECKS.each do |key, validity_check| next unless check_value = options[key] - - value ||= [] if key == :maximum - - value_length = value.respond_to?(:length) ? value.length : value.to_s.length next if value_length.send(validity_check, check_value) errors_options = options.except(*RESERVED_OPTIONS) @@ -55,6 +53,18 @@ module ActiveModel record.errors.add(attribute, MESSAGES[key], errors_options) end end + + private + + def tokenize(value) + if value.kind_of?(String) + if options[:tokenizer] + options[:tokenizer].call(value) + elsif !value.encoding_aware? + value.mb_chars + end + end || value + end end module HelperMethods @@ -96,7 +106,7 @@ module ActiveModel # * <tt>:tokenizer</tt> - Specifies how to split up the attribute string. (e.g. <tt>:tokenizer => lambda {|str| str.scan(/\w+/)}</tt> to # count words as in above example.) # Defaults to <tt>lambda{ |value| value.split(//) }</tt> which counts individual characters. - # * <tt>:strict</tt> - Specifies whether validation should be strict. + # * <tt>:strict</tt> - Specifies whether validation should be strict. # See <tt>ActiveModel::Validation#validates!</tt> for more information def validates_length_of(*attr_names) validates_with LengthValidator, _merge_attributes(attr_names) diff --git a/activemodel/test/cases/naming_test.rb b/activemodel/test/cases/naming_test.rb index e8db73ba52..b2976ab1fb 100644 --- a/activemodel/test/cases/naming_test.rb +++ b/activemodel/test/cases/naming_test.rb @@ -34,6 +34,10 @@ class NamingTest < ActiveModel::TestCase def test_human assert_equal 'Track back', @model_name.human end + + def test_i18n_key + assert_equal :"post/track_back", @model_name.i18n_key + end end class NamingWithNamespacedModelInIsolatedNamespaceTest < ActiveModel::TestCase @@ -74,6 +78,10 @@ class NamingWithNamespacedModelInIsolatedNamespaceTest < ActiveModel::TestCase def test_param_key assert_equal 'post', @model_name.param_key end + + def test_i18n_key + assert_equal :"blog/post", @model_name.i18n_key + end end class NamingWithNamespacedModelInSharedNamespaceTest < ActiveModel::TestCase @@ -114,6 +122,10 @@ class NamingWithNamespacedModelInSharedNamespaceTest < ActiveModel::TestCase def test_param_key assert_equal 'blog_post', @model_name.param_key end + + def test_i18n_key + assert_equal :"blog/post", @model_name.i18n_key + end end class NamingWithSuppliedModelNameTest < ActiveModel::TestCase @@ -154,6 +166,10 @@ class NamingWithSuppliedModelNameTest < ActiveModel::TestCase def test_param_key assert_equal 'article', @model_name.param_key end + + def test_i18n_key + assert_equal :"article", @model_name.i18n_key + end end class NamingUsingRelativeModelNameTest < ActiveModel::TestCase @@ -188,6 +204,10 @@ class NamingUsingRelativeModelNameTest < ActiveModel::TestCase def test_param_key assert_equal 'post', @model_name.param_key end + + def test_i18n_key + assert_equal :"blog/post", @model_name.i18n_key + end end class NamingHelpersTest < Test::Unit::TestCase diff --git a/activemodel/test/cases/translation_test.rb b/activemodel/test/cases/translation_test.rb index 1b1d972d5c..54e86d48db 100644 --- a/activemodel/test/cases/translation_test.rb +++ b/activemodel/test/cases/translation_test.rb @@ -56,6 +56,16 @@ class ActiveModelI18nTests < ActiveModel::TestCase assert_equal 'person gender attribute', Person::Gender.human_attribute_name('attribute') end + def test_translated_nested_model_attributes + I18n.backend.store_translations 'en', :activemodel => {:attributes => {:"person/addresses" => {:street => 'Person Address Street'}}} + assert_equal 'Person Address Street', Person.human_attribute_name('addresses.street') + end + + def test_translated_nested_model_attributes_with_namespace_fallback + I18n.backend.store_translations 'en', :activemodel => {:attributes => {:addresses => {:street => 'Cool Address Street'}}} + assert_equal 'Cool Address Street', Person.human_attribute_name('addresses.street') + end + def test_translated_model_names I18n.backend.store_translations 'en', :activemodel => {:models => {:person => 'person model'} } assert_equal 'person model', Person.model_name.human diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 9bc0023539..7ba585b8e0 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -27,6 +27,7 @@ require 'active_support/deprecation' require 'arel' require 'active_record/errors' require 'active_record/log_subscriber' +require 'active_record/explain_subscriber' module ActiveRecord #:nodoc: # = Active Record @@ -721,10 +722,10 @@ module ActiveRecord #:nodoc: end def sequence_name - if superclass == Base + if base_class == self @sequence_name ||= reset_sequence_name else - (@sequence_name ||= nil) || superclass.sequence_name + (@sequence_name ||= nil) || base_class.sequence_name end end diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb index ce4c5a1383..3ced32e42d 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -23,7 +23,12 @@ module ActiveRecord # === Example # table_exists?(:developers) def table_exists?(table_name) - tables.include?(table_name.to_s) + begin + select_value("SELECT 1 FROM #{quote_table_name(table_name)} where 1=0") + true + rescue + false + end end # Returns an array of indexes for the given table. diff --git a/activerecord/lib/active_record/explain.rb b/activerecord/lib/active_record/explain.rb index abe6fff5d5..92b38d1b70 100644 --- a/activerecord/lib/active_record/explain.rb +++ b/activerecord/lib/active_record/explain.rb @@ -1,68 +1,48 @@ module ActiveRecord module Explain - # logging_query_plan calls could appear nested in the call stack. In - # particular this happens when a relation fetches its records, since - # that results in find_by_sql calls downwards. - # - # This flag allows nested calls to detect this situation and bypass - # it, thus preventing repeated EXPLAINs. - LOGGING_QUERY_PLAN = :logging_query_plan - # If auto explain is enabled, this method triggers EXPLAIN logging for the # queries triggered by the block if it takes more than the threshold as a # whole. That is, the threshold is not checked against each individual # query, but against the duration of the entire block. This approach is # convenient for relations. - def logging_query_plan(&block) # :nodoc: + # + # The available_queries_for_explain thread variable collects the queries + # to be explained. If the value is nil, it means queries are not being + # currently collected. A false value indicates collecting is turned + # off. Otherwise it is an array of queries. + def logging_query_plan # :nodoc: threshold = auto_explain_threshold_in_seconds - if threshold && !Thread.current[LOGGING_QUERY_PLAN] && !Thread.current[SILENCED] + current = Thread.current + if threshold && current[:available_queries_for_explain].nil? begin - Thread.current[LOGGING_QUERY_PLAN] = true + queries = current[:available_queries_for_explain] = [] start = Time.now - result, sqls, binds = collecting_sqls_for_explain(&block) - logger.warn(exec_explain(sqls, binds)) if Time.now - start > threshold + result = yield + logger.warn(exec_explain(queries)) if Time.now - start > threshold result ensure - Thread.current[LOGGING_QUERY_PLAN] = false + current[:available_queries_for_explain] = nil end else yield end end - # SCHEMA queries cannot be EXPLAINed, also we do not want to run EXPLAIN on - # our own EXPLAINs now matter how loopingly beautiful that would be. - SKIP_EXPLAIN_FOR = %w(SCHEMA EXPLAIN) - def ignore_explain_notification?(payload) # :nodoc: - payload[:exception] || SKIP_EXPLAIN_FOR.include?(payload[:name]) - end - - # Collects all queries executed while the passed block runs. Returns an - # array with three elements, the result of the block, the strings with the - # queries, and their respective bindings. - def collecting_sqls_for_explain # :nodoc: - sqls = [] - binds = [] - callback = lambda do |*args| - payload = args.last - unless ignore_explain_notification?(payload) - sqls << payload[:sql] - binds << payload[:binds] - end - end - - result = nil - ActiveSupport::Notifications.subscribed(callback, "sql.active_record") do - result = yield - end - - [result, sqls, binds] + # Relation#explain needs to be able to collect the queries regardless of + # whether auto explain is enabled. This method serves that purpose. + def collecting_queries_for_explain # :nodoc: + current = Thread.current + original, current[:available_queries_for_explain] = current[:available_queries_for_explain], [] + return yield, current[:available_queries_for_explain] + ensure + # Note that the return value above does not depend on this assigment. + current[:available_queries_for_explain] = original end - # Makes the adapter execute EXPLAIN for the given queries and bindings. + # Makes the adapter execute EXPLAIN for the tuples of queries and bindings. # Returns a formatted string ready to be logged. - def exec_explain(sqls, binds) # :nodoc: - sqls.zip(binds).map do |sql, bind| + def exec_explain(queries) # :nodoc: + queries && queries.map do |sql, bind| [].tap do |msg| msg << "EXPLAIN for: #{sql}" unless bind.empty? @@ -74,8 +54,6 @@ module ActiveRecord end.join("\n") end - SILENCED = :silence_explain - # Silences automatic EXPLAIN logging for the duration of the block. # # This has high priority, no EXPLAINs will be run even if downwards @@ -84,13 +62,11 @@ module ActiveRecord # As the name of the method suggests this only applies to automatic # EXPLAINs, manual calls to +ActiveRecord::Relation#explain+ run. def silence_auto_explain - # Implemented as a flag rather that setting the threshold to nil - # because we should not depend on a value that may be changed - # downwards. - Thread.current[SILENCED] = true + current = Thread.current + original, current[:available_queries_for_explain] = current[:available_queries_for_explain], false yield ensure - Thread.current[SILENCED] = false + current[:available_queries_for_explain] = original end end end diff --git a/activerecord/lib/active_record/explain_subscriber.rb b/activerecord/lib/active_record/explain_subscriber.rb new file mode 100644 index 0000000000..fc76410499 --- /dev/null +++ b/activerecord/lib/active_record/explain_subscriber.rb @@ -0,0 +1,21 @@ +require 'active_support/notifications' + +module ActiveRecord + class ExplainSubscriber # :nodoc: + def call(*args) + if queries = Thread.current[:available_queries_for_explain] + payload = args.last + queries << payload.values_at(:sql, :binds) unless ignore_payload?(payload) + end + end + + # SCHEMA queries cannot be EXPLAINed, also we do not want to run EXPLAIN on + # our own EXPLAINs now matter how loopingly beautiful that would be. + IGNORED_PAYLOADS = %w(SCHEMA EXPLAIN) + def ignore_payload?(payload) + payload[:exception] || IGNORED_PAYLOADS.include?(payload[:name]) + end + + ActiveSupport::Notifications.subscribe("sql.active_record", new) + end +end diff --git a/activerecord/lib/active_record/log_subscriber.rb b/activerecord/lib/active_record/log_subscriber.rb index 3a015ee8c2..a25f2c7bca 100644 --- a/activerecord/lib/active_record/log_subscriber.rb +++ b/activerecord/lib/active_record/log_subscriber.rb @@ -26,9 +26,9 @@ module ActiveRecord return if 'SCHEMA' == payload[:name] - name = '%s (%.1fms)' % [payload[:name], event.duration] - sql = payload[:sql].squeeze(' ') - binds = nil + name = '%s (%.1fms)' % [payload[:name], event.duration] + sql = payload[:sql].squeeze(' ') + binds = nil unless (payload[:binds] || []).empty? binds = " " + payload[:binds].map { |col,v| diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 30cd8679fe..2897e354c5 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -155,8 +155,8 @@ module ActiveRecord # Please see further details in the # {Active Record Query Interface guide}[http://edgeguides.rubyonrails.org/active_record_querying.html#running-explain]. def explain - _, sqls, binds = collecting_sqls_for_explain { exec_query } - exec_explain(sqls, binds) + _, queries = collecting_queries_for_explain { exec_queries } + exec_explain(queries) end def to_a @@ -169,11 +169,11 @@ module ActiveRecord # threshold, and at the same time the sum of them all does. The user # should get a query plan logged in that case. logging_query_plan do - exec_query + exec_queries end end - def exec_query + def exec_queries return @records if loaded? default_scoped = with_default_scope @@ -204,7 +204,7 @@ module ActiveRecord @loaded = true @records end - private :exec_query + private :exec_queries def as_json(options = nil) #:nodoc: to_a.as_json(options) diff --git a/activerecord/lib/active_record/validations/associated.rb b/activerecord/lib/active_record/validations/associated.rb index 7af0352a31..9f072c4c39 100644 --- a/activerecord/lib/active_record/validations/associated.rb +++ b/activerecord/lib/active_record/validations/associated.rb @@ -2,8 +2,9 @@ module ActiveRecord module Validations class AssociatedValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) - return if (value.is_a?(Array) ? value : [value]).collect{ |r| r.nil? || r.valid? }.all? - record.errors.add(attribute, :invalid, options.merge(:value => value)) + if Array.wrap(value).reject {|r| r.marked_for_destruction? || r.valid?}.any? + record.errors.add(attribute, :invalid, options.merge(:value => value)) + end end end diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index d846eb03aa..343c8ef373 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -1601,6 +1601,16 @@ class BasicsTest < ActiveRecord::TestCase end end + def test_sequence_name_with_abstract_class + ak = Class.new(ActiveRecord::Base) + ak.abstract_class = true + k = Class.new(ak) + k.table_name = "projects" + orig_name = k.sequence_name + return skip "sequences not supported by db" unless orig_name + assert_equal k.reset_sequence_name, orig_name + end + def test_count_with_join res = Post.count_by_sql "SELECT COUNT(*) FROM posts LEFT JOIN comments ON posts.id=comments.post_id WHERE posts.#{QUOTED_TYPE} = 'Post'" diff --git a/activerecord/test/cases/explain_test.rb b/activerecord/test/cases/explain_test.rb index 0284cca920..6ae6f83446 100644 --- a/activerecord/test/cases/explain_test.rb +++ b/activerecord/test/cases/explain_test.rb @@ -15,8 +15,8 @@ if ActiveRecord::Base.connection.supports_explain? end def test_logging_query_plan - base.logger.expects(:warn).with do |value| - value.starts_with?('EXPLAIN for:') + base.logger.expects(:warn).with do |message| + message.starts_with?('EXPLAIN for:') end with_threshold(0) do @@ -24,41 +24,42 @@ if ActiveRecord::Base.connection.supports_explain? end end - def test_collecting_sqls_for_explain + def test_collect_queries_for_explain base.auto_explain_threshold_in_seconds = nil - honda = cars(:honda) - - expected_sqls = [] - expected_binds = [] - callback = lambda do |*args| - payload = args.last - unless base.ignore_explain_notification?(payload) - expected_sqls << payload[:sql] - expected_binds << payload[:binds] - end + queries = Thread.current[:available_queries_for_explain] = [] + + with_threshold(0) do + Car.where(:name => 'honda').all end - result = sqls = binds = nil - ActiveSupport::Notifications.subscribed(callback, "sql.active_record") do - with_threshold(0) do - result, sqls, binds = base.collecting_sqls_for_explain { - Car.where(:name => 'honda').all - } - end + sql, binds = queries[0] + assert_match "SELECT", sql + assert_match "honda", sql + assert_equal [], binds + ensure + Thread.current[:available_queries_for_explain] = nil + end + + def test_collecting_queries_for_explain + result, queries = ActiveRecord::Base.collecting_queries_for_explain do + Car.where(:name => 'honda').all end - assert_equal result, [honda] - assert_equal expected_sqls, sqls - assert_equal expected_binds, binds + sql, binds = queries[0] + assert_match "SELECT", sql + assert_match "honda", sql + assert_equal [], binds + assert_equal [cars(:honda)], result end def test_exec_explain_with_no_binds - sqls = %w(foo bar) - binds = [[], []] + sqls = %w(foo bar) + binds = [[], []] + queries = sqls.zip(binds) connection.stubs(:explain).returns('query plan foo', 'query plan bar') expected = sqls.map {|sql| "EXPLAIN for: #{sql}\nquery plan #{sql}"}.join("\n") - assert_equal expected, base.exec_explain(sqls, binds) + assert_equal expected, base.exec_explain(queries) end def test_exec_explain_with_binds @@ -66,8 +67,9 @@ if ActiveRecord::Base.connection.supports_explain? cols[0].expects(:name).returns('wadus') cols[1].expects(:name).returns('chaflan') - sqls = %w(foo bar) - binds = [[[cols[0], 1]], [[cols[1], 2]]] + sqls = %w(foo bar) + binds = [[[cols[0], 1]], [[cols[1], 2]]] + queries = sqls.zip(binds) connection.stubs(:explain).returns("query plan foo\n", "query plan bar\n") expected = <<-SQL.strip_heredoc @@ -77,7 +79,7 @@ if ActiveRecord::Base.connection.supports_explain? EXPLAIN for: #{sqls[1]} [["chaflan", 2]] query plan bar SQL - assert_equal expected, base.exec_explain(sqls, binds) + assert_equal expected, base.exec_explain(queries) end def test_silence_auto_explain diff --git a/activerecord/test/cases/validations/association_validation_test.rb b/activerecord/test/cases/validations/association_validation_test.rb index 56e345990f..768fbc5b0a 100644 --- a/activerecord/test/cases/validations/association_validation_test.rb +++ b/activerecord/test/cases/validations/association_validation_test.rb @@ -61,6 +61,16 @@ class AssociationValidationTest < ActiveRecord::TestCase assert r.valid? end + def test_validates_associated_marked_for_destruction + Topic.validates_associated(:replies) + Reply.validates_presence_of(:content) + t = Topic.new + t.replies << Reply.new + assert t.invalid? + t.replies.first.mark_for_destruction + assert t.valid? + end + def test_validates_associated_with_custom_message_using_quotes Reply.validates_associated :topic, :message=> "This string contains 'single' and \"double\" quotes" Topic.validates_presence_of :content diff --git a/activesupport/lib/active_support/dependencies.rb b/activesupport/lib/active_support/dependencies.rb index 100b48312a..989c7205fa 100644 --- a/activesupport/lib/active_support/dependencies.rb +++ b/activesupport/lib/active_support/dependencies.rb @@ -227,7 +227,7 @@ module ActiveSupport #:nodoc: def load_dependency(file) if Dependencies.load? - Dependencies.new_constants_in(Object) { yield }.presence + Dependencies.new_constants_in(Object) { yield } else yield end @@ -236,13 +236,13 @@ module ActiveSupport #:nodoc: raise end - def load(file, *) + def load(file, wrap = false) result = false load_dependency(file) { result = super } result end - def require(file, *) + def require(file) result = false load_dependency(file) { result = super } result diff --git a/activesupport/test/dependencies_test.rb b/activesupport/test/dependencies_test.rb index fe8f51e11a..7039f90499 100644 --- a/activesupport/test/dependencies_test.rb +++ b/activesupport/test/dependencies_test.rb @@ -258,6 +258,85 @@ class DependenciesTest < Test::Unit::TestCase $:.replace(original_path) end + def test_require_returns_true_when_file_not_yet_required + path = File.expand_path("../autoloading_fixtures/load_path", __FILE__) + original_path = $:.dup + original_features = $".dup + $:.push(path) + + with_loading do + assert_equal true, require('loaded_constant') + end + ensure + remove_constants(:LoadedConstant) + $".replace(original_features) + $:.replace(original_path) + end + + def test_require_returns_true_when_file_not_yet_required_even_when_no_new_constants_added + path = File.expand_path("../autoloading_fixtures/load_path", __FILE__) + original_path = $:.dup + original_features = $".dup + $:.push(path) + + with_loading do + Object.module_eval "module LoadedConstant; end" + assert_equal true, require('loaded_constant') + end + ensure + remove_constants(:LoadedConstant) + $".replace(original_features) + $:.replace(original_path) + end + + def test_require_returns_false_when_file_already_required + path = File.expand_path("../autoloading_fixtures/load_path", __FILE__) + original_path = $:.dup + original_features = $".dup + $:.push(path) + + with_loading do + require 'loaded_constant' + assert_equal false, require('loaded_constant') + end + ensure + remove_constants(:LoadedConstant) + $".replace(original_features) + $:.replace(original_path) + end + + def test_require_raises_load_error_when_file_not_found + with_loading do + assert_raise(LoadError) { require 'this_file_dont_exist_dude' } + end + ensure + remove_constants(:LoadedConstant) + end + + def test_load_returns_true_when_file_found + path = File.expand_path("../autoloading_fixtures/load_path", __FILE__) + original_path = $:.dup + original_features = $".dup + $:.push(path) + + with_loading do + assert_equal true, load('loaded_constant.rb') + assert_equal true, load('loaded_constant.rb') + end + ensure + remove_constants(:LoadedConstant) + $".replace(original_features) + $:.replace(original_path) + end + + def test_load_raises_load_error_when_file_not_found + with_loading do + assert_raise(LoadError) { load 'this_file_dont_exist_dude.rb' } + end + ensure + remove_constants(:LoadedConstant) + end + def failing_test_access_thru_and_upwards_fails with_autoloading_fixtures do assert ! defined?(ModuleFolder) diff --git a/railties/lib/rails/source_annotation_extractor.rb b/railties/lib/rails/source_annotation_extractor.rb index 1eed763aa3..a9f2180196 100644 --- a/railties/lib/rails/source_annotation_extractor.rb +++ b/railties/lib/rails/source_annotation_extractor.rb @@ -30,7 +30,7 @@ class SourceAnnotationExtractor # Prints all annotations with tag +tag+ under the root directories +app+, +config+, +lib+, # +script+, and +test+ (recursively). Only filenames with extension - # +.builder+, +.rb+, +.rxml+, +.rhtml+, or +.erb+ are taken into account. The +options+ + # +.builder+, +.rb+, and +.erb+ are taken into account. The +options+ # hash is passed to each annotation's +to_s+. # # This class method is the single entry point for the rake tasks. @@ -47,7 +47,7 @@ class SourceAnnotationExtractor # Returns a hash that maps filenames under +dirs+ (recursively) to arrays # with their annotations. Only files with annotations are included, and only - # those with extension +.builder+, +.rb+, +.rxml+, +.rhtml+, and +.erb+ + # those with extension +.builder+, +.rb+, and +.erb+ # are taken into account. def find(dirs=%w(app config lib script test)) dirs.inject({}) { |h, dir| h.update(find_in(dir)) } @@ -55,7 +55,7 @@ class SourceAnnotationExtractor # Returns a hash that maps filenames under +dir+ (recursively) to arrays # with their annotations. Only files with annotations are included, and only - # those with extension +.builder+, +.rb+, +.rxml+, +.rhtml+, and +.erb+ + # those with extension +.builder+, +.rb+, and +.erb+ # are taken into account. def find_in(dir) results = {} @@ -65,9 +65,9 @@ class SourceAnnotationExtractor if File.directory?(item) results.update(find_in(item)) - elsif item =~ /\.(builder|(r(?:b|xml|js)))$/ + elsif item =~ /\.(builder|rb)$/ results.update(extract_annotations_from(item, /#\s*(#{tag}):?\s*(.*)$/)) - elsif item =~ /\.(rhtml|erb)$/ + elsif item =~ /\.erb$/ results.update(extract_annotations_from(item, /<%\s*#\s*(#{tag}):?\s*(.*?)\s*%>/)) end end |