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

DEV: Upgrading Discourse to Rails 6 #8083

Merged
merged 9 commits into from Sep 12, 2019

Conversation

@lis2
Copy link
Contributor

commented Sep 9, 2019

Hey Team,

This PR is about upgrading Discourse to Rails 6.0.0 with a classic autoloader.

In addition, I created a topic in meta to discuss those changes - https://meta.discourse.org/t/upgrading-discourse-to-rails-6/128004

@discoursebot

This comment has been minimized.

Copy link

commented Sep 9, 2019

You've signed the CLA, lis2. Thank you! This pull request is ready for review.

@discoursebot

This comment has been minimized.

Copy link

commented Sep 9, 2019

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/upgrading-discourse-to-rails-6/128004/1

@lis2 lis2 changed the title Upgrading Discourse to Rails 6 DEV: Upgrading Discourse to Rails 6 Sep 9, 2019

@gschlager

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

I guess we can remove lib/freedom_patches/rails6.rb when this gets merged.

@eviltrout eviltrout requested a review from SamSaffron Sep 9, 2019

@eviltrout
Copy link
Member

left a comment

This is a remarkably simple PR for an upgrade like this, which is great news. I am assigning to @SamSaffron since he usually has opinions about when to merge these things. But looks good to me, thanks!

lis2 added a commit to lis2/discourse-chat-integration that referenced this pull request Sep 9, 2019
FIX: Save on chat_controller ready for Rails 6.0.0
I think that passing attributes to save method is not necessary when the Object was initialized with them.

Problem is that with Rails 6.0.0 (discourse/discourse#8083) this code is not working https://travis-ci.org/discourse/discourse/jobs/582556988
SamSaffron added a commit to discourse/discourse-chat-integration that referenced this pull request Sep 10, 2019
FIX: Save on chat_controller ready for Rails 6.0.0 (#31)
I think that passing attributes to save method is not necessary when the Object was initialized with them.

Problem is that with Rails 6.0.0 (discourse/discourse#8083) this code is not working https://travis-ci.org/discourse/discourse/jobs/582556988
@@ -26,14 +26,10 @@ def initialize(disposition:, filename:)
@filename = filename
end

TRADITIONAL_ESCAPED_CHAR = /[^ A-Za-z0-9!#$+.^_`|~-]/

This comment has been minimized.

Copy link
@SamSaffron

SamSaffron Sep 10, 2019

Member

wait, do we need any of this file if it is all included in rails 6? Why not just nuke this file?

This comment has been minimized.

Copy link
@gschlager

gschlager Sep 10, 2019

Member

I backported everything below line 13 of that file and that stuff can be safely removed. I'm quite sure the whole file can be deleted.

This comment has been minimized.

Copy link
@lis2

lis2 Sep 10, 2019

Author Contributor

Nice catch! Thank you, I will remove that file

@@ -36,6 +36,7 @@ def committed!(*)

def before_committed!(*); end
def rolledback!(*); end
def trigger_transactional_callbacks?; end

This comment has been minimized.

Copy link
@SamSaffron

SamSaffron Sep 10, 2019

Member

is "no transactional callbacks" the default we are after here?

This comment has been minimized.

Copy link
@lis2

lis2 Sep 10, 2019

Author Contributor

Good question, I think that to stay safe, we should default that to true. I checked how it is used in ActiveRecord and it is used to rollback transaction:

def trigger_transactional_callbacks? # :nodoc:
  (@_new_record_before_last_commit || _trigger_update_callback) && persisted? ||
    _trigger_destroy_callback && destroyed?
end

def rollback_records
  return unless records
  ite = records.uniq(&:object_id)
  already_run_callbacks = {}
  while record = ite.shift
    trigger_callbacks = record.trigger_transactional_callbacks?
    should_run_callbacks = !already_run_callbacks[record] && trigger_callbacks
    already_run_callbacks[record] ||= trigger_callbacks
    record.rolledback!(force_restore_state: full_rollback?, should_run_callbacks: should_run_callbacks)
  end
  ...
end
@SamSaffron

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Overall this looks great, just need to confirm all official plugins have working specs and decide if the rails6 monkey patch needs nuking.

Great work!

@lis2 lis2 force-pushed the lis2:rails6 branch from 57fee13 to a8842ca Sep 11, 2019

@lis2 lis2 force-pushed the lis2:rails6 branch from a8842ca to a922ed0 Sep 11, 2019

@discoursebot

This comment has been minimized.

Copy link

commented Sep 11, 2019

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/upgrading-discourse-to-zeitwerk/128337/1

@SamSaffron

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

We need to merge this sooner rather than later, we have a little window now of a week to merge and then we would need to wait even longer.

I am merging it in, hoping it gets through our CI and then will deploy it to some canary site.

🤞

Thanks heaps @lis2

@SamSaffron SamSaffron merged commit 32b8a2c into discourse:master Sep 12, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.