DEV: Upgrading Discourse to Rails 6 (PR #8083)

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 - Upgrading Discourse to Rails 6 - dev - Discourse Meta

GitHub

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

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

The title of this pull request changed from “Upgrading Discourse to Rails 6” to "DEV: Upgrading Discourse to Rails 6

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

2 Likes

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!

1 Like

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

is “no transactional callbacks” the default we are after here?

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!

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.

1 Like

Nice catch! Thank you, I will remove that file

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

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

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.

:crossed_fingers:

Thanks heaps @lis2