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

Rails 5 support #1125

Merged
merged 11 commits into from
Jun 20, 2017
Merged

Rails 5 support #1125

merged 11 commits into from
Jun 20, 2017

Conversation

tvdeyen
Copy link
Member

@tvdeyen tvdeyen commented Sep 11, 2016

Rails 5 support

As with mayor Rails updates before, we plan to support Rails 5 with the next mayor version of Alchemy (4.0).

This PR is an ongoing process to make the necessary changes.

@tvdeyen tvdeyen added this to the 4.0 milestone Sep 11, 2016
@tvdeyen tvdeyen self-assigned this Sep 11, 2016
@@ -2,7 +2,7 @@ module Alchemy
module Touching
# Touches the timestamps and userstamps
#
def touch
def touch(*names, time: nil)

Choose a reason for hiding this comment

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

Unused method argument - names. If it's necessary, use _ or names as an argument name to indicate that it won't be used. You can also write as touch() if you want the method to accept any arguments but don't care about them.
Unused method argument - time. You can also write as touch(_) if you want the method to accept any arguments but don't care about them.

@hyiltiz
Copy link

hyiltiz commented Jan 4, 2017

Any estimates on when Rails 5 will be fully supported?

@tvdeyen
Copy link
Member Author

tvdeyen commented Jan 5, 2017

@hyiltiz unfortunately not yet. Working on it. Any help is appreciated.

@tvdeyen tvdeyen force-pushed the rails-5 branch 4 times, most recently from db4a0e4 to 2d17e9a Compare January 8, 2017 22:01
let(:legacy_url4) do
Alchemy::LegacyPageUrl.create(
urlname: 'index.php?option=com_content&view=article&id=48&Itemid=69',
page: second_page)

Choose a reason for hiding this comment

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

Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

@tvdeyen tvdeyen force-pushed the rails-5 branch 3 times, most recently from f2409aa to 92e314f Compare February 24, 2017 18:30
@fastjames
Copy link

I pulled down this branch and ran the test suite locally, and came up with 0 failures (yay!). If the mysql failures in travis were fixed, would this branch be considered for merge prior to 4.0? I'm not planning to use mysql, but if I can help with making it work I don't mind doing so.

@fastjames
Copy link

I was able to replicate the failures with mysql, and they all seem to tie to expected changes to timestamp fields. After some research, I found a patch to rails to enable subsecond resolution for MySQL timestamps so I'm not sure why it does not seem to happen here. The good news is that all 17 failures I saw seem to center around this problem, so one fix will probably green them all.

@fastjames
Copy link

As expected, something is truncating the fractional seconds:

jkane@KingOfTown:~/src/alchemy_cms(rails-5*)% DB=mysql rspec spec/controllers/alchemy/admin/essence_files_controller_spec.rb:70
Run options: include {:focus=>true, :locations=>{"./spec/controllers/alchemy/admin/essence_files_controller_spec.rb"=>[70]}}
before: content updated_at is 2017-03-10 22:30:47.000000000
after: content updated_at is 2017-03-10 22:30:47.000000000

@fastjames
Copy link

According to The MySQL docs the default precision for datetime fields is 0.

I'm happy to create migrations that explicitly set the precision on all timestamp fields to 6. That should match with what postgresql already does, while bringing mysql up to speed.

@fastjames
Copy link

Verified: adding full precision to a test column made the tests related to that column pass. I'll build a migration for all of them and submit it.

@tvdeyen
Copy link
Member Author

tvdeyen commented Mar 10, 2017

Thanks. This is really helpful. I think using a higher precision on timestamps just for MySQL is not what we want to maintain. Instead we should try to fix the specs.

would this branch be considered for merge prior to 4.0

