Rails 6 support

[parndt]

No description provided.

[parndt]

@joebutler2 I've opened this as a pull request for CI

[joebutler2]

This seems to be most of the way there. There is only one new failing test (note this isn't counting the other failing test that was seen in master.

---

For quick reference here are the two failing tests.

1

The new one for this branch (only in ruby 2.4.5, using mysql, in the pages extension).

  1) Pages new/create allows to easily create nested page
1405     Failure/Error: find("a[href='#{refinery.new_admin_page_path(:parent_id => parent_page.id)}']").click
1406     
1407     Capybara::ElementNotFound:
1408       Unable to find css "a[href='/refinery/pages/new?parent_id=15']"
1409     # ./pages/spec/features/refinery/admin/pages_spec.rb:199:in `block (3 levels) in <module:Admin>'
1410     # -e:1:in `<main>'

Here is a link to the failing job

2

The current one that is failing in master. (It's failing in the 6 jobs related to the pages extension). See this comment for more info.

  2) with multiple locales redirects should redirect to default locale slug
1413     Failure/Error: visit "/#{ru_page_slug_encoded}"
1414     
1415     ActionController::RoutingError:
1416       No route matches [GET] "/%D0%BD%D0%BE%D0%B2%D0%BE%D1%81%D1%82%D0%B8"
1417     # ./vendor/bundle/ruby/2.4.0/gems/request_store-1.4.1/lib/request_store/middleware.rb:19:in `call'
1418     # ./pages/spec/features/refinery/pages_spec.rb:402:in `block (3 levels) in <module:Refinery>'
1419     # -e:1:in `<main>'

[joebutler2]

So after re-triggering the build the only failure we see is the one in master. This should be good to merge into master from what I can see. Although typically I prefer to only merge into master when the build is green. I'll spend some time looking into the failure in master.

[parndt]

It's a really tricky one.. I tried to diagnose it the other day as well. Good luck!! Thank you for your help!

[joebutler2]

It's a really tricky one.. I tried to diagnose it the other day as well. Good luck!! Thank you for your help!

With regards to the broken build in master:

After researching this failure a little more there are a few observations I have:

1. Their is a lot of churn in the ruby / rails ecosystem, including Rails 6 itself. This includes bundler 2.0 being released and used in Travis CI 710eaf5.

2. Since there is a lot of churn in the ecosystem, I think it revealed a concurrency bug, and I believe that's what caused the build to break. Of course this is speculation at this point, since I'm new to this project it's all I have to go on.

3. a. Looking at the failure on ./pages/spec/features/refinery/pages_spec.rb:402, we see within that context Mobility is called and places an ru locale/translation for the test page. When inspecting it in a debugger the translation is missing, causing the failure.
   b. While looking into the Mobility gem, I noticed in their gitter channel that they released version 0.8.8 in order to support Rails 6. That version was released about a week ago.
   c. It seems that refinerycms-i18n locks the mobility version at 0.6.x. I'll go ahead and update that in the rails-6-support PR for refinerycms-i18n. Note that I still need to fix the build for that PR as well.

Please let me know if you have any other thoughts or additional context! :)

[joebutler2]

The pull request for updating refinerycms-i18n is ready to be merged refinery/refinerycms-i18n#80.

Hopefully by updating the mobility gem to 0.8.8, that it will also fix the build for this repository!

Then with that, I believe refinerycms will support Rails 6, or at least be installable ;)

[joebutler2]

I compiled the updated version of refinerycms-i18n locally and now I'm receiving dependency conflicts in bundler. https://gist.github.com/joebutler2/d78c3ed245652663032c7ede84075ef8

I'm hoping to get them resolved quickly!

[joebutler2]

Made more progress on the dependency conflicts. The ones related to refinerycms-i18n have been resolved. Once refinery/refinerycms-i18n#80 is merged in and the gem is built & published, we can push further on this branch.

I'm hoping to resolve the remain dependency conflicts tomorrow!

For a full list of the remaining ones check out https://gist.github.com/joebutler2/d78c3ed245652663032c7ede84075ef8

[joebutler2]

Was able to resolve all of the dependency conflicts locally (with a custom rubygem server). Now there is a loading issue with refinerycms-core.

Thank you @parndt for merging the PR for refinerycms-i18n!

Once that gem is released I'll update this PR. Also had to update the decorators gem as well (See this PR for details).

I'll keep working on this, hopefully, it'll be up and running in the next couple of days. Then we can wrap up this PR so everyone else can use RefineryCMS on Rails 6!

With that said, since there has been so much work around resolving dependencies, I'm wondering if we should bump up the minor version (e.g. 4.1.0) instead of the patch version (e.g. 4.0.4) when this branch is ready to merge in. Thoughts?

[parndt]

@joebutler2

Successfully registered gem: refinerycms-i18n (5.0.1)

[joebutler2]

I know this needs a little more work to get across the finish line and merged in. I'm hoping to spend some time in the next day or so to wrap it up.

Everything is working fine locally (based on my gem server).

The only other dependency we need updated and released is decorators. I have the pull request ready at parndt/decorators#19.

I hope everyone else is excited as I am about this!

[joebutler2]

Hmm, updating mobility to 0.8.8 didn't fix the build as I had hoped. I checked the bundle install output to ensure it installed 0.8.8.

[parndt]

@joebutler2 do you think it's worth asking in the Mobility Gitter whether anybody knows what's going on with this spec? Otherwise I fear we might have to either spend a lot of time on it, or delete it.

[joebutler2]

@parndt yeah, that's a good idea.

Another option might be to place it in a separate test run/configuration. Since it looks like a race condition in a multi-threaded environment, maybe running the test separately with Puma configured with 1 thread would avoid this failure.

Though that wouldn't solve the issue in a production environment. Since there haven't been any issues filed for random missing translations, I'm not too worried. There are too many variables (with dependencies & etc) for us to be certain that the root cause is solved (without spending a significant amount of time).

[joebutler2]

@parndt when you have the time could you release version 2.0.5 of decorators?

We need that for Rails 6 support. Transitive dependencies... 🤷‍♂

[parndt]

@parndt when you have the time could you release version 2.0.5 of decorators?

@joebutler2 it is pushed:

Successfully registered gem: decorators (2.0.5)

[joebutler2]

With regards to the failure in master, it seems trickier than I originally thought. While updating refinerycms-blog to support Rails 6 there were a ton of failures related to missing translations.

PR: refinery/refinerycms-blog#512
Build Failures: https://travis-ci.org/refinery/refinerycms-blog/jobs/593166886

While everything in core and the blog seems fine on my project, the truth is we aren't using translations. So it looks like we're going to have to do a deep dive to get it resolved.

[evenreven]

Comment from the sidelines: most of the unexpected behaviour and bugs I've found in Refinery and associated gems in the five years since we started using it have been related to translation/i18n, so I'm happy that you're taking this aspect seriously despite not using it yourself. 👍

[parndt]

refinery/refinerycms-authentication-devise#45 merged

[bricesanchez]

This changes has been merged in #3446. Thanks @joebutler2