Merge pull request #37516 from jonathanhefner/generator-generate-avoid-shell-out

Avoid shelling out for generator `generate` action

railties/lib/rails/generators/actions.rb

@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+require "shellwords"
 require "active_support/core_ext/string/strip"
 
 module Rails
@@ -225,9 +226,9 @@ module Rails
         log :generate, what
 
         options = args.extract_options!
-        argument = args.flat_map(&:to_s).join(" ")
+        options[:abort_on_failure] = !options[:inline]
 
-        execute_command :rails, "generate #{what} #{argument}", options
+        rails_command "generate #{what} #{args.join(" ")}", options
       end
 
       # Runs the supplied rake task (invoked with 'rake ...')
@@ -247,7 +248,17 @@ module Rails
       #   rails_command("gems:install", sudo: true)
       #   rails_command("gems:install", capture: true)
       def rails_command(command, options = {})
-        execute_command :rails, command, options
+        if options[:inline]
+          log :rails, command
+          command, *args = Shellwords.split(command)
+          in_root do
+            silence_warnings do
+              ::Rails::Command.invoke(command, args, options)
+            end
+          end
+        else
+          execute_command :rails, command, options
+        end
       end
 
       # Make an entry in Rails routing file <tt>config/routes.rb</tt>

railties/test/generators/actions_test.rb

@@ -378,22 +378,36 @@ class ActionsTest < Rails::Generators::TestCase
     assert_file "config/initializers/constants.rb", code.strip_heredoc
   end
 
-  def test_generate_should_run_script_generate_with_argument_and_options
+  test "generate" do
     run_generator
     action :generate, "model", "MyModel"
     assert_file "app/models/my_model.rb", /MyModel/
   end
 
-  def test_generate_aborts_when_subprocess_fails_if_requested
+  test "generate should raise on failure" do
     run_generator
-    content = capture(:stderr) do
+    message = capture(:stderr) do
       assert_raises SystemExit do
-        action :generate, "model", "MyModel:ADsad", abort_on_failure: true
-        action :generate, "model", "MyModel"
+        action :generate, "model", "1234567890"
       end
     end
-    assert_match(/wrong constant name MyModel:aDsad/, content)
-    assert_no_file "app/models/my_model.rb"
+    assert_match(/1234567890/, message)
+  end
+
+  test "generate with inline option" do
+    run_generator
+    assert_not_called(generator, :run) do
+      action :generate, "model", "MyModel", inline: true
+    end
+    assert_file "app/models/my_model.rb", /MyModel/
+  end
+
+  test "generate with inline option should raise on failure" do
+    run_generator
+    error = assert_raises do
+      action :generate, "model", "1234567890", inline: true
+    end
+    assert_match(/1234567890/, error.message)
   end
 
   test "rake should run rake with the default environment" do
@@ -438,6 +452,14 @@ class ActionsTest < Rails::Generators::TestCase
     end
   end
 
+  test "rake with abort_on_failure option should raise on failure" do
+    capture(:stderr) do
+      assert_raises SystemExit do
+        action :rake, "invalid", abort_on_failure: true
+      end
+    end
+  end
+
   test "rails_command should run rails with the default environment" do
     assert_runs "rails log:clear", env: { "RAILS_ENV" => "development" } do
       with_rails_env nil do
@@ -484,6 +506,31 @@ class ActionsTest < Rails::Generators::TestCase
     end
   end
 
+  test "rails_command with abort_on_failure option should raise on failure" do
+    run_generator
+    capture(:stderr) do
+      assert_raises SystemExit do
+        action :rails_command, "invalid", abort_on_failure: true
+      end
+    end
+  end
+
+  test "rails_command with inline option" do
+    run_generator
+    assert_not_called(generator, :run) do
+      action :rails_command, "generate model MyModel", inline: true
+    end
+    assert_file "app/models/my_model.rb", /MyModel/
+  end
+
+  test "rails_command with inline option should raise on failure" do
+    run_generator
+    error = assert_raises do
+      action :rails_command, "generate model 1234567890", inline: true
+    end
+    assert_match(/1234567890/, error.message)
+  end
+
   test "route should add route" do
     run_generator
     route_commands = ["get 'foo'", "get 'bar'", "get 'baz'"]