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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update for Rails 5 #12

Merged
merged 7 commits into from Sep 10, 2015
Merged

Conversation

eileencodes
Copy link
Member

Rails 5 changes a lot and actionpack-xm_parser was not compatible with it.

Following changes were made:

  • bumps ActionPack and Rack dependencies
  • Replaces deep_munge with normalize_encode_params
  • Updates #dump_params_keys method in webservice_test to match Rails following Make AC::Parameters not inherited from Hash聽rails#20868
  • fixes deprecation warnings about keyword arguments, rendering plain text, and passing middleware as strings/symbols

Once Rails 5 is released this should not be pointed at Rails master / Rack master but we at Basecamp need this change to use Rails 5 and this gem together. 馃槃

@eileencodes eileencodes force-pushed the update-for-rails5 branch 2 times, most recently from 7c26771 to 65df026 Compare August 26, 2015 15:07
@eileencodes
Copy link
Member Author

I'm not entirely sure what to do here about the ones that are failing. actionpack-xml_parsers won't work on Rails 5 without these changes, but because of these change it doesn't work on older versions of Ruby.

Should future releases only support Ruby 2+ and Rails 5+?

1.9.3 is failing because of changes to caller args from rails/rails/pull/19131
activesupport/lib/active_support/core_ext/module/delegation.rb:170:incaller': wrong number of arguments (2 for 0..1)`

2.0.0 is failing because of changes in (rails/rails/pull/19413)
activesupport/lib/active_support/core_ext/object/json.rb:44:inblock in <top (required)>': private method prepend' called for Enumerable:Module (NoMethodError)

ActionPack has removed `deep_munge` and this gem no longer works with
Rails 5.

This bumps the dependency for ActionPack which requires Rack 2.0 so I
added that to the Gemfile as well since it's unreleased.
`deep_munge` no longer exists. We should now use
`normalize_encode_params`
Following rails/rails#20868 changed how
ActionController::Parameters worked.

This change makes `dump_param_keys` match how the Rails side reads teh
hash keys.

Without this change the right keys don't make it to `Request::Utils` so
the test `test_dasherized_keys_as_xml` was failing because `sub-key`
wasn't being recognized as a key.
Passing middleware as strings or symbols is now deprecated.

```
DEPRECATION WARNING: Passing strings or symbols to the middleware
builder is deprecated, please change
them to actual class references.  For example:

  "ActionDispatch::XmlParamsParser" => ActionDispatch::XmlParamsParser
```
It is now required that HTTP request methods pass keyword arguments
instead of params without keywords.

This fixes the following deprecation warning:

```
DEPRECATION WARNING: ActionDispatch::IntegrationTest HTTP request
methods will accept only the following keyword arguments in future
Rails versions:
params, headers, env, xhr

Examples:

get '/profile',
  params: { id: 1 },
  headers: { 'X-Extra-Header' => '123' },
  env: { 'action_dispatch.custom' => 'custom' },
  xhr: true
```
Fixes deprecation warning to swicth from using `render text:` to `render
plain:`.

```
`render :text` is deprecated because it does not actually render a
`text/plain` response. Switch to `render plain: 'plain text'` to render
as `text/plain`
```
@sikachu
Copy link
Member

sikachu commented Aug 26, 2015

Since Rails 5 is targeting Ruby 2.2+ I can see that this gem requires Ruby 2.2+. We can actually do this:

  • a-xp 1.x requires Ruby 1.9+ / works on Rails ~> 4.0 / on branch rails-4.
  • a-xp 2.x requires Ruby 2.2+ / works on Rails ~> 5.0 / on master.

What do you think?

@eileencodes
Copy link
Member Author

Sounds good to me, @sikachu 馃槃

@sikachu
Copy link
Member

sikachu commented Aug 26, 2015

Can you go ahead and alter .travis.yml in a separate commit (but in this PR) to remove old versions of Rails and Ruby then?

@eileencodes
Copy link
Member Author

Yup will do 馃憤

@@ -17,7 +17,7 @@ Gem::Specification.new do |s|
s.extra_rdoc_files = %w( README.md )
s.rdoc_options.concat ['--main', 'README.md']

s.add_dependency('actionpack', '>= 4.0.0', '< 5')
s.add_dependency('actionpack', '~> 5.x')
Copy link
Member

Choose a reason for hiding this comment

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

what about bumping the gem version to 2.0.x

Copy link
Member

Choose a reason for hiding this comment

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

You commented on a wrong line.

But sure. @eileencodes do you mind changing the version in another commit as well?

Copy link
Member

Choose a reason for hiding this comment

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

i could not comment in line 4. 馃樃

Copy link
Member

Choose a reason for hiding this comment

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

What the heck .. NVM then. I wish they allow you to comment on any line.

@eileencodes
Copy link
Member Author

馃憤 Version updated, old versions of Ruby removed, and Ruby dependency updated.

@robin850
Copy link
Member

It looks like Ruby 2.1 is still present in the build matrix ; otherwise this is looking good, nice work! 馃憤

s.summary = 'XML parameters parser for Action Pack (removed from core in Rails 4.0)'

s.required_ruby_version = '>= 1.9.3'
s.required_ruby_version = '>= 2.2.1'
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be 2.2.2.

Copy link
Member Author

Choose a reason for hiding this comment

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

2.2.1 passes though. I know Rails is set to 2.2.2 but I didn't want to remove 2.2.1 since it does work.

Rails 5 requires Ruby 2.2.2 and will fail on versions lower than 2.2.2.
This change removes the versions that cannot work with the changes to
Rails 5 and actionpack-xml_parser. Updates corresponding .travis.yml
file so that master no longer runs against those.
@eileencodes
Copy link
Member Author

Ok I removed Ruby 2.2.1 and set it so it has to use 2.2.2 since that's what Rails master is using. 馃槃

eileencodes added a commit that referenced this pull request Sep 10, 2015
@eileencodes eileencodes merged commit bee036e into rails:master Sep 10, 2015
@jeremy
Copy link
Member

jeremy commented Sep 11, 2015

馃

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.

None yet

5 participants