Skip to content
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

Clarification regarding Rack::Request#[] #1277

Closed
ioquatix opened this issue May 25, 2018 · 12 comments · Fixed by #1690
Closed

Clarification regarding Rack::Request#[] #1277

ioquatix opened this issue May 25, 2018 · 12 comments · Fixed by #1690
Milestone

Comments

@ioquatix
Copy link
Member

This method issues a warning that it will be deprecated.

But it's used in a lot of code.

What is the reason for it's deprecation and what is the timeline for removal?

I couldn't find any documentation regarding this.

It's a convenient shortcut.

@twitnithegirl
Copy link
Contributor

It was moved into helpers about 2 years ago and looks to me like it's not so much that it will go away any time soon just that it's discouraged in lieu of Request.params[]. Looks like @sgrif made the original change. Perhaps he can dig in his memory bank and remember why?

@sgrif
Copy link
Contributor

sgrif commented Aug 8, 2018

I think Aaron and I were pairing on something but I don't remember specifically why. Either way, you should use request.params[] as the deprecation warning says. I would guess it'll be removed in the next major version

@ioquatix
Copy link
Member Author

ioquatix commented Aug 9, 2018

It's basically just an alias for params.

rack/lib/rack/request.rb

Lines 426 to 444 in e2793b2

# shortcut for <tt>request.params[key]</tt>
def [](key)
if $VERBOSE
warn("Request#[] is deprecated and will be removed in a future version of Rack. Please use request.params[] instead")
end
params[key.to_s]
end
# shortcut for <tt>request.params[key] = value</tt>
#
# Note that modifications will not be persisted in the env. Use update_param or delete_param if you want to destructively modify params.
def []=(key, value)
if $VERBOSE
warn("Request#[]= is deprecated and will be removed in a future version of Rack. Please use request.params[]= instead")
end
params[key.to_s] = value
end

I really like it as it makes code short and concise.

@twitnithegirl
Copy link
Contributor

@ioquatix if the issue is that you want to be able to convert to future versions without changing this everywhere, while I think you should just make the necessary changes now or at upgrade, nothing stops you from aliasing this yourself from your own app.

@matthewd
Copy link
Contributor

matthewd commented Aug 9, 2018

To me, it's confusingly ambiguous because there's more than one thing it could reasonably be indexing: most notably, the env that the Request is wrapping, but also the headers.

That's why I'd be inclined to remove it, at least.

@ioquatix
Copy link
Member Author

ioquatix commented Aug 9, 2018

I don't personally find it confusing or ambiguous, since it's well documented, especially since headers and env have entirely different keys.

I can certainly understand how it's superfluous to existing functionality, and converting the key to a string is also an additional overhead, so on that basis I can understand it's removal. That being said, I find it a convenient shortcut which leads to terser code.

@ioquatix
Copy link
Member Author

ioquatix commented Aug 9, 2018

if the issue is that you want to be able to convert to future versions without changing this everywhere, while I think you should just make the necessary changes now or at upgrade, nothing stops you from aliasing this yourself from your own app.

For existing use cases, I'm not sure what we gain by removing this API. That was really my original question. Basically, once this API is removed, I need to make a lot of changes (I'm sure I'm not the only one), but I'm not sure to what end anyone benefits from this removal.

@twitnithegirl
Copy link
Contributor

Sorry to overload you with pings @tenderlove but I was wondering if you had thoughts on this, particularly as far as when one might expect this behavior to retired.

@ioquatix
Copy link
Member Author

Removing an API would need to be a major version bump, so maybe Rack 3.0?

@tenderlove
Copy link
Member

Ya, I don't expect we'd remove this until 3.0. My personal expectation is that Rack::Request#[] would reach in to env (or at least give you CGI env stuff), and the ambiguity that @matthewd points out is why I want to remove it. @ioquatix I'd like something shorter than Rack::Request#params[], but more "discoverable" ("discoverable" == "I don't need to read the docs to understand the intention") than Rack::Request#[], and I'm open to suggestions. I'd also like it if the method didn't return the whole params hash like Rack::Request#params does.

@jeremyevans
Copy link
Contributor

I think if we are planning on deprecating this in Rack 3, the warning should be emitted even in non-verbose mode at some point before Rack 3. I'm not sure if that point is now, but the sooner the better in my opinion. Since 2.1 has already been released, I think 2.2 (master) would be a good time to do that.

@ioquatix ioquatix added this to the v3.0.0 milestone Feb 3, 2020
jeremyevans added a commit to jeremyevans/rack that referenced this issue Jul 14, 2020
Additionally, use the :uplevel keyword to warn, so it includes
the caller information in the warning output (similar to rb_warn).
This only works on Ruby 2.5+, so output will look a little odd on
Ruby 2.3/2.4, but I don't think it warrants separate handling in
older versions.

Fixes rack#1277
jeremyevans added a commit to jeremyevans/rack that referenced this issue Jul 14, 2020
Additionally, use the :uplevel keyword to warn, so it includes
the caller information in the warning output (similar to rb_warn).
This only works on Ruby 2.5+, so output will look a little odd on
Ruby 2.3/2.4, but I don't think it warrants separate handling in
older versions.

Fixes rack#1277
jeremyevans added a commit that referenced this issue Jul 14, 2020
Additionally, use the :uplevel keyword to warn, so it includes
the caller information in the warning output (similar to rb_warn).
This only works on Ruby 2.5+, so output will look a little odd on
Ruby 2.3/2.4, but I don't think it warrants separate handling in
older versions.

Fixes #1277
@ioquatix
Copy link
Member Author

Great. The only thing we haven't done is:

I'd like something shorter than Rack::Request#params[], but more "discoverable"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants