Rails 5 Compatibility PR #575

imajes · 2016-05-11T16:13:18Z

There are a couple other PRs to deliver Rails 5 compat, but this one should pass specs, and be mergeable.

I've focused mostly on:

the few small fixes required to actually bring compat
supporting appraisals for rails 4 & 5
re-introducing fuubar, which is now rspec compatible again
silencing or fixing a number of deprecation notices

Regarding the strong params issue, my approach doesn't blanket permit (which is going to be a security vulnerability, due to the abstract way it's implemented and the potential for scriptable exploits). Instead it permits specific, expected params. I don't know if this is sufficient, and we may need to extend this with some of the ATTRIBUTE constants to encapsulate all of the expected params. Will text this further as I integrate administrate into my own app.

There is one deprec notice remaining that is particularly annoying; resolving it removes rails4 test compatibility. I considered wrapping the calls in a deprecation silencer, but felt that isn't a good idea given how it may be missed later. One option may be overriding the specific deprecation notice to silence it.

-- since resolved that with a wrapper def.

Problem: The `bin/merge` script only works for merging in branches on the original repository. Any contributor PRs whose branches live on forks won't be mergeable with the original script. Solution: Create a second merge script that can merge branches from forks of the repo.

rails/rails@70ac072

…sue was fixed since Rails 4.0.beta.

Allows for rails 4/5 concurrent testing

This can't exist here unless that gem is brought in. The gem itself should require itself- otherwise it's a fake dependency.

houndci-bot · 2016-05-11T16:13:25Z

spec/example_app/config/environments/test.rb

+    config.public_file_server.headers = { 'Cache-Control' => 'public, max-age=3600' }
+  else
+    config.serve_static_files   = true
+    config.static_cache_control = 'public, max-age=3600'


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

lukeredpath · 2016-05-13T18:12:01Z

I've just setting up this gem from this branch in a fresh Rails 5 RC1 app. The generator fails with this error:

WARNING: Unable to generate a dashboard for ApplicationRecord.
         It is not connected to a database table.
         Make sure your database migrations are up to date.
      create  app/controllers/admin/application_controller.rb
      create  app/dashboards/_dashboard.rb
/Users/luke/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/railties-5.0.0.rc1/lib/rails/generators/named_base.rb:102:in `map!': undefined method `camelize' for nil:NilClass (NoMethodError)

I haven't dug into the code but I'm guessing its looking for sub-classes of ActiveRecord::Base and not checking to see if they are abstract?

imajes · 2016-05-13T18:28:07Z

@lukeredpath: hey Luke! is your app not hooked up to any db at all? It does appear to use AR::B for the routes generator-- i think that could get switched to just the new AR-- but i didn't have this issue, possibly because i had already setup a DB for my app.

BenMorganIO · 2016-05-13T18:38:43Z

@lukeredpath can you open a new issue for this? I have a feeling its because administrate may not be respecting the abstract_class setting.

BenMorganIO · 2016-05-13T20:04:34Z

Linking this PR here for historical reasons: BenMorganIO#1

lukeredpath · 2016-05-16T10:48:13Z

@BenMorganIO sure, done

BenMorganIO · 2016-05-16T19:37:45Z

Alright, so I just happen to be working on a Rails 5.0.0.rc1 app right now that is getting nicely sizable. @imajes I'll be using your branch this week. I totally want to get your work in. Let's aim for Monday next week after some battle testing for Rails 5 compatibility 😄 .

In the mean time, how do you feel about splitting you +237 lines of code out into smaller PRs? The before_filter => before_action is something we can definitely get in pretty fast. I'm sure there's other hunks of code that can get reviewed and merged on the fast lane as well.

imajes · 2016-05-16T19:46:30Z

@BenMorganIO: do you have write access to this repo?

BenMorganIO · 2016-05-16T22:22:56Z

@imajes that's on the way from what I hear :) cc @Graysonwright

acrogenesis · 2016-05-20T03:58:20Z

If you are using this branch with ruby 2.3.1 it won't build, had to fork this branch and change the Gemfile ruby version. Not sure if thats expected behavior...

Edit: this gets fixed with #591

imajes · 2016-05-22T23:41:50Z

@Graysonwright: so.... any particular reason you haven't responded to this PR?

ACPK · 2016-05-23T17:56:23Z

@Graysonwright @imajes +1

netmask · 2016-05-24T19:13:38Z

This will be merge any time soon ?

