Merge remote-tracking branch 'upstream/master' into all-custom-error

* upstream/master: have plugin generator respect root option Fix very strange bug with form_for and capture_html @ujifgc padrino#1177 [padrino-core/rake] Adds lib as a load_path for rake tasks padrino#1185 [form_helpers] Use count instead of size (Closes padrino#1184) [format_helpers] Mark escaped text as html_safe (Closes padrino#1183) Closes padrino#1179 for accidental appended extensions for js urls format activerecord migrate formatting activerecord migration test Thanks to @kenkeiter for bringing up the fact that http_router [is missing PATCH](https://github.com/joshbuddy/http_router/blob/master/lib/http_router/route.rb#L5).
WaYdotNET · Mar 26, 2013 · 6e6a02a · 6e6a02a
2 parents 5c4e325 + 1f4ef4a
commit 6e6a02a
Show file tree

Hide file tree

Showing 19 changed files with 84 additions and 31 deletions.
diff --git a/padrino-core/lib/padrino-core/application/routing.rb b/padrino-core/lib/padrino-core/application/routing.rb
@@ -64,6 +64,8 @@ def process_destination_path(path, env)
 
   # @private
   class Route
+    VALID_HTTP_VERBS = %w{GET POST PUT PATCH DELETE HEAD OPTIONS LINK UNLINK}
+
     attr_accessor :use_layout, :controller, :action, :cache, :cache_key, :cache_expires_in, :parent
 
     def before_filters(&block)

diff --git a/padrino-core/lib/padrino-core/cli/rake.rb b/padrino-core/lib/padrino-core/cli/rake.rb
@@ -7,8 +7,9 @@
 
 module PadrinoTasks
   def self.init(init=false)
+    $LOAD_PATH.unshift(File.expand_path("lib")) # Adds "lib" to the load path
     Padrino::Tasks.files.flatten.uniq.each { |rakefile| Rake.application.add_import(rakefile) rescue puts "<= Failed load #{ext}" }
-    load(File.expand_path('../rake_tasks.rb', __FILE__))
+    load(File.expand_path('../rake_tasks.rb', __FILE__)) # Load default rake tasks
     Rake.application.load_imports
   end
 

diff --git a/padrino-gen/lib/padrino-gen/generators/components/orms/activerecord.rb b/padrino-gen/lib/padrino-gen/generators/components/orms/activerecord.rb
@@ -149,7 +149,7 @@ def self.down
 end
 MIGRATION
 
-AR_MODEL_UP_MG = (<<-MIGRATION) unless defined?(AR_MODEL_UP_MG)
+AR_MODEL_UP_MG = (<<-MIGRATION).gsub(/^/,'    ') unless defined?(AR_MODEL_UP_MG)
 create_table :!TABLE! do |t|
   !FIELDS!
   t.timestamps

diff --git a/padrino-gen/lib/padrino-gen/generators/plugin.rb b/padrino-gen/lib/padrino-gen/generators/plugin.rb
@@ -39,9 +39,9 @@ def setup_plugin
         if options[:list] || plugin_file.nil? # list method ran here
           list_plugins
         else # executing the plugin instructions
+          self.destination_root = options[:root]
           if in_app_root?
             self.behavior = :revoke if options[:destroy]
-            self.destination_root = options[:root]
             execute_runner(:plugin, plugin_file)
           else
             say "You are not at the root of a Padrino application! (config/boot.rb not found)"

diff --git a/padrino-gen/test/test_model_generator.rb b/padrino-gen/test/test_model_generator.rb
@@ -82,11 +82,11 @@ def teardown
       capture_io { generate(:model, 'person', "name", "age:integer", "email", "-r=#{@apptmp}/sample_project") }
       migration_file_path = "#{@apptmp}/sample_project/db/migrate/001_create_people.rb"
       assert_match_in_file(/class CreatePeople < ActiveRecord::Migration/m, migration_file_path)
