Don't add RAILS_ENV in generate action#34980
Conversation
In the case of generator, `RAILS_ENV` is interpreted as an argument as it is. Avoid this because it will result unintended by the user. Fixes rails#34979.
| config[:abort_on_failure] = options[:abort_on_failure] if options[:abort_on_failure] | ||
|
|
||
| in_root { run("#{sudo}#{extify(executor)} #{command} RAILS_ENV=#{env}", config) } | ||
| in_root { run("#{sudo}#{extify(executor)} #{command}#{rails_env}", config) } |
There was a problem hiding this comment.
Could we fix this by prefixing the command with the Rails environment? Seems off that the environment handling leaks into the generate method.
There was a problem hiding this comment.
Yeah, handling env variables as arguments is a rake only thing. Prefixing the command with the variable instead should work everywhere 👍.
There was a problem hiding this comment.
Wait, so if I run a rake task and set an env variable at the end it’s actually not bash that makes it available in ENV, but Rake? I did not know that!
There was a problem hiding this comment.
Yeah, the shell assumes nothing about program's arguments, no matter whether they include an equal sign 😃
$ touch a.txt; cat a.txt RAILS_ENV=production
cat: 'RAILS_ENV=production': No existe el archivo o el directorioThere was a problem hiding this comment.
@deivid-rodriguez
Thanks for your useful information. I was wondering if this feature works on Windows. By doing this, I think that rake was working as expected on Windows.
@kaspth
In my understanding, the same fix as rake is necessary for rails command. On unix, there is no problem if you specify the environment variable before the command, but on Windows it is aware that rake and rails are necessary to provide equivalent functionality. What do you think?
There was a problem hiding this comment.
@y-yagi That's a very good point 👍.
Another idea would be to add support for passing env to thor's run method. system already supports passing an env to it, and backticks would need to be replaced with Open3.capture2e or something similar.
There was a problem hiding this comment.
I suspect that rails might need to learn to recognise such env-like parameters, at least while we're transitioning subcommands from rake to thor, even if we mark them as deprecated along the way. (I'm a little surprised we haven't hit this yet, but imagine it'll be more noticeable as we cover more commands, that are run in more varied circumstances.)
As implied by the mention of deprecation, though, I'd prefer we didn't use that as our internal mechanism.
While I always prefer a many-args strategy for running subcommands, including passing env "properly", I'm not sure it'd work reliably here: I don't recall when it comes into play, but that #{sudo} looks ominous.
All of that said, though, if we know we're running a rails subcommand, might it be worth considering invoking Rails::Command-type stuff directly?
There was a problem hiding this comment.
Thanks, I think your concern is correct.
Originally, generate action did not recognize env (document has no explanation). And since I can not think of a use case that needs to specify env when executing generator, I think there is no need to support that.
Since processing to eliminate RAILS_ENV is necessary, I will merge this PR first, especially if there is no objection.
I would like to consider separately about the behavior when rails action is executed.
Follow-up to rails#34980. Also, refactor tests to be less brittle.
In the case of generator,
RAILS_ENVis interpreted as an argument as it is. Avoid this because it will result unintended by the user.Fixes #34979.
By the way, I think that this is a problem that does not occur if env is specified before the command.
I did not understand why env was last specified(it seems that was due to the first commit d57b193), so this time I leave it as it is.