BenMorganIO · 2016-05-25T02:32:44Z

spec/controllers/admin/customers_controller_spec.rb

+      {params: args}
+    else
+      args
+    end


You could probably change this to a ternary operator here:

Rails::VERSION::MAJOR >= 5 ? { params: args } : args

more than ternary it should be a guard clause, no?

def kwarg_params(args) return { params: args } if Rails::VERSION::MAJOR >= 5 args end

Yep, that's another solution. I would opt for the ternary mainly because I think the purpose of the method is to format the params if its greater than or equal to Rails 4... If we had to use guard clauses, I think a more optimal outcome would be:

def kwarg_params(args) return args if Rails::VERSION::MAJOR <= 4 { params: args } end

🚲 🏠

Actually this is the worst place to put in any kind of optimal code. This is a yak shaving method to bring about compatibility in a situation where the api has broken.

It will eventually be removed, but till then it should be as self documenting as possible and as plain spoken as possible lest it turn into an actual valuable feature.

On May 24, 2016, at 22:49, Ben A. Morgan notifications@github.com wrote:

In spec/controllers/admin/customers_controller_spec.rb:

expect(response).to redirect_to(admin_customers_url) end

end

def kwarg_params(args)

if Rails::VERSION::MAJOR >= 5

{params: args}

else

args

end
Yep, that's another solution. I would opt for the ternary mainly because I think the purpose of the method is to format the params if its greater than or equal to Rails 4... If we had to use guard clauses, I think a more optimal outcome would be:

return args if Rails::VERSION::MAJOR <= 4
{ params: args }
🚲 🏠

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

acrogenesis · 2016-05-31T04:23:34Z

I made a PR to greetpoint/rails5#2 updating kaminari, because if your dashboard needs pagination it will crash with the old kaminari version

Update Kaminari, bug fix

fx · 2016-06-05T09:51:35Z

@Graysonwright +1

onemanstartup · 2016-06-23T16:11:22Z

Maybe update kaminari to 1.0.0.alpha?

ID25 · 2016-06-28T20:40:35Z

Hey guys, how is your progress?

chamnap · 2016-07-06T14:45:54Z

How is it going, guys?

thomascchen · 2016-10-14T17:24:03Z

Can this PR be closed? It looks like Rails 5 support has been merged in with #636

pedantic-git · 2016-10-17T07:34:00Z

I don't think the master branch is ready for Rails 5 yet. For example, the app/views/administrate/application/_search.html.erb line 17 still generates this error:

"Attempting to generate a URL from non-sanitized request parameters! An attacker can inject malicious data into the generated URL, such as changing the host. Whitelist and sanitize passed parameters to be secure."

I've been using @v-kumar's branch from #656 which resolves all my Rails 5 issues, but the current master does not.

pcantrell · 2016-10-17T21:15:40Z

I’ve been using the master branch with Rails 5 for a few days of development now, and am not having the sanitized params problem. I was on commit 84096c for a while, now e533a6:

$ bundle show administrate
/Users/paul/.rvm/gems/ruby-2.3.0/bundler/gems/administrate-e533a6088521

@pedantic-git, what commit are you showing? ~~Do you perhaps have a customized _search.html.erb that still uses params instead of sanitized_params?~~

pcantrell · 2016-10-17T21:23:37Z

My bad — the fix isn’t in _search.html.erb; it’s in kaminari. All it takes to fix it is bundle update kaminari to get it to version 0.17. An update to this administrate’s gemfile should fix it. I’d already done that in my app, which is why it worked for me.

nickcharlton · 2016-12-17T18:21:09Z

Closing as we now have Rails 5 support in 0.3.0, but please open new issues if we've missed something!

c-lliope and others added 15 commits May 10, 2016 10:31

Rename merge2 -> merge-contrib

5f0e191

Update ruby version, and add a local gemset for tools that support it

7c2da53

Updates Ruby, Rails and Bourbon versions

9d5f730

New API replaces column_types with attribute_types

f82ee4e

rails/rails@70ac072

We can remove this, since administrate is a Rails 4+ app, and this is…

b15a2da

…sue was fixed since Rails 4.0.beta.

Brings f82ee4e into a Rails 4/5 compatibility

38b44c7

Fixes gem version compatibility

a17124c

Allows for rails 4/5 concurrent testing

Add helper to reduce output from rspec backtraces

f45b4c8

Silence/Fix most deprecation notices

b4f3f69