-      assert_match_in_file(/create_table :people/m, migration_file_path)
-      assert_match_in_file(/t.string :name/m,   migration_file_path)
-      assert_match_in_file(/t.integer :age/m,   migration_file_path)
-      assert_match_in_file(/t.string :email/m,  migration_file_path)
-      assert_match_in_file(/drop_table :people/m, migration_file_path)
+      assert_match_in_file(/    create_table :people/m, migration_file_path)
+      assert_match_in_file(/      t.string :name/m,   migration_file_path)
+      assert_match_in_file(/      t.integer :age/m,   migration_file_path)
+      assert_match_in_file(/      t.string :email/m,  migration_file_path)
+      assert_match_in_file(/    drop_table :people/m, migration_file_path)
     end
   end
 
@@ -116,8 +116,8 @@ def teardown
       capture_io { generate(:model, 'user', "-r=#{@apptmp}/sample_project") }
       migration_file_path = "#{@apptmp}/sample_project/db/migrate/001_create_users.rb"
       assert_match_in_file(/class CreateUsers < ActiveRecord::Migration/m, migration_file_path)
-      assert_match_in_file(/create_table :users/m, migration_file_path)
-      assert_match_in_file(/drop_table :users/m, migration_file_path)
+      assert_match_in_file(/    create_table :users/m, migration_file_path)
+      assert_match_in_file(/    drop_table :users/m, migration_file_path)
     end
 
     should "generate migration file with given fields" do
@@ -126,11 +126,11 @@ def teardown
       capture_io { generate(:model, 'person', "name:string", "age:integer", "email:string", "-r=#{@apptmp}/sample_project") }
       migration_file_path = "#{@apptmp}/sample_project/db/migrate/001_create_people.rb"
       assert_match_in_file(/class CreatePeople < ActiveRecord::Migration/m, migration_file_path)
-      assert_match_in_file(/create_table :people/m, migration_file_path)
-      assert_match_in_file(/t.string :name/m,   migration_file_path)
-      assert_match_in_file(/t.integer :age/m,   migration_file_path)
-      assert_match_in_file(/t.string :email/m,  migration_file_path)
-      assert_match_in_file(/drop_table :people/m, migration_file_path)
+      assert_match_in_file(/    create_table :people/m, migration_file_path)
+      assert_match_in_file(/      t.string :name/m,   migration_file_path)
+      assert_match_in_file(/      t.integer :age/m,   migration_file_path)
+      assert_match_in_file(/      t.string :email/m,  migration_file_path)
+      assert_match_in_file(/    drop_table :people/m, migration_file_path)
     end
   end
 

diff --git a/padrino-gen/test/test_plugin_generator.rb b/padrino-gen/test/test_plugin_generator.rb
@@ -10,6 +10,15 @@ def teardown
     `rm -rf #{@apptmp}`
   end
 
+  context "the plugin generator" do
+    should "respect --root option" do
+      path = File.expand_path('../fixtures/plugin_template.rb', __FILE__)
+      capture_io { generate(:project, 'sample_project', "--root=#{@apptmp}") }
+      out, err = capture_io { generate(:plugin, path, "--root=#{@apptmp}/sample_project") }
+      refute_match /You are not at the root/, out
+    end
+  end
+
   context "the plugin destroy option" do
     should "remove the plugin instance" do
       path = File.expand_path('../fixtures/plugin_template.rb', __FILE__)

diff --git a/padrino-helpers/lib/padrino-helpers/asset_tag_helpers.rb b/padrino-helpers/lib/padrino-helpers/asset_tag_helpers.rb
@@ -5,8 +5,8 @@ module Helpers
     #
     module AssetTagHelpers
       FRAGMENT_HASH = "#".html_safe.freeze
-       # assets that require an appended extension
-      APPEND_ASSET_EXTENSIONS = ["js", "css"]
+      APPEND_ASSET_EXTENSIONS = ["js", "css"]  # assets that require an appended extension
+      ABSOLUTE_URL_PATTERN = %r{^(https?://)} # absolute url regex
 
       ##
       # Creates a div to display the flash of given type if it exists
@@ -323,7 +323,7 @@ def image_path(src)
       # @api semipublic
       def asset_path(kind, source)
         source = asset_normalize_extension(kind, URI.escape(source.to_s))
-        return source if source =~ %r{^(/|https?://)} # absolute source
+        return source if source =~ ABSOLUTE_URL_PATTERN || source =~ /^\// # absolute source
         source = File.join(asset_folder_name(kind), source)
         timestamp = asset_timestamp(source)
         result_path = uri_root_path(source)
@@ -405,7 +405,7 @@ def asset_folder_name(kind)
       #
       def asset_normalize_extension(kind, source)
         ignore_extension = !APPEND_ASSET_EXTENSIONS.include?(kind.to_s)
-        source << ".#{kind}" unless ignore_extension or source =~ /\.#{kind}/
+        source << ".#{kind}" unless ignore_extension || source =~ /\.#{kind}/ || source =~ ABSOLUTE_URL_PATTERN
         source
       end
 

diff --git a/padrino-helpers/lib/padrino-helpers/form_helpers.rb b/padrino-helpers/lib/padrino-helpers/form_helpers.rb
@@ -31,7 +31,8 @@ module FormHelpers
       # @api public
       def form_for(object, url, settings={}, &block)
         instance = builder_instance(object, settings)
-        form_tag(url, settings) { capture_html(instance, &block) }
+        html = capture_html(instance, &block)
+        form_tag(url, settings) { html }
       end
 
       ##
@@ -170,7 +171,7 @@ def error_messages_for(*objects)
         objects = objects.map { |object_name|
           object_name.is_a?(Symbol) ? instance_variable_get("@#{object_name}") : object_name
         }.compact
-        count   = objects.inject(0) { |sum, object| sum + object.errors.size }
+        count   = objects.inject(0) { |sum, object| sum + object.errors.count }
 
         unless count.zero?
           html = {}

diff --git a/padrino-helpers/lib/padrino-helpers/format_helpers.rb b/padrino-helpers/lib/padrino-helpers/format_helpers.rb
@@ -19,7 +19,7 @@ module FormatHelpers
       #
       # @api public
       def escape_html(text)
-        Rack::Utils.escape_html(text)
+        Rack::Utils.escape_html(text).html_safe
       end
       alias h escape_html
       alias sanitize_html escape_html
@@ -41,8 +41,8 @@ def escape_html(text)
       #
       # @api public
       def h!(text, blank_text = '&nbsp;')
-        return blank_text if text.nil? || text.empty?
-        h text
+        return blank_text.html_safe if text.nil? || text.empty?
+        h(text)
       end
 
       ##

diff --git a/padrino-helpers/lib/padrino-helpers/output_helpers.rb b/padrino-helpers/lib/padrino-helpers/output_helpers.rb
@@ -48,12 +48,12 @@ def render(engine, *) # @private
       # @api semipublic
       def capture_html(*args, &block)
         handler = find_proper_handler
-        captured_html = ""
+        captured_block, captured_html = nil, ""
         if handler && handler.is_type? && handler.block_is_type?(block)
-          captured_html = handler.capture_from_template(*args, &block)
+          captured_html, captured_block = handler.capture_from_template(*args, &block)
         end
         # invoking the block directly if there was no template
-        captured_html = block_given? && block.call(*args) if captured_html.blank?
+        captured_html = block_given? && ( captured_block || block.call(*args) )  if captured_html.blank?
         captured_html
       end
       alias :capture :capture_html

diff --git a/padrino-helpers/lib/padrino-helpers/output_helpers/erb_handler.rb b/padrino-helpers/lib/padrino-helpers/output_helpers/erb_handler.rb
@@ -29,10 +29,10 @@ def is_type?
         #
         def capture_from_template(*args, &block)
           self.output_buffer, _buf_was = ActiveSupport::SafeBuffer.new, self.output_buffer
-          block.call(*args)
+          captured_block = block.call(*args)
           ret = eval("@_out_buf", block.binding)
           self.output_buffer = _buf_was
-          ret
+          [ ret, captured_block ]
         end
 
         ##

diff --git a/padrino-helpers/lib/padrino-helpers/output_helpers/slim_handler.rb b/padrino-helpers/lib/padrino-helpers/output_helpers/slim_handler.rb
@@ -31,10 +31,10 @@ def is_type?
         #
         def capture_from_template(*args, &block)
           self.output_buffer, _buf_was = ActiveSupport::SafeBuffer.new, self.output_buffer
-          block.call(*args)
+          captured_block = block.call(*args)
           ret = eval("@_out_buf", block.binding)
           self.output_buffer = _buf_was
-          ret
+          [ ret, captured_block ]
         end
 
         ##

diff --git a/padrino-helpers/test/fixtures/render_app/app.rb b/padrino-helpers/test/fixtures/render_app/app.rb
@@ -31,6 +31,10 @@ class RenderDemo < Padrino::Application
     render :explicit_engine
   end
 
+  get '/double_capture_:ext' do
+    render "double_capture_#{params[:ext]}"
+  end
+
   # partial with object
   get '/partial/object' do
     partial 'template/user', :object => RenderUser.new('John'), :locals => { :extra => "bar" }

diff --git a/padrino-helpers/test/fixtures/render_app/views/double_capture_erb.erb b/padrino-helpers/test/fixtures/render_app/views/double_capture_erb.erb
@@ -0,0 +1,3 @@
+<% form_for( :object, '/' ) do |f| %>
+  <%= $number_of_captures += 1 %>
+<% end %>
diff --git a/padrino-helpers/test/fixtures/render_app/views/double_capture_haml.haml b/padrino-helpers/test/fixtures/render_app/views/double_capture_haml.haml
@@ -0,0 +1,2 @@
+- form_for :object, '/' do |f|
+  = $number_of_captures += 1
diff --git a/padrino-helpers/test/fixtures/render_app/views/double_capture_slim.slim b/padrino-helpers/test/fixtures/render_app/views/double_capture_slim.slim
@@ -0,0 +1,2 @@
+- form_for :object, '/' do |f|
+  = $number_of_captures += 1
diff --git a/padrino-helpers/test/test_asset_tag_helpers.rb b/padrino-helpers/test/test_asset_tag_helpers.rb
@@ -294,6 +294,12 @@ def flash
       assert_has_tag('script', :src => "/blog/javascripts/application.js?#{time.to_i}", :type => "text/javascript") { actual_html }
     end
 
+    should "not append extension to absolute paths" do
+      time = stop_time_for_test
+      actual_html = javascript_include_tag('https://maps.googleapis.com/maps/api/js?key=value&sensor=false')
+      assert_has_tag('script', :src => "https://maps.googleapis.com/maps/api/js?key=value&sensor=false") { actual_html }
+    end
+
     should "display javascript items" do
       time = stop_time_for_test
       actual_html = javascript_include_tag('application', 'base.js', 'http://google.com/lib.js')

diff --git a/padrino-helpers/test/test_format_helpers.rb b/padrino-helpers/test/test_format_helpers.rb
@@ -139,6 +139,11 @@ def setup
     should "return text escaped if not empty" do
       assert_equal '&lt;h1&gt;hello&lt;&#x2F;h1&gt;', h!('<h1>hello</h1>')
     end
+    should "mark escaped text as safe" do
+      assert_equal false, '<h1>hello</h1>'.html_safe?
+      assert_equal true, h('<h1>hello</h1>').html_safe?
+      assert_equal true, h!("", "default").html_safe?
+    end
   end
 
   context 'for #time_ago_in_words method' do

diff --git a/padrino-helpers/test/test_render_helpers.rb b/padrino-helpers/test/test_render_helpers.rb
@@ -72,5 +72,23 @@ def app
       assert_have_selector 'p.slim span',  :content => "slim"
       assert_have_selector 'p.end',   :content => "haml"
     end
+
+    should "capture slim template once and only once" do
+      $number_of_captures = 0
+      visit '/double_capture_slim'
+      assert_equal 1,$number_of_captures
+    end
+
+    should "capture haml template once and only once" do
+      $number_of_captures = 0
+      visit '/double_capture_haml'
+      assert_equal 1,$number_of_captures
+    end
+
+    should "capture erb template once and only once" do
+      $number_of_captures = 0
+      visit '/double_capture_erb'
+      assert_equal 1,$number_of_captures
+    end
   end
 end