opened
txus
username_0
Strong parameters included `Rack::Test::UploadedFile` as a permitted scalar in 91d1ac0, adding an implicit dependency on `rack/test`. This commit makes that explicit so that cherry-picking on Rails modules (for instance using rails-api) doesn't break strong parameters.
Add missing require "rack/test"
issue
created
rafaelfranca
username_1
Not sure if it is a good idea to add a test library as runtime dependency of strong parameters. But if we will do this can we require only upload_file?
comment
created
carlosantoniodasilva
username_2
/cc @username_3
comment
created
txus
username_0
@username_1 I know, I cringed at that too. However rack-test *is* already a runtime dependency of strong parameters (introduced in 91d1ac0), albeit implicit -- I'm just making it explicit.
comment
created
fxn
username_3
First a point about the need of an explicit require, which is the motivation for the PR.

The Rails code base is big, and we use Ruby autoloading to avoid repeating requires where we feel it pays off. Contract is that any client code should load the main file of a Rails component, you can't just cherry-pick a random file from a Rails library. The entry point of every Rails library declares autoloads among other things. See for example [*active_record.rb*](https://github.com/rails/rails/blob/master/activerecord/lib/active_record.rb).

In that line, every Rails component is also responsible for loading [*active_support/rails.rb*](https://github.com/rails/rails/blob/master/activesupport/lib/active_support/rails.rb), and thus no internal file is supposed to explicitly load any of those ones. You can freely use `blank?` for example, no need to put an explicit require for it (and it would be against project rules to do so).

So, in that sense, I am :-1: on the explicit require.

Another debate is whether `rack/test` should be loaded as a side-effect of the current autoload in Action Dispatch. That should not happen. Why do we have that autoload in a so [prominent place](https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch.rb#L31-L33)? Could we move it closer to something that is only evaluated in a test environment perhaps? (Note that `defined?(Rack::Test)` returns true because there's an autoload, so we cannot conditionally add the class.)
comment
created
Ferdy89
username_4
This was already implemented here https://github.com/rails/rails/pull/22475 so that makes this issue stale
comment
created
arthurnn
username_5
indeed this was done on https://github.com/rails/rails/pull/22475. Thanks @username_4 for noticing.

@username_6 have you saw this discussion before doing that? thoughts? cc @matthewd
comment
created
claudiob
username_6
No thoughts about it, whatever works :wink:
comment