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.

Sign up for GitHub

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

Jump to bottom

Rack 2.0 #1

Closed
wants to merge 4 commits into from
Closed

Rack 2.0 #1

wants to merge 4 commits into from

Conversation

natemontgomery
Copy link

No description provided.

sshaw and others added 2 commits October 17, 2017 20:23
@sshaw
Support for Rack 2.0
28d4c71 
This removes the Rack version checks from @pabse's PR
(rack#129) and requires
rack ~> 2.0.

Notable Changes:

* Rack::NestedParams's nested query processing has been dropped in favor
of Rack::Utils.parse_nested_query
* Rack::Sendfile has been removed in favor of the version included with
  Rack 2
SI-2191: add missing file lib/rack/contrib/sendfile.rb
c1f6b95
@seerahulsingh
Copy link

@sshaw
We were getting this error cannot load such file -- rack/contrib/sendfile (LoadError)
may be because of the line here -> https://github.com/sshaw/rack-contrib/blob/master/lib/rack/contrib.rb#L37, We removed this line and the file (sendfile.rb) both and now its working for us.

@sshaw
Copy link

sshaw commented Nov 2, 2017

Oh. Can't believe I missed that. Thanks. I've updated my branch/PR.

mojowen
mojowen suggested changes Nov 14, 2017
View reviewed changes
Copy link

@mojowen mojowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question about the test case change

test/spec_rack_nested_params.rb
app = Rack::MethodOverride.new(middleware)
req = app.call(form_post("foo=1&foo=0")).last
req.POST.must_equal({"foo" => '1'})
req.POST.must_equal({"foo" => "0"})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like a subtle and potentially dangerous change - why are changing our the test cases to accommodate?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior will change with 2.0. See this issue (and maybe this too). A deprecation warning was added to v1.8.0.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sshaw makes sense - thanks for the documentation

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

@sshaw sshaw sshaw left review comments

@mojowen mojowen mojowen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@natemontgomery @seerahulsingh @sshaw @mojowen