Fix sanitized params errors.

59b2737

Switch before filter > action in template.

3910b94

Remove requirement for field/image

18e140d

This can't exist here unless that gem is brought in. The gem itself should require itself- otherwise it's a fake dependency.

Reintroduce fuubar, handle rails 4/5 difference

9ce1b0f

Restore to work with rails4 also

f82d1dc

houndci-bot reviewed May 11, 2016
View reviewed changes

lukeredpath mentioned this pull request May 16, 2016

Generator not respecting abstract_class #581

Closed

BenMorganIO mentioned this pull request May 22, 2016

Rails 5 compatibility #538

Closed

BenMorganIO reviewed May 25, 2016
View reviewed changes

update kaminari dependency

ec2af96

Merge pull request #2 from DisruptiveAngels/kaminari

3ac84a8

Update Kaminari, bug fix

clemensg mentioned this pull request Jul 1, 2016

Rails 5 support #623

Closed

buren mentioned this pull request Jul 25, 2016

Rails 5 upgrade justarrived/just_match_api#485

Merged

10 tasks

nickcharlton closed this Dec 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rails 5 Compatibility PR #575

Rails 5 Compatibility PR #575

imajes commented May 11, 2016 •

edited

Loading

houndci-bot May 11, 2016

lukeredpath commented May 13, 2016 •

edited

Loading

imajes commented May 13, 2016

BenMorganIO commented May 13, 2016

BenMorganIO commented May 13, 2016

lukeredpath commented May 16, 2016

BenMorganIO commented May 16, 2016 •

edited

Loading

imajes commented May 16, 2016

BenMorganIO commented May 16, 2016 •

edited

Loading

acrogenesis commented May 20, 2016 •

edited

Loading

imajes commented May 22, 2016

ACPK commented May 23, 2016

netmask commented May 24, 2016

BenMorganIO May 25, 2016

acrogenesis May 25, 2016

BenMorganIO May 25, 2016 •

edited

Loading

imajes May 25, 2016 •

edited

Loading

acrogenesis commented May 31, 2016

fx commented Jun 5, 2016

onemanstartup commented Jun 23, 2016

ID25 commented Jun 28, 2016

chamnap commented Jul 6, 2016

thomascchen commented Oct 14, 2016

pedantic-git commented Oct 17, 2016

pcantrell commented Oct 17, 2016 •

edited

Loading

pcantrell commented Oct 17, 2016

nickcharlton commented Dec 17, 2016

Rails 5 Compatibility PR #575

Rails 5 Compatibility PR #575

Conversation

imajes commented May 11, 2016 • edited Loading

houndci-bot May 11, 2016

Choose a reason for hiding this comment

lukeredpath commented May 13, 2016 • edited Loading

imajes commented May 13, 2016

BenMorganIO commented May 13, 2016

BenMorganIO commented May 13, 2016

lukeredpath commented May 16, 2016

BenMorganIO commented May 16, 2016 • edited Loading

imajes commented May 16, 2016

BenMorganIO commented May 16, 2016 • edited Loading

acrogenesis commented May 20, 2016 • edited Loading

imajes commented May 22, 2016

ACPK commented May 23, 2016

netmask commented May 24, 2016

BenMorganIO May 25, 2016

Choose a reason for hiding this comment

acrogenesis May 25, 2016

Choose a reason for hiding this comment

BenMorganIO May 25, 2016 • edited Loading

Choose a reason for hiding this comment

imajes May 25, 2016 • edited Loading

Choose a reason for hiding this comment

acrogenesis commented May 31, 2016

fx commented Jun 5, 2016

onemanstartup commented Jun 23, 2016

ID25 commented Jun 28, 2016

chamnap commented Jul 6, 2016

thomascchen commented Oct 14, 2016

pedantic-git commented Oct 17, 2016

pcantrell commented Oct 17, 2016 • edited Loading

pcantrell commented Oct 17, 2016

nickcharlton commented Dec 17, 2016

imajes commented May 11, 2016 •

edited

Loading

lukeredpath commented May 13, 2016 •

edited

Loading

BenMorganIO commented May 16, 2016 •

edited

Loading

BenMorganIO commented May 16, 2016 •

edited

Loading

acrogenesis commented May 20, 2016 •

edited

Loading

BenMorganIO May 25, 2016 •

edited

Loading

imajes May 25, 2016 •

edited

Loading

pcantrell commented Oct 17, 2016 •

edited

Loading