New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make all rails commands work in engine #27601

Merged
merged 2 commits into from Jan 15, 2017

Conversation

Projects
None yet
3 participants
@y-yagi
Member

y-yagi commented Jan 7, 2017

Currently, all rails commands can be executed in engine,
but server, console, dbconsole and runner do not work.

This make all rails commands work in engine.
Related to #22588

r? @kaspth

@y-yagi y-yagi changed the title from Make work all commands from engine to Make work all rails commands from engine Jan 7, 2017

@y-yagi y-yagi changed the title from Make work all rails commands from engine to Make all rails commands work in engine Jan 7, 2017

@y-yagi y-yagi force-pushed the y-yagi:make_work_all_commands_from_engine branch 2 times, most recently Jan 7, 2017

@maclover7 maclover7 added the railties label Jan 7, 2017

@y-yagi y-yagi force-pushed the y-yagi:make_work_all_commands_from_engine branch Jan 7, 2017

@kaspth

Lovely! ❤️

railties/lib/rails/commands/server/server_command.rb Outdated
if options[:pid] == DEFAULT_PID_PATH
File.expand_path(DEFAULT_PID_PATH)
else
options[:pid]

This comment has been minimized.

@kaspth

kaspth Jan 8, 2017

Member

Won't we need to expand this path too?

This comment has been minimized.

@y-yagi

y-yagi Jan 9, 2017

Member

I did this according to the original code. But you are right.
If it does not expand it may cause error in daemon mode. I fixed.

railties/lib/rails/engine/commands.rb Outdated
@@ -1,12 +1,7 @@
require "rails/command"
unless defined?(APP_PATH)
if File.exists?(File.expand_path("test/dummy/config/application.rb", ENGINE_ROOT))

This comment has been minimized.

@kaspth

kaspth Jan 8, 2017

Member

Isn't exists? deprecated? I think exist? is the one we should use.

railties/lib/rails/engine/commands.rb Outdated
require "rails/command"
unless defined?(APP_PATH)
if File.exists?(File.expand_path("test/dummy/config/application.rb", ENGINE_ROOT))
Object.const_set(:APP_PATH, File.expand_path("test/dummy/config/application", ENGINE_ROOT))

This comment has been minimized.

@kaspth

kaspth Jan 8, 2017

Member

Why do we have to go through const_set?

Won't a better place for this be somewhere the Rails::Command can set this internally?

This comment has been minimized.

@y-yagi

y-yagi Jan 9, 2017

Member

Sorry, this is a remnant of when I was fixing variously, const_set is unnecessary in the current code. I removed const_set.

@@ -3,6 +3,7 @@
ENGINE_ROOT = File.expand_path('../..', __FILE__)
ENGINE_PATH = File.expand_path('../../lib/<%= namespaced_name -%>/engine', __FILE__)
APP_PATH = File.expand_path('../../<%= dummy_path -%>/config/application', __FILE__)

This comment has been minimized.

@kaspth

kaspth Jan 8, 2017

Member

Is the APP_PATH trickery in the rails/engine/commands file to provide backwards compat for this line?

Won't the rails app:update command add this line for users?

This comment has been minimized.

@y-yagi

y-yagi Jan 9, 2017

Member

Is the APP_PATH trickery in the rails/engine/commands file to provide backwards compat for this line?

Yes, it is.

Won't the rails app:update command add this line for users?

Currently app:update works only with rails application and does not work with rails engine. However, I think that it is correct to correspond with the update command. I will fix in another PR.

Dir.chdir("..") do
default_options = parse_arguments
assert_equal old_default_options, default_options
assert_equal old_default_options, server.default_options

This comment has been minimized.

@kaspth

kaspth Jan 8, 2017

Member

I don't understand the need for this change. Can you elaborate?

This comment has been minimized.

@y-yagi

y-yagi Jan 9, 2017

Member

The value of Rails::Command::ServerCommand#server_options is changed when path is changed. Because it made it to acquire pid path dynamically.
This is because the directory is changed according to APP_PATH at server startup, so it is not possible to use a fixed path at the time of file loading.
https://github.com/rails/rails/blob/master/railties/lib/rails/command/actions.rb#L8

Therefore, it is correct that Rails::Command::ServerCommand#server_options will change when the directory changes, so I think the original test is incorrect.
However, the purpose of the test was first added, it was to confirm that Rails::Server#default_options returns the same result. So I changed to use Rails::Server#default_options.
Ref: 221b4ae

railties/CHANGELOG.md Outdated
@@ -1,3 +1,11 @@
* Make all rails commands work in engine.

This comment has been minimized.

@kaspth

kaspth Jan 8, 2017

Member

Make every Rails command work within engines.

railties/CHANGELOG.md Outdated
By default, commands that require rails application are executed against
dummy testing application. If you want to execute a command against a real
application, define `APP_PATH` of real application to `bin/rails` script.

This comment has been minimized.

@kaspth

kaspth Jan 8, 2017

Member

I find this tough to understand. Why don't we mention the commands that couldn't be run before and why they now can (i.e. we link the engine to the dummy app via APP_PATH?

I think we should leave the override mention to an updated engine guide in another PR 😊

This comment has been minimized.

@y-yagi

y-yagi Jan 9, 2017

Member

I think we should leave the override mention to an updated engine guide in another PR

👍 I will do!

@maclover7 maclover7 added the needs work label Jan 8, 2017

y-yagi added some commits Dec 14, 2016

make all rails commands work in engine
Currently, all rails commands can be executed in engine,
but `server`, `console`, `dbconsole` and `runner` do not work.

This make all rails commands work in engine.
Related to #22588
improve server default options test
This test was added in 221b4ae.
221b4ae modified to return the same result even if `Rails::Server#default_options`
is called more than once. Therefore, also use `Rails::Server#default_options`
instead of `ServerCommand#default_options` in test.

@y-yagi y-yagi force-pushed the y-yagi:make_work_all_commands_from_engine branch to f5f834f Jan 9, 2017

@kaspth

This comment has been minimized.

Member

kaspth commented Jan 15, 2017

Again, lovely! ❤️

Currently app:update works only with rails application and does not work with rails engine. However, I think that it is correct to correspond with the update command. I will fix in another PR.

👍 on that later PR you proposed 😉

@kaspth kaspth merged commit f8a3981 into rails:master Jan 15, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@y-yagi

This comment has been minimized.

Member

y-yagi commented Jan 15, 2017

Thanks!!

@y-yagi y-yagi deleted the y-yagi:make_work_all_commands_from_engine branch Jan 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment