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

Feature/rails 5.2 #3352

Merged
merged 8 commits into from
Jun 13, 2018
Merged

Feature/rails 5.2 #3352

merged 8 commits into from
Jun 13, 2018

Conversation

bricesanchez
Copy link
Member

@bricesanchez bricesanchez commented Mar 7, 2018

We support Rails 5.2, we need to have a green build to merge this PR.

We will have to wait for rails 5.2.0.rc2, see: #3352 (comment)

[UPDATE 2018/06/02]:

@bricesanchez bricesanchez changed the title WIP: Feature/rails 5 2 WIP: Feature/rails 5.2 Mar 8, 2018
@bricesanchez
Copy link
Member Author

Hi @parndt or @simi !

Could you help me to find why we have this error?

Failures:

  1) Refinery::Page custom slugs given a page with a custom_slug exists fails validation when a new record uses that custom_slug
     Failure/Error: new_page.valid?

     NoMethodError:
       undefined method `except!' for nil:NilClass
     # /Users/bricesanchez/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/globalize-5.1.0/lib/globalize/active_record/adapter_dirty.rb:45:in `_reset_attribute'
     # /Users/bricesanchez/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/globalize-5.1.0/lib/globalize/active_record/adapter_dirty.rb:16:in `write'
     # /Users/bricesanchez/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/globalize-5.1.0/lib/globalize/active_record/instance_methods.rb:31:in `write_attribute'
     # /Users/bricesanchez/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/globalize-5.1.0/lib/globalize/active_record/class_methods.rb:96:in `block in define_translated_attr_writer'
     # /Users/bricesanchez/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/bundler/gems/friendly_id-80bc0022910e/lib/friendly_id/slugged.rb:380:in `unset_slug_if_invalid'
     # /Users/bricesanchez/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/activemodel-5.2.0.rc1/lib/active_model/validations/callbacks.rb:118:in `run_validations!'
     # /Users/bricesanchez/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/activemodel-5.2.0.rc1/lib/active_model/validations.rb:339:in `valid?'
     # ./pages/spec/models/refinery/page_url_spec.rb:278:in `block (4 levels) in <module:Refinery>'

Also, should we support Rails 5.1 and 5.2 on Refinery 4.1? Drop support of Rails 5.1 for Refinery 4.1? Start using Appraisal gem or wait for a request from the community?

@bricesanchez bricesanchez added this to the 4.1 milestone Mar 8, 2018
@@ -2,7 +2,7 @@
require File.expand_path('../../core/lib/refinery/version', __FILE__)

version = Refinery::Version.to_s
rails_version = ['>= 5.1.0', '< 5.2']
rails_version = ['>= 5.2.0.rc1', '< 6']
Copy link
Member

Choose a reason for hiding this comment

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

seems like this could stay at >= 5.1.0 and you could specify the rc1 in the Gemfile

Copy link
Member Author

Choose a reason for hiding this comment

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

At this time the only breaking change between Rails 5.1 and 5.2 is this : 70cd41c

So i'm ok to do what to say.

Any thoughts for this last remaining error undefined method except!' for nil:NilClass` ?

Copy link
Member Author

@bricesanchez bricesanchez Mar 8, 2018

Choose a reason for hiding this comment

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

Oh it looks like it's a rails 5.2.0.rc1 bug, mobility gem has also this kind of problem:
shioyama/mobility#164
rails/rails@5fcbdcf

Copy link
Member

Choose a reason for hiding this comment

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

seems like it might be addressed in the next rc

@shioyama
Copy link
Contributor

FWIW, released Mobility 0.5.1 with a fix for 5.2.0.rc2. The problematic change in rc1 was reverted.

@parndt
Copy link
Member

parndt commented Mar 21, 2018

Thanks @shioyama !!

@bricesanchez bricesanchez modified the milestones: 4.1, 5.0 May 5, 2018
@bricesanchez bricesanchez modified the milestone: 5.0 Jun 3, 2018
@bricesanchez bricesanchez changed the title WIP: Feature/rails 5.2 Feature/rails 5.2 Jun 4, 2018
@bricesanchez bricesanchez reopened this Jun 7, 2018
@bricesanchez
Copy link
Member Author

@parndt ready to merge?

@bricesanchez
Copy link
Member Author

🆙 @parndt 🙃

@parndt parndt merged commit e49a6be into master Jun 13, 2018
@parndt parndt deleted the feature/rails-5-2 branch June 13, 2018 05:08
@ttmthanh
Copy link

Sorry sir... When can I use this for Rails 5.2 :(

@bricesanchez
Copy link
Member Author

bricesanchez commented Aug 15, 2018

@ttmthanh You can use it right now in production mode:

# Gemfile

gem 'refinerycms', git: 'https://github.com/refinery/refinerycms', branch: 'master'

We will release the new gem when #3366 will be finished and merged.

@ttmthanh
Copy link

@bricesanchez But... What about 'refinerycms-authentication-devise' and 'refinerycms-inquiries' ? Because when I update rails and refinerycms, it has a bit of a conflict.

@bricesanchez
Copy link
Member Author

@ttmthanh You could use the master branch for refinerycms-inquiries:
https://github.com/refinery/refinerycms-inquiries

For refinerycms-authentication-devise, i've forget to ask Philip for a review of this PR:
refinery/refinerycms-authentication-devise#40

@ttmthanh
Copy link

@bricesanchez Please help me ask him :D Thank you very much 👍 😞

@bricesanchez
Copy link
Member Author

It's already asked :)

@ttmthanh
Copy link

ttmthanh commented Aug 17, 2018

gem 'refinerycms-authentication-devise', git: 'https://github.com/refinery/refinerycms-authentication-devise', branch: 'master'
So I could use like this, right? 😃

@ttmthanh
Copy link

@bricesanchez Sorry sir... Please help me if you are free..

@parndt
Copy link
Member

parndt commented Aug 20, 2018

@ttmthanh I merged it..

@ttmthanh
Copy link

@parndt Thanks so much... Because I am trying to use the Active storage service (Rails 5.2) combined with refinerycms..

@ttmthanh
Copy link

ttmthanh commented Aug 21, 2018

@parndt refinery/refinerycms-page-images#151 Sorry sir...

@bricesanchez
Copy link
Member Author

bricesanchez commented Aug 21, 2018

@ttmthanh

 @parndt Thanks so much... Because I am trying to use the Active storage service (Rails 5.2) combined with refinerycms..

Nice! We just merged a bugfix with ActiveStorage and Refinery #3385

 @parndt refinery/refinerycms-page-images#151 Sorry sir...

You will have to use the feature/mobility branch of refinerycms-page-images until we merge this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants