-
Notifications
You must be signed in to change notification settings - Fork 138
(experimental WIP) Rack 3.x support #325
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
base: master
Are you sure you want to change the base?
Conversation
5da0514
to
9b01b6a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice attempt to support both versions, seems maintainable...
@runtime = @rack_factory.getApplication.getRuntime | ||
should_eval_as_not_nil "defined?(Rack.release)" | ||
should_eval_as_eql_to "Rack.release.to_s[0, 3]", '2.2' | ||
should_eval_as_eql_to "%w(2.2 3.1 3.2).include? Rack.release.to_s[0, 3]", true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO isn't a good test this way, we should maybe duplicate these per each Rails/Rack version tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now that there's "rails80" => %w[rack22 rack31 rack32]
so in that sense I guess this is fine...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's not good, but I couldn't think of a way to pass in the expected version cleanly without checking loaded gems which is kind of circular....
# emulating Rack::Utils.parse_nested_query behavior | ||
raise ::Rack::Utils::ParamsTooDeepError if depth >= 32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't the depth
configurable in Rack itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems just a hard coded security thing :(
8eca6a7
to
52944a0
Compare
52944a0
to
c4cd00e
Compare
Highly WIP/experimental right now as currently involves a rewrite/re-"port-from-rack" of the parameter parsing algorithm, and needs further discussion since this library diverges from Rack in some respects already, and we'll need to decide whether we will persist with these in v3 operation.
https://github.com/rack/rack/blob/main/UPGRADE-GUIDE.md
Will also summarise the open discussion points at some point.
Open questions/discussion point
rack-session
also?rackup
? Or rely on Rails/frameworks to bring them in?HeaderHash
wrapper for error app has any effect on Rack 2.2#call
support is needed