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

Support for Rack 2.0 #137

Merged
merged 1 commit into from
Nov 30, 2017
Merged

Support for Rack 2.0 #137

merged 1 commit into from
Nov 30, 2017

Conversation

sshaw
Copy link
Contributor

@sshaw sshaw commented Oct 14, 2017

This removes the Rack version checks from @pabse's PR in #129 and, based on the conversion there, requires rack ~> 2.0.

Regarding this repo's Rack::Sendfile. Why is this necessary, given Rack::Sendfile in rack?

@mpalmer
Copy link
Contributor

mpalmer commented Oct 17, 2017

Regarding this repo's Rack::Sendfile. Why is this necessary, given Rack::Sendfile in rack?

There is a reasonable possibility that ours may predate the one in Rack, or one of any number of other possibilities. I encourage you to rummage around in the ancient history of both repos if you're particularly curious. There's some eye-opening stuff in there.

@mpalmer
Copy link
Contributor

mpalmer commented Oct 17, 2017

I can't see any show stoppers in the changes you've made. So we're one step closer.

For cleanliness, the minitest deprecation fixes and whitespace cleanups, while necessary, should go into separate PRs, because they can be merged immediately and independently of the Rack 2.0 compatibility work.

This change will break compatibility with any Rack 1.x users (they're still out there -- rack 1.6.8, released in May, has 3.3 million downloads) using rack-contrib, so we'll need to do a major version bump for rack-contrib. I'd like to fix #92 as part of that, too, but the main thing that's going to delay the 2.0 release will be making sure everything is set for maintaining a 1.x branch for a period, to ensure that Rack 1.x users are supported for at least important fixes for a while to come. I have my doubts that I'll have a chance to make that happen this month, but I may get a rush of blood to the head. I'll certainly be more motivated to make it happen quickly if I know this PR is looming over my head, 100% ready to merge. 😀

@mpalmer mpalmer added this to the 2.0 milestone Oct 17, 2017
@sshaw
Copy link
Contributor Author

sshaw commented Oct 17, 2017

Minitest fixes are at #139.

I'll take a look at #92.

@sshaw
Copy link
Contributor Author

sshaw commented Oct 18, 2017

This fixes #92 as well and also removes Rack::Sendfile, which was pretty much the same as core Rack's version without support for body proxies and multiple mappings.

@mpalmer
Copy link
Contributor

mpalmer commented Oct 25, 2017

OK, everything looks good. Thanks for your work.

I'm planning on making the next release a (semi-)final 1.x release, with deprecation warnings for the user-visible changes in this PR (I'm stumped as to how to "deprecate" the changed ordering of the values produced by Rack::NestedParams, but I'll see if any brainwaves develop...). Next month's release will then be the 2.0 release, and the regular monthly release from then on will be on the 2.x series. If any spectacular security bugs appear in the 1.x codebase, they'll be tackled by as-needed releases.

@sshaw
Copy link
Contributor Author

sshaw commented Oct 25, 2017

I'm stumped as to how to "deprecate" the changed ordering of the values produced by Rack::NestedParams

Maybe add the warning at this codepath.

@mpalmer
Copy link
Contributor

mpalmer commented Oct 25, 2017

Brilliant!

@mpalmer
Copy link
Contributor

mpalmer commented Oct 31, 2017

Sadly, on further cogitation, I don't think this will work. We need to emit the deprecation warning when the same variable is encountered twice, not when an array is encountered, which is (AFAICT) what the code path you pointed to is doing. I'm having trouble figuring out how it all works, though, so finding the right place is proving tricky.

@sshaw
Copy link
Contributor Author

sshaw commented Nov 2, 2017

We need to emit the deprecation warning when the same variable is encountered twice, not when an array is encountered...

Hurmmmm, yes.

Okay, then it seems that adding something like this:

warn "DEPRECATION WARNING ...." if top.include?(key) && value.is_a?(String)

above this line would be appropriate.

@sshaw
Copy link
Contributor Author

sshaw commented Nov 2, 2017

I've updated my PR. I had not removed Rack::Sendfile's autoload.

@mpalmer
Copy link
Contributor

mpalmer commented Nov 5, 2017

👍 I've been really sick this week, so the new release (and Rack 2.0 movement) has been delayed.

This removes the Rack version checks from @pabse's PR
(#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
* Rack::AcceptFormat has been removed
@sshaw
Copy link
Contributor Author

sshaw commented Nov 6, 2017

Updated PR, to remove Rack::AcceptFormat. I saw that this was already deprecated.

Also opened #141.

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 this pull request may close these issues.

2 participants