I want to have feature parity between current (yet unreleased) 3.6 and 4.0 (Rails 5 compatible version of Alchemy. Therefore we need to release a 3.6 first. This version needs to include deprecation warnings for stuff we need to change for Rails 5. There is still lot to do.

Unfortunately, my time for working on Alchemy is very limited. So, this will take some time.

Thanks for looking into the MySQL stuff.

@fastjames
Copy link

@phoet did you supply the DB=mysql env var when running the tests? I was able to produce the fails on a fresh install (and a fresh fork) by adding that config tweak.

@mtomov
Copy link
Contributor

mtomov commented Mar 15, 2017

Wow, it's just brilliant how small this PRs, and 70% of the changes are to the dummy app. Great one on bringing the rails 4 alchemy so up to date!

Actually, am I missing something or there isn't a single line of non-test code change in there, which is not Rails 5 specific?

Then what deprecations warning do you want to put in place Thomas? Maybe for some old PRs, which have made a deprecation without documenting it?

@mtomov
Copy link
Contributor

mtomov commented Mar 24, 2017

I want to have feature parity between current (yet unreleased) 3.6 and 4.0 (Rails 5 compatible version of Alchemy. Therefore we need to release a 3.6 first. This version needs to include deprecation warnings for stuff we need to change for Rails 5. There is still lot to do.

Sorry, just wanted to ask if I can help out with writing out those deprecation warnings, or would it be too much to explain?

@etewiah
Copy link

etewiah commented Mar 29, 2017

Great that Rails 5 support is almost ready! I'm looking forward to using Alchemy as the content management part of a real estate website builder I'm working on.

Thanks for all the great work.

@mikel
Copy link

mikel commented Jun 6, 2017

Hey there @tvdeyen, from what I can read, the roadmap to Rails 5 support is:

  1. get a list of deprecations to put into a yet unreleased 3.6 version
  2. publish a 3.6 version
  3. publish a 4.0 version based on this branch

Would that summarise what is needed? If so, I can get a developer perhaps from my company to help push this through. Please let me know.

@tvdeyen
Copy link
Member Author

tvdeyen commented Jun 20, 2017

@mikel yes, this is how I plan to do this.

Thanks for your offer, this is very much appreciated, but you can't actually gems, so I have to do this. But due to lack of time I can't really make this soonish. I don't want to rush this either, as releasing open source software has to be done carefully, for that I need some time to concentrate on.

This is not very satisfy for you, I see that.

You could help us by testing the rails-5 branch in your sites. This is pretty much the same feature set as Alchemy 3.5 with mostly changes to the test suite. So this should be ready for production. Testing this in your sites would really help us to iron out potential issues.

Thomas von Deyen and others added 11 commits June 20, 2017 14:14
Rails 5 only supports 2.2.2+, so we.
It is not needed any more with sprockets-rails 3.1
Rails 5 has dropped support for `assign` and `assert_template` methods.
Rails 5 now wants the database environment set.
With Rails 5 the supported Ruby version was raised to 2.2.2

Adjust the travis configuration to build with Ruby 2.2.6, 2.3.3 and 2.4.0
@tvdeyen tvdeyen changed the title WIP: Rails 5 support Rails 5 support Jun 20, 2017
@tvdeyen tvdeyen removed their assignment Jun 20, 2017
@tvdeyen tvdeyen requested a review from mamhoff June 20, 2017 15:16
@tvdeyen
Copy link
Member Author

tvdeyen commented Jun 20, 2017

@mamhoff just released 3.6, so this is now finally ready to be merged 🎊

Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks to everyone involved!

@tvdeyen tvdeyen merged commit d63cc66 into master Jun 20, 2017
@mtomov
Copy link
Contributor

mtomov commented Jun 20, 2017

Wooow, so sooon? Wow, I really didn't expect it for another few months after your message. Thanks so much all!!

I am really excited about the updated UI in #1261 , so now that this one is merged, I will see to help out there! Thanks again all!

@tvdeyen tvdeyen deleted the rails-5 branch June 20, 2017 19:00
@tvdeyen
Copy link
Member Author

tvdeyen commented Jun 20, 2017

Haha, yeah. I wanted to have this finally done myself. Very annoying to have al this TODOs laying around.

@mtomov
Copy link
Contributor

mtomov commented Jun 20, 2017

Absolutely, know the feeling. Thanks again!

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.

9